Created attachment 2787 [details] Patch file for this bug report Hello All, In reviewing code in OpenSSH-7.1p2, there are some instances where free() is called in file 'auth1.c', in which the contents are not sanitized before free() is called. The patch file below should address these instances: --- auth1.c.orig 2016-02-13 18:13:35.016278903 -0800 +++ auth1.c 2016-02-13 18:18:43.853530529 -0800 @@ -207,6 +207,7 @@ debug("sending challenge '%s'", challenge); packet_start(SSH_SMSG_AUTH_TIS_CHALLENGE); packet_put_cstring(challenge); + explicit_bzero(challenge, sizeof(*challenge)); free(challenge); packet_send(); packet_write_wait(); @@ -356,6 +357,7 @@ /* Log before sending the reply */ auth_log(authctxt, authenticated, 0, get_authname(type), NULL); + explicit_bzero(client_user, sizeof(*client_user)); free(client_user); client_user = NULL; ======================================================================= In the case of variable 'client_user', calling free() and setting the static char pointer to NULL does not explicitly scrub any of the contents past the first character, does it not? In reviewing code in openssh-7.1p2, there are some instances where free() is called in file 'auth2.c', in which the contents are not sanitized before free() is called. The patch file below should address these instances: --- auth2.c.orig 2016-02-13 18:43:48.284735999 -0800 +++ auth2.c 2016-02-13 18:50:11.694465404 -0800 @@ -125,6 +125,7 @@ close(fd); if (n != len) { + explicit_bzero(banner, sizeof(*banner)); free(banner); return (NULL); } @@ -159,6 +160,7 @@ userauth_send_banner(banner); done: + explicit_bzero(banner, sizeof(*banner)); free(banner); } @@ -204,6 +206,7 @@ debug("bad service request %s", service); packet_disconnect("bad service request %s", service); } + explicit_bzero(service, sizeof(*service)) free(service); return 0; } @@ -282,8 +285,11 @@ } userauth_finish(authctxt, authenticated, method, NULL); + explicit_bzero(service, sizeof(*service)); free(service); + explicit_bzero(user, sizeof(*user)); free(user); + explicit_bzero(method, sizeof(*method)); free(method); return 0; } @@ -373,6 +379,7 @@ packet_put_char(partial); packet_send(); packet_write_wait(); + explicit_bzero(methods, sizeof(*methods)); free(methods); } } @@ -491,6 +498,7 @@ } ret = 0; out: + explicit_bzero(omethods, sizeof(*omethods)); free(omethods); return ret; } @@ -581,6 +589,7 @@ if (*p == ',') p++; *methods = xstrdup(p); + explicit_bzero(omethods, sizeof(*omethods)); free(omethods); return 1; } ======================================================================= In reviewing code in openssh-7.1p2, there are some instances where free() is called in file 'auth2-hostbased.c', in which the contents are not sanitized before free() is called. The patch file below should address these instances: --- auth2-hostbased.c.orig 2016-02-13 19:00:19.828756146 -0800 +++ auth2-hostbased.c 2016-02-13 19:04:31.173700796 -0800 @@ -147,10 +147,15 @@ debug2("userauth_hostbased: authenticated %d", authenticated); if (key != NULL) key_free(key); + explicit_bzero(pkalg, sizeof(*pkalg)); free(pkalg); + explicit_bzero(pkblob, sizeof(*pkblob)); free(pkblob); + explicit_bzero(cuser, sizeof(*cuser)); free(cuser); + explicit_bzero(chost, sizeof(*chost)); free(chost); + explicit_bzero(sig, sizeof(*sig)); free(sig); return authenticated; } @@ -237,6 +242,7 @@ verbose("Accepted %s public key %s from %s@%s", key_type(key), fp, cuser, lookup); } + explicit_bzero(fp, sizeof(*fp)); free(fp); } ======================================================================= I am attaching the patch file(s) to this bug report...
Created attachment 2788 [details] Patch file for this bug report
Created attachment 2789 [details] Patch file for this bug report
Note that none of these changes actually does what you intend; they all only overwrite the first byte. All of the variables are a pointer to a char type, so sizeof(what is pointed to by this pointer) = 1. (In reply to Bill Parker from comment #0) > + explicit_bzero(challenge, sizeof(*challenge)); This is not sensitive; it's sent by the server to the client pre-authentication. > + explicit_bzero(client_user, sizeof(*client_user)); This is not sensitive either, the client sends it pre-auth and both client and server know it. > In the case of variable 'client_user', calling free() and setting the > static char pointer to NULL does not explicitly scrub any of the > contents past the first character, does it not? It doesn't scrub any of the content of the string at all, it just sets the pointer to NULL and leaves the content intact. > + explicit_bzero(banner, sizeof(*banner)); banner is not sensitive, it's sent to the client pre-auth. > + explicit_bzero(service, sizeof(*service)); > free(service); > + explicit_bzero(user, sizeof(*user)); > free(user); > + explicit_bzero(method, sizeof(*method)); These are not sensitive; they'e basically "I'd like to log in as this user via these methods") and both client and server know them. > + explicit_bzero(omethods, sizeof(*omethods)); This is just a list of valid authentication methods before we removed the one that we just did. Both client and server know them. > + explicit_bzero(pkblob, sizeof(*pkblob)); > free(pkblob); This is the only one that might be valid although I doubt it. It's a signed challenge from the client, so both client and server know it, and because the challenge is random it would be of no use to anyone else. If you were going to do this you would need to pass the length of the blob, and because it's binary you couldn't use strlen, you'd need to use the length reported by packet_get_string above (ie "blen"). > + explicit_bzero(cuser, sizeof(*cuser)); > free(cuser); > + explicit_bzero(chost, sizeof(*chost)); > free(chost); > + explicit_bzero(sig, sizeof(*sig)); > free(sig); These aren't sensitive. > + explicit_bzero(fp, sizeof(*fp)); The fingerprint is not sensitive, it's stored in the clear in the system logs and is of no value for authenticating.
Close RESOLVED bugs with the release of openssh-8.0