[PATCH] fix for older patch - Re: [PATCH] ssh-client gssapi leak and code cleanup.

[Markus Schmidt]

https://bugzilla.mindrot.org/show_bug.cgi?id=2952


The patch below was sent some time ago, but not applied.  It now doesn't 
apply cleanly anymore due to the removal of the opacket api.

I'm attaching a new patch.

It also slightly changes the call of pubkey_cleanup, now adding it to 
the method table as a cleanup handler, rather than calling it explicitely.

Patch attached here and in bugzilla 2952

Best regards.


Markus Schmidt


On 01.08.19 11:46 , Markus Schmidt wrote:
> 
> https://bugzilla.mindrot.org/show_bug.cgi?id=2952
> 
> userauth_gssapi allocates a bit of memory for the authctxt->methoddata 
> pointer but doesn't clean up.
> 
> Side issue: userauth_gssapi is also using two function-static variables. 
>   One of these leaks.  The other one makes prevents reusability (e.g. 
> porting to OO languages) because there is no way to reset it.  They 
> should be moved to authctxt.
> 
> Another side issue: some gssapi-userauth related functions could be made 
> static and there is a function prototype (input_gssapi_hash) that is no 
> longer used.
> 
> Patch attached.
> 
> 
> Markus
> 
> 
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev at mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
> 

-------------- next part --------------
diff --git a/sshconnect2.c b/sshconnect2.c
index 2aa7b99..8ce309b 100644
--- a/sshconnect2.c
+++ b/sshconnect2.c
@@ -265,6 +265,11 @@ struct cauthctxt {
 	struct cauthmethod *method;
 	sig_atomic_t success;
 	char *authlist;
+#ifdef GSSAPI
+	/* gssapi */
+	gss_OID_set gss_supported_mechs;
+	u_int mech_tried;
+#endif
 	/* pubkey */
 	struct idlist keys;
 	int agent_fd;
@@ -302,24 +307,25 @@ int	input_userauth_passwd_changereq(int, u_int32_t, struct ssh *);
 
 int	userauth_none(struct ssh *);
 int	userauth_pubkey(struct ssh *);
+void	userauth_pubkey_cleanup(struct ssh *);
 int	userauth_passwd(struct ssh *);
 int	userauth_kbdint(struct ssh *);
 int	userauth_hostbased(struct ssh *);
 
 #ifdef GSSAPI
+<<<<<<< HEAD
 int	userauth_gssapi(struct ssh *);
-int	input_gssapi_response(int type, u_int32_t, struct ssh *);
-int	input_gssapi_token(int type, u_int32_t, struct ssh *);
-int	input_gssapi_hash(int type, u_int32_t, struct ssh *);
-int	input_gssapi_error(int, u_int32_t, struct ssh *);
-int	input_gssapi_errtok(int, u_int32_t, struct ssh *);
+void	userauth_gssapi_cleanup(struct ssh *);
+static int input_gssapi_response(int type, u_int32_t, struct ssh *);
+static int input_gssapi_token(int type, u_int32_t, struct ssh *);
+static int input_gssapi_error(int, u_int32_t, struct ssh *);
+static int input_gssapi_errtok(int, u_int32_t, struct ssh *);
 #endif
 
 void	userauth(struct ssh *, char *);
 
 static int sign_and_send_pubkey(struct ssh *ssh, Identity *);
 static void pubkey_prepare(Authctxt *);
-static void pubkey_cleanup(Authctxt *);
 static void pubkey_reset(Authctxt *);
 static struct sshkey *load_identity_file(Identity *);
 
@@ -331,7 +337,7 @@ Authmethod authmethods[] = {
 #ifdef GSSAPI
 	{"gssapi-with-mic",
 		userauth_gssapi,
-		NULL,
+		userauth_gssapi_cleanup,
 		&options.gss_authentication,
 		NULL},
 #endif
@@ -342,7 +348,7 @@ Authmethod authmethods[] = {
 		NULL},
 	{"publickey",
 		userauth_pubkey,
-		NULL,
+		userauth_pubkey_cleanup,
 		&options.pubkey_authentication,
 		NULL},
 	{"keyboard-interactive",
@@ -390,6 +396,10 @@ ssh_userauth2(struct ssh *ssh, const char *local_user,
 	authctxt.info_req_seen = 0;
 	authctxt.attempt_kbdint = 0;
 	authctxt.attempt_passwd = 0;
+#if GSSAPI
+	authctxt.gss_supported_mechs = NULL;;
+	authctxt.mech_tried = 0;
+#endif
 	authctxt.agent_fd = -1;
 	pubkey_prepare(&authctxt);
 	if (authctxt.method == NULL) {
@@ -409,7 +419,6 @@ ssh_userauth2(struct ssh *ssh, const char *local_user,
 	ssh_dispatch_run_fatal(ssh, DISPATCH_BLOCK, &authctxt.success);	/* loop until success */
 	ssh->authctxt = NULL;
 
-	pubkey_cleanup(&authctxt);
 	ssh_dispatch_range(ssh, SSH2_MSG_USERAUTH_MIN, SSH2_MSG_USERAUTH_MAX, NULL);
 
 	if (!authctxt.success)
@@ -685,26 +694,24 @@ userauth_gssapi(struct ssh *ssh)
 {
 	Authctxt *authctxt = (Authctxt *)ssh->authctxt;
 	Gssctxt *gssctxt = NULL;
-	static gss_OID_set gss_supported = NULL;
-	static u_int mech = 0;
 	OM_uint32 min;
 	int r, ok = 0;
 
 	/* Try one GSSAPI method at a time, rather than sending them all at
 	 * once. */
 
-	if (gss_supported == NULL)
-		gss_indicate_mechs(&min, &gss_supported);
+	if (authctxt->gss_supported_mechs == NULL)
+		gss_indicate_mechs(&min, &authctxt->gss_supported_mechs);
 
 	/* Check to see if the mechanism is usable before we offer it */
-	while (mech < gss_supported->count && !ok) {
+	while (authctxt->mech_tried < authctxt->gss_supported_mechs->count && !ok) {
 		/* My DER encoding requires length<128 */
-		if (gss_supported->elements[mech].length < 128 &&
+		if (authctxt->gss_supported_mechs->elements[authctxt->mech_tried].length < 128 &&
 		    ssh_gssapi_check_mechanism(&gssctxt,
-		    &gss_supported->elements[mech], authctxt->host)) {
+		    &authctxt->gss_supported_mechs->elements[authctxt->mech_tried], authctxt->host)) {
 			ok = 1; /* Mechanism works */
 		} else {
-			mech++;
+			authctxt->mech_tried++;
 		}
 	}
 
@@ -719,13 +726,13 @@ userauth_gssapi(struct ssh *ssh)
 	    (r = sshpkt_put_cstring(ssh, authctxt->method->name)) != 0 ||
 	    (r = sshpkt_put_u32(ssh, 1)) != 0 ||
 	    (r = sshpkt_put_u32(ssh,
-	    (gss_supported->elements[mech].length) + 2)) != 0 ||
+	    (authctxt->gss_supported_mechs->elements[authctxt->mech_tried].length) + 2)) != 0 ||
 	    (r = sshpkt_put_u8(ssh, SSH_GSS_OIDTYPE)) != 0 ||
 	    (r = sshpkt_put_u8(ssh,
-	    gss_supported->elements[mech].length)) != 0 ||
+	    authctxt->gss_supported_mechs->elements[authctxt->mech_tried].length)) != 0 ||
 	    (r = sshpkt_put(ssh,
-	    gss_supported->elements[mech].elements,
-	    gss_supported->elements[mech].length)) != 0 ||
+	    authctxt->gss_supported_mechs->elements[authctxt->mech_tried].elements,
+	    authctxt->gss_supported_mechs->elements[authctxt->mech_tried].length)) != 0 ||
 	    (r = sshpkt_send(ssh)) != 0)
 		fatal("%s: %s", __func__, ssh_err(r));
 
@@ -734,11 +741,24 @@ userauth_gssapi(struct ssh *ssh)
 	ssh_dispatch_set(ssh, SSH2_MSG_USERAUTH_GSSAPI_ERROR, &input_gssapi_error);
 	ssh_dispatch_set(ssh, SSH2_MSG_USERAUTH_GSSAPI_ERRTOK, &input_gssapi_errtok);
 
-	mech++; /* Move along to next candidate */
+	authctxt->mech_tried++; /* Move along to next candidate */
 
 	return 1;
 }
 
+void
+userauth_gssapi_cleanup(struct ssh *ssh)
+{
+	Authctxt *authctxt = (Authctxt *)ssh->authctxt;
+
+	Gssctxt *gssctxt = (Gssctxt *)authctxt->methoddata;
+	ssh_gssapi_delete_ctx(&gssctxt);
+	authctxt->methoddata = NULL;
+
+	free(authctxt->gss_supported_mechs);
+	authctxt->gss_supported_mechs = NULL;
+}
+
 static OM_uint32
 process_gssapi_token(struct ssh *ssh, gss_buffer_t recv_tok)
 {
@@ -1618,9 +1638,11 @@ pubkey_prepare(Authctxt *authctxt)
 	debug2("%s: done", __func__);
 }
 
-static void
-pubkey_cleanup(Authctxt *authctxt)
+void
+userauth_pubkey_cleanup(struct ssh *ssh)
 {
+	Authctxt *authctxt = (Authctxt *)ssh->authctxt;
+
 	Identity *id;
 
 	if (authctxt->agent_fd != -1) {