From 063e1a255b53abde1147522f9aceccfd2a7ceb9b Mon Sep 17 00:00:00 2001
From: Jakub Jelen <jjelen@redhat.com>
Date: Tue, 2 Mar 2021 19:45:25 +0100
Subject: [PATCH] Unbreak gsi-openssh by not holding the sshbuf structures
 originated from incoming packet buffer

Keeping buffers from sshpkt_getb_froms() for breaks further packet
processing because the "derived" buffer keeps a "link" to te parent
buffer, which is then considered readonly.

This addresses the visible issue in the kexgss_server(), which
demonstrated with GSI (requiring more round trips than GSSAPI
with kerberos).

The additional two places in the client were never hit, because the host
keys are never sent as part of the gssapi key exchange but in case we
would have different server or we would start sending hostkeys as the
code is ready for that, we would hit it anyway.

Fixes #18
---
 kexgssc.c | 14 ++++++++++++--
 kexgsss.c | 48 ++++++++++++++++++++++++++++--------------------
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/kexgssc.c b/kexgssc.c
index 1c62740e..29b8b031 100644
--- a/kexgssc.c
+++ b/kexgssc.c
@@ -162,11 +162,16 @@ kexgss_client(struct ssh *ssh)
 			do {
 				type = ssh_packet_read(ssh);
 				if (type == SSH2_MSG_KEXGSS_HOSTKEY) {
+					char *tmp = NULL;
+					size_t tmp_len = 0;
+
 					debug("Received KEXGSS_HOSTKEY");
 					if (server_host_key_blob)
 						fatal("Server host key received more than once");
-					if ((r = sshpkt_getb_froms(ssh, &server_host_key_blob)) != 0)
+					if ((r = sshpkt_get_string(ssh, &tmp, &tmp_len)) != 0)
 						fatal("Failed to read server host key: %s", ssh_err(r));
+					if ((server_host_key_blob = sshbuf_from(tmp, tmp_len)) == NULL)
+						fatal("sshbuf_from failed");
 				}
 			} while (type == SSH2_MSG_KEXGSS_HOSTKEY);
 
@@ -453,11 +458,16 @@ kexgssgex_client(struct ssh *ssh)
 			do {
 				type = ssh_packet_read(ssh);
 				if (type == SSH2_MSG_KEXGSS_HOSTKEY) {
+					char *tmp = NULL;
+					size_t tmp_len = 0;
+
 					debug("Received KEXGSS_HOSTKEY");
 					if (server_host_key_blob)
 						fatal("Server host key received more than once");
-					if ((r = sshpkt_getb_froms(ssh, &server_host_key_blob)) != 0)
+					if ((r = sshpkt_get_string(ssh, &tmp, &tmp_len)) != 0)
 						fatal("sshpkt failed: %s", ssh_err(r));
+					if ((server_host_key_blob = sshbuf_from(tmp, tmp_len)) == NULL)
+						fatal("sshbuf_from failed");
 				}
 			} while (type == SSH2_MSG_KEXGSS_HOSTKEY);
 
diff --git a/kexgsss.c b/kexgsss.c
index a2c02148..c8b7d652 100644
--- a/kexgsss.c
+++ b/kexgsss.c
@@ -64,7 +64,7 @@ kexgss_server(struct ssh *ssh)
 	 */
 
 	OM_uint32 ret_flags = 0;
-	gss_buffer_desc gssbuf, recv_tok, msg_tok;
+	gss_buffer_desc gssbuf = {0, NULL}, recv_tok, msg_tok;
 	gss_buffer_desc send_tok = GSS_C_EMPTY_BUFFER;
 	Gssctxt *ctxt = NULL;
 	struct sshbuf *shared_secret = NULL;
@@ -104,7 +104,7 @@ kexgss_server(struct ssh *ssh)
 		type = ssh_packet_read(ssh);
 		switch(type) {
 		case SSH2_MSG_KEXGSS_INIT:
-			if (client_pubkey != NULL)
+			if (gssbuf.value != NULL)
 				fatal("Received KEXGSS_INIT after initialising");
 			if ((r = ssh_gssapi_sshpkt_get_buffer_desc(ssh,
 			        &recv_tok)) != 0 ||
@@ -135,6 +135,31 @@ kexgss_server(struct ssh *ssh)
 				goto out;
 
 			/* Send SSH_MSG_KEXGSS_HOSTKEY here, if we want */
+
+			/* Calculate the hash early so we can free the
+			* client_pubkey, which has reference to the parent
+			* buffer state->incoming_packet
+			*/
+			hashlen = sizeof(hash);
+			if ((r = kex_gen_hash(
+			    kex->hash_alg,
+			    kex->client_version,
+			    kex->server_version,
+			    kex->peer,
+			    kex->my,
+			    empty,
+			    client_pubkey,
+			    server_pubkey,
+			    shared_secret,
+			    hash, &hashlen)) != 0)
+				goto out;
+
+			gssbuf.value = hash;
+			gssbuf.length = hashlen;
+
+			sshbuf_free(client_pubkey);
+			client_pubkey = NULL;
+
 			break;
 		case SSH2_MSG_KEXGSS_CONTINUE:
 			if ((r = ssh_gssapi_sshpkt_get_buffer_desc(ssh,
@@ -156,7 +181,7 @@ kexgss_server(struct ssh *ssh)
 		if (maj_status != GSS_S_COMPLETE && send_tok.length == 0)
 			fatal("Zero length token output when incomplete");
 
-		if (client_pubkey == NULL)
+		if (gssbuf.value == NULL)
 			fatal("No client public key");
 
 		if (maj_status & GSS_S_CONTINUE_NEEDED) {
@@ -185,23 +210,6 @@ kexgss_server(struct ssh *ssh)
 	if (!(ret_flags & GSS_C_INTEG_FLAG))
 		fatal("Integrity flag wasn't set");
 
-	hashlen = sizeof(hash);
-	if ((r = kex_gen_hash(
-	    kex->hash_alg,
-	    kex->client_version,
-	    kex->server_version,
-	    kex->peer,
-	    kex->my,
-	    empty,
-	    client_pubkey,
-	    server_pubkey,
-	    shared_secret,
-	    hash, &hashlen)) != 0)
-		goto out;
-
-	gssbuf.value = hash;
-	gssbuf.length = hashlen;
-
 	if (GSS_ERROR(PRIVSEP(ssh_gssapi_sign(ctxt, &gssbuf, &msg_tok))))
 		fatal("Couldn't get MIC");