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
-------------- next part --------------
diff --git a/sshconnect2.c b/sshconnect2.c
index 0e8f323..0dab40a 100644
--- a/sshconnect2.c
+++ b/sshconnect2.c
@@ -264,6 +264,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;
@@ -307,11 +312,11 @@ int userauth_hostbased(Authctxt *);
#ifdef GSSAPI
int userauth_gssapi(Authctxt *authctxt);
-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(Authctxt *authctxt);
+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(Authctxt *, char *);
@@ -330,7 +335,7 @@ Authmethod authmethods[] = {
#ifdef GSSAPI
{"gssapi-with-mic",
userauth_gssapi,
- NULL,
+ userauth_gssapi_cleanup,
&options.gss_authentication,
NULL},
#endif
@@ -389,6 +394,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) {
@@ -683,26 +692,24 @@ userauth_gssapi(Authctxt *authctxt)
{
struct ssh *ssh = active_state; /* XXX */
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++;
}
}
@@ -717,13 +724,13 @@ userauth_gssapi(Authctxt *authctxt)
(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));
@@ -732,11 +739,23 @@ userauth_gssapi(Authctxt *authctxt)
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(Authctxt *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)
{
Markus Schmidt
2019-Feb-05 10:37 UTC
[PATCH] fix for older patch - Re: [PATCH] ssh-client gssapi leak and code cleanup.
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) {