openssh bugs - Feb 2016 - [Bug 2541] New: Add explicit_bzero() before free() in OpenSSH-7.1p2 for auth1.c/auth2.c/auth2-hostbased.c

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

            Bug ID: 2541
           Summary: Add explicit_bzero() before free() in OpenSSH-7.1p2
                    for auth1.c/auth2.c/auth2-hostbased.c
           Product: Portable OpenSSH
           Version: 7.1p1
          Hardware: All
                OS: All
            Status: NEW
          Severity: normal
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: wp02855 at gmail.com

Created attachment 2787
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2787&action=edit
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...

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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

Bill Parker <wp02855 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2788|                            |ok?
              Flags|                            |

--- Comment #1 from Bill Parker <wp02855 at gmail.com> ---
Created attachment 2788
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2788&action=edit
Patch file for this bug report

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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

Bill Parker <wp02855 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2789|                            |ok?
              Flags|                            |

--- Comment #2 from Bill Parker <wp02855 at gmail.com> ---
Created attachment 2789
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2789&action=edit
Patch file for this bug report

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |WONTFIX
                 CC|                            |djm at mindrot.org
             Status|NEW                         |RESOLVED

-- 
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.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
Close RESOLVED bugs with the release of openssh-8.0

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