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.c23 Jun 2019 12:21:46 -  1.77
+++ sshkey.c27 Jun 2019 03:29:39 -
@@ -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(, sizeof(tmp));
+   freezero(enc, enclen);
freezero(prekey, SSHKEY_SHIELD_PREKEY_LEN);
sshkey_free(kswap);
sshbuf_free(prvbuf);

Potential null pointer dereference in sshkey shielding

[Reynir Björnsson]

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.

The relevant code in sshkey_unshield_private():

/* ... */
/* encrypt */
enclen = sshbuf_len(prvbuf);
if ((enc = malloc(enclen)) == NULL) {
r = SSH_ERR_ALLOC_FAIL;
goto out;
}
/* SNIP... */
 out:
/* ... */
explicit_bzero(enc, enclen);
/* ... */

A fix is to set enclen to zero before doing cleanup:

---
 usr.bin/ssh/sshkey.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/usr.bin/ssh/sshkey.c b/usr.bin/ssh/sshkey.c
index 9d90c9a51a3..a6015a16939 100644
--- a/usr.bin/ssh/sshkey.c
+++ b/usr.bin/ssh/sshkey.c
@@ -1910,6 +1910,8 @@ sshkey_shield_private(struct sshkey *k)
enclen = sshbuf_len(prvbuf);
if ((enc = malloc(enclen)) == NULL) {
r = SSH_ERR_ALLOC_FAIL;
+   /* set enclen to zero so we don't explicit_bzero() NULL */
+   enclen = 0;
goto out;
}
if ((r = cipher_crypt(cctx, 0, enc,
-- 
2.22.0




signature.asc
Description: OpenPGP digital signature