- djm@cvs.openbsd.org 2010/10/28 11:22:09

     [authfile.c key.c key.h ssh-keygen.c]
     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.

diff --git a/ChangeLog b/ChangeLog
index 2e7f92c9477563a45926f3635e0b980fcb3e5eb3..79419367e400a6d1370dc9be301006c3f8603175 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -3,6 +3,14 @@
    - djm@cvs.openbsd.org 2010/09/22 12:26:05
      [regress/Makefile regress/kextype.sh]
      regress test for each of the key exchange algorithms that we support
+   - djm@cvs.openbsd.org 2010/10/28 11:22:09
+     [authfile.c key.c key.h ssh-keygen.c]
+     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.
 
 20101025
  - (tim) [openbsd-compat/glob.h] Remove sys/cdefs.h include that came with

diff --git a/authfile.c b/authfile.c
index b1e3eda5c4f72fdce8eb2ced4fe2771c8589aabc..7f98ab5474c2cd343b0bec2cd34979f3be813527 100644 (file)
--- a/authfile.c
+++ b/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
@@ -523,13 +523,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__);
@@ -538,7 +534,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
 #endif /* OPENSSL_HAS_ECC */

diff --git a/key.c b/key.c
index 196092de5c6b19330336f3f569f092b6ad5ac71f..c71bf5b0a8e89b485e61f2e9b953053fbd0cb899 100644 (file)
--- a/key.c
+++ b/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
@@ -1053,12 +1053,8 @@ key_ecdsa_bits_to_nid(int bits)
 }
 
 #ifdef OPENSSL_HAS_ECC
-/*
- * 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[] = {
@@ -1067,23 +1063,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];
 }
 
@@ -1098,6 +1110,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;
 }
 #endif /* OPENSSL_HAS_ECC */

diff --git a/key.h b/key.h
index 86a1d889c4373165a841a560e653f157317f8cf4..ec5ac5eb87c14cc70b04899e59693a2b94861570 100644 (file)
--- a/key.h
+++ b/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.
@@ -121,7 +121,7 @@ const char *         key_curve_nid_to_name(int);
 u_int           key_curve_nid_to_bits(int);
 int             key_ecdsa_bits_to_nid(int);
 #ifdef OPENSSL_HAS_ECC
-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/ssh-keygen.c b/ssh-keygen.c
index bbd434b0b2224ca26b7ffa74eda2a64a771a09ee..560c4818a44baf831a7a78a9dc8934b53cc10728 100644 (file)
--- a/ssh-keygen.c
+++ b/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
@@ -556,8 +556,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;
 #endif
        default: