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

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]

 




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@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev


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) {
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux