[Bug 2541] Add explicit_bzero() before free() in OpenSSH-7.1p2 for auth1.c/auth2.c/auth2-hostbased.c

[bugzilla-daemon at bugzilla.mindrot.org]

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

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au

--- Comment #3 from Darren Tucker <dtucker at zip.com.au> ---
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.