fix a possible NULL deref on loading a corrupt ECDH key

store ECDH group information in private keys files as "named groups"
rather than as a set of explicit group parameters (by setting
the OPENSSL_EC_NAMED_CURVE flag). This makes for shorter key files and
retrieves the group's OpenSSL NID that we need for various things.

4 files changed, 31 insertions, 23 deletions

diff --git a/usr.bin/ssh/authfile.c b/usr.bin/ssh/authfile.c
index 16338b78e0d..aa6b3989178 100644
--- a/usr.bin/ssh/authfile.c
+++ b/usr.bin/ssh/authfile.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: authfile.c,v 1.84 2010/09/08 03:54:36 djm Exp $ */
+/* $OpenBSD: authfile.c,v 1.85 2010/10/28 11:22:09 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -511,13 +511,9 @@ key_load_private_pem(int fd, int type, const char *passphrase,
 		prv = key_new(KEY_UNSPEC);
 		prv->ecdsa = EVP_PKEY_get1_EC_KEY(pk);
 		prv->type = KEY_ECDSA;
-		prv->ecdsa_nid = key_ecdsa_group_to_nid(
-		    EC_KEY_get0_group(prv->ecdsa));
-		if (key_curve_nid_to_name(prv->ecdsa_nid) == NULL) {
-			key_free(prv);
-			prv = NULL;
-		}
-		if (key_ec_validate_public(EC_KEY_get0_group(prv->ecdsa),
+		if ((prv->ecdsa_nid = key_ecdsa_key_to_nid(prv->ecdsa)) == -1 ||
+		    key_curve_nid_to_name(prv->ecdsa_nid) == NULL ||
+		    key_ec_validate_public(EC_KEY_get0_group(prv->ecdsa),
 		    EC_KEY_get0_public_key(prv->ecdsa)) != 0 ||
 		    key_ec_validate_private(prv->ecdsa) != 0) {
 			error("%s: bad ECDSA key", __func__);
@@ -526,7 +522,7 @@ key_load_private_pem(int fd, int type, const char *passphrase,
 		}
 		name = "ecdsa w/o comment";
 #ifdef DEBUG_PK
-		if (prv->ecdsa != NULL)
+		if (prv != NULL && prv->ecdsa != NULL)
 			key_dump_ec_key(prv->ecdsa);
 #endif
 	} else {
diff --git a/usr.bin/ssh/key.c b/usr.bin/ssh/key.c
index ba2709dfb4c..f176574f630 100644
--- a/usr.bin/ssh/key.c
+++ b/usr.bin/ssh/key.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: key.c,v 1.93 2010/09/09 10:45:45 djm Exp $ */
+/* $OpenBSD: key.c,v 1.94 2010/10/28 11:22:09 djm Exp $ */
 /*
  * read_bignum():
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -1019,12 +1019,8 @@ key_ecdsa_bits_to_nid(int bits)
 	}
 }
 
-/*
- * This is horrid, but OpenSSL's PEM_read_PrivateKey seems not to restore
- * the EC_GROUP nid when loading a key...
- */
 int
-key_ecdsa_group_to_nid(const EC_GROUP *g)
+key_ecdsa_key_to_nid(EC_KEY *k)
 {
 	EC_GROUP *eg;
 	int nids[] = {
@@ -1033,23 +1029,39 @@ key_ecdsa_group_to_nid(const EC_GROUP *g)
 		NID_secp521r1,
 		-1
 	};
+	int nid;
 	u_int i;
 	BN_CTX *bnctx;
+	const EC_GROUP *g = EC_KEY_get0_group(k);
 
+	/*
+	 * The group may be stored in a ASN.1 encoded private key in one of two
+	 * ways: as a "named group", which is reconstituted by ASN.1 object ID
+	 * or explicit group parameters encoded into the key blob. Only the
+	 * "named group" case sets the group NID for us, but we can figure
+	 * it out for the other case by comparing against all the groups that
+	 * are supported.
+	 */
+	if ((nid = EC_GROUP_get_curve_name(g)) > 0)
+		return nid;
 	if ((bnctx = BN_CTX_new()) == NULL)
 		fatal("%s: BN_CTX_new() failed", __func__);
 	for (i = 0; nids[i] != -1; i++) {
 		if ((eg = EC_GROUP_new_by_curve_name(nids[i])) == NULL)
 			fatal("%s: EC_GROUP_new_by_curve_name failed",
 			    __func__);
-		if (EC_GROUP_cmp(g, eg, bnctx) == 0) {
-			EC_GROUP_free(eg);
+		if (EC_GROUP_cmp(g, eg, bnctx) == 0)
 			break;
-		}
 		EC_GROUP_free(eg);
 	}
 	BN_CTX_free(bnctx);
 	debug3("%s: nid = %d", __func__, nids[i]);
+	if (nids[i] != -1) {
+		/* Use the group with the NID attached */
+		EC_GROUP_set_asn1_flag(eg, OPENSSL_EC_NAMED_CURVE);
+		if (EC_KEY_set_group(k, eg) != 1)
+			fatal("%s: EC_KEY_set_group", __func__);
+	}
 	return nids[i];
 }
 
@@ -1064,6 +1076,7 @@ ecdsa_generate_private_key(u_int bits, int *nid)
 		fatal("%s: EC_KEY_new_by_curve_name failed", __func__);
 	if (EC_KEY_generate_key(private) != 1)
 		fatal("%s: EC_KEY_generate_key failed", __func__);
+	EC_KEY_set_asn1_flag(private, OPENSSL_EC_NAMED_CURVE);
 	return private;
 }
 
diff --git a/usr.bin/ssh/key.h b/usr.bin/ssh/key.h
index ba1a20c0754..1b9b21ceaa8 100644
--- a/usr.bin/ssh/key.h
+++ b/usr.bin/ssh/key.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: key.h,v 1.32 2010/09/09 10:45:45 djm Exp $ */
+/* $OpenBSD: key.h,v 1.33 2010/10/28 11:22:09 djm Exp $ */
 
 /*
  * Copyright (c) 2000, 2001 Markus Friedl.  All rights reserved.
@@ -114,7 +114,7 @@ int		 key_curve_name_to_nid(const char *);
 const char *	 key_curve_nid_to_name(int);
 u_int		 key_curve_nid_to_bits(int);
 int		 key_ecdsa_bits_to_nid(int);
-int		 key_ecdsa_group_to_nid(const EC_GROUP *);
+int		 key_ecdsa_key_to_nid(EC_KEY *);
 const EVP_MD *	 key_ec_nid_to_evpmd(int nid);
 int		 key_ec_validate_public(const EC_GROUP *, const EC_POINT *);
 int		 key_ec_validate_private(const EC_KEY *);
diff --git a/usr.bin/ssh/ssh-keygen.c b/usr.bin/ssh/ssh-keygen.c
index 1d9a870fc99..711456f21be 100644
--- a/usr.bin/ssh/ssh-keygen.c
+++ b/usr.bin/ssh/ssh-keygen.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-keygen.c,v 1.203 2010/09/02 17:21:50 naddy Exp $ */
+/* $OpenBSD: ssh-keygen.c,v 1.204 2010/10/28 11:22:09 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1994 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -545,8 +545,7 @@ do_convert_from_pkcs8(Key **k, int *private)
 		*k = key_new(KEY_UNSPEC);
 		(*k)->type = KEY_ECDSA;
 		(*k)->ecdsa = EVP_PKEY_get1_EC_KEY(pubkey);
-		(*k)->ecdsa_nid = key_ecdsa_group_to_nid(
-		    EC_KEY_get0_group((*k)->ecdsa));
+		(*k)->ecdsa_nid = key_ecdsa_key_to_nid((*k)->ecdsa);
 		break;
 	default:
 		fatal("%s: unsupported pubkey type %d", __func__,