openssh unix dev - Apr 2020 - [PATCH] Make SSH2 private key parsing errors fatal

This matches the other do_convert_* functions, which also cannot
fail. Otherwise, ssh-keygen will crash when it tries to check the
key type before writing it to stdout.

For example, if I corrupt the key magic:

$ sed 's,^P2/5,AAAA,' regress/rsa_ssh2.prv > bad.prv && chmod
600 bad.prv
$ ssh-keygen -i -f bad.prv
bad magic 0xeb != 0x3f6ff9eb
Segmentation fault
$
---
 ssh-keygen.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/ssh-keygen.c b/ssh-keygen.c
index d50ca5f2..6c160f0c 100644
--- a/ssh-keygen.c
+++ b/ssh-keygen.c
@@ -487,9 +487,8 @@ do_convert_private_ssh2(struct sshbuf *b)
 		fatal("%s: buffer error: %s", __func__, ssh_err(r));
 
 	if (magic != SSH_COM_PRIVATE_KEY_MAGIC) {
-		error("bad magic 0x%x != 0x%x", magic,
+		fatal("bad magic 0x%x != 0x%x", magic,
 		    SSH_COM_PRIVATE_KEY_MAGIC);
-		return NULL;
 	}
 	if ((r = sshbuf_get_u32(b, &i1)) != 0 ||
 	    (r = sshbuf_get_cstring(b, &type, NULL)) != 0 ||
@@ -499,12 +498,8 @@ do_convert_private_ssh2(struct sshbuf *b)
 	    (r = sshbuf_get_u32(b, &i4)) != 0)
 		fatal("%s: buffer error: %s", __func__, ssh_err(r));
 	debug("ignore (%d %d %d %d)", i1, i2, i3, i4);
-	if (strcmp(cipher, "none") != 0) {
-		error("unsupported cipher %s", cipher);
-		free(cipher);
-		free(type);
-		return NULL;
-	}
+	if (strcmp(cipher, "none") != 0)
+		fatal("unsupported cipher %s", cipher);
 	free(cipher);
 
 	if (strstr(type, "dsa")) {
@@ -512,8 +507,7 @@ do_convert_private_ssh2(struct sshbuf *b)
 	} else if (strstr(type, "rsa")) {
 		ktype = KEY_RSA;
 	} else {
-		free(type);
-		return NULL;
+		fatal("unsupported key type %s", type);
 	}
 	if ((key = sshkey_new(ktype)) == NULL)
 		fatal("sshkey_new failed");
@@ -556,11 +550,8 @@ do_convert_private_ssh2(struct sshbuf *b)
 		}
 		if ((rsa_e = BN_new()) == NULL)
 			fatal("%s: BN_new", __func__);
-		if (!BN_set_word(rsa_e, e)) {
-			BN_clear_free(rsa_e);
-			sshkey_free(key);
-			return NULL;
-		}
+		if (!BN_set_word(rsa_e, e))
+			fatal("%s: BN_set_word", __func__);
 		if ((rsa_n = BN_new()) == NULL ||
 		    (rsa_d = BN_new()) == NULL ||
 		    (rsa_p = BN_new()) == NULL ||
@@ -592,9 +583,7 @@ do_convert_private_ssh2(struct sshbuf *b)
 	    NULL, NULL, 0) != 0 ||
 	    sshkey_verify(key, sig, slen, data, sizeof(data),
 	    NULL, 0, NULL) != 0) {
-		sshkey_free(key);
-		free(sig);
-		return NULL;
+		fatal("%s: converted key is not valid", __func__);
 	}
 	free(sig);
 	return key;
-- 
2.26.2

On Sat, 25 Apr 2020, Michael Forney wrote:
> This matches the other do_convert_* functions, which also cannot
> fail. Otherwise, ssh-keygen will crash when it tries to check the
> key type before writing it to stdout.
> 
> For example, if I corrupt the key magic:
> 
> $ sed 's,^P2/5,AAAA,' regress/rsa_ssh2.prv > bad.prv &&
chmod 600 bad.prv
> $ ssh-keygen -i -f bad.prv
> bad magic 0xeb != 0x3f6ff9eb
> Segmentation fault
> $
I just committed a different fix - thanks

diff --git a/ssh-keygen.c b/ssh-keygen.c
index 526b57f..a046c04 100644
--- a/ssh-keygen.c
+++ b/ssh-keygen.c
@@ -653,9 +653,10 @@ do_convert_from_ssh2(struct passwd *pw, struct sshkey **k,
int *private)
 		encoded[len-3] = '\0';
 	if ((r = sshbuf_b64tod(buf, encoded)) != 0)
 		fatal("%s: base64 decoding failed: %s", __func__, ssh_err(r));
-	if (*private)
-		*k = do_convert_private_ssh2(buf);
-	else if ((r = sshkey_fromb(buf, k)) != 0)
+	if (*private) {
+		if ((*k = do_convert_private_ssh2(buf)) == NULL)
+			fatal("%s: private key conversion failed", __func__);
+	} else if ((r = sshkey_fromb(buf, k)) != 0)
 		fatal("decode blob failed: %s", ssh_err(r));
 	sshbuf_free(buf);
 	fclose(fp);