Re: Potential null pointer dereference in sshkey shielding

[Damien Miller]

On Wed, 26 Jun 2019, Reynir Björnsson wrote:

> Hello,
> 
> I have noticed a potential NULL pointer dereference in the recent code
> for ssh key shielding. Essentially, during error handling
> explicit_bzero(enc, enclen) is called. This should be fine when enc is
> NULL as long as enclen is zero. However, there is one spot in the code
> where a malloc() call for enc could fail where enclen could be non-zero.

Thanks for looking at the code.

I think my mistake was not reaching for freezero() - this also fixes the
leak of enc on other error paths. It is guaranteed to be a no-op if its
first argument is NULL.

Index: sshkey.c
===================================================================
RCS file: /cvs/src/usr.bin/ssh/sshkey.c,v
retrieving revision 1.77
diff -u -p -r1.77 sshkey.c
--- sshkey.c    23 Jun 2019 12:21:46 -0000      1.77
+++ sshkey.c    27 Jun 2019 03:29:39 -0000
@@ -1943,9 +1943,9 @@ sshkey_shield_private(struct sshkey *k)
  out:
        /* XXX behaviour on error - invalidate original private key? */
        cipher_free(cctx);
-       explicit_bzero(enc, enclen);
        explicit_bzero(keyiv, sizeof(keyiv));
        explicit_bzero(&tmp, sizeof(tmp));
+       freezero(enc, enclen);
        freezero(prekey, SSHKEY_SHIELD_PREKEY_LEN);
        sshkey_free(kswap);
        sshbuf_free(prvbuf);