From 924cd100a6dd6d5738e7816457efb3e40579f19d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= <mpg@elzevir.fr>
Date: Tue, 14 Apr 2015 11:18:04 +0200
Subject: [PATCH] Fix bug in pk_parse_key()

---
 ChangeLog                            |   2 +
 library/pkparse.c                    |  97 ++++++++++++++-------------
 tests/data_files/ec_prv.noopt.der    | Bin 0 -> 67 bytes
 tests/suites/test_suite_pkparse.data |   4 ++
 4 files changed, 56 insertions(+), 47 deletions(-)
 create mode 100644 tests/data_files/ec_prv.noopt.der

diff --git a/ChangeLog b/ChangeLog
index 3697f5310..671df6477 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -25,6 +25,8 @@ Features
      errors on use of deprecated functions.
 
 Bugfix
+   * Fix bug in pk_parse_key() that caused some valid private EC keys to be
+     rejected.
    * Fix bug in Via Padlock support (found by Nikos Mavrogiannopoulos).
    * Fix thread safety bug in RSA operations (found by Fredrik Axelsson).
    * Fix hardclock() (only used in the benchmarking program) with some
diff --git a/library/pkparse.c b/library/pkparse.c
index 06fb2929f..39c51f648 100644
--- a/library/pkparse.c
+++ b/library/pkparse.c
@@ -761,58 +761,61 @@ static int pk_parse_key_sec1_der( ecp_keypair *eck,
 
     p += len;
 
-    /*
-     * Is 'parameters' present?
-     */
-    if( ( ret = asn1_get_tag( &p, end, &len,
-                    ASN1_CONTEXT_SPECIFIC | ASN1_CONSTRUCTED | 0 ) ) == 0 )
+    pubkey_done = 0;
+    if( p != end )
     {
-        if( ( ret = pk_get_ecparams( &p, p + len, &params) ) != 0 ||
-            ( ret = pk_use_ecparams( &params, &eck->grp )  ) != 0 )
+        /*
+         * Is 'parameters' present?
+         */
+        if( ( ret = asn1_get_tag( &p, end, &len,
+                        ASN1_CONTEXT_SPECIFIC | ASN1_CONSTRUCTED | 0 ) ) == 0 )
+        {
+            if( ( ret = pk_get_ecparams( &p, p + len, &params) ) != 0 ||
+                ( ret = pk_use_ecparams( &params, &eck->grp )  ) != 0 )
+            {
+                ecp_keypair_free( eck );
+                return( ret );
+            }
+        }
+        else if( ret != POLARSSL_ERR_ASN1_UNEXPECTED_TAG )
+        {
+            ecp_keypair_free( eck );
+            return( POLARSSL_ERR_PK_KEY_INVALID_FORMAT + ret );
+        }
+
+        /*
+         * Is 'publickey' present? If not, or if we can't read it (eg because it
+         * is compressed), create it from the private key.
+         */
+        if( ( ret = asn1_get_tag( &p, end, &len,
+                        ASN1_CONTEXT_SPECIFIC | ASN1_CONSTRUCTED | 1 ) ) == 0 )
+        {
+            end2 = p + len;
+
+            if( ( ret = asn1_get_bitstring_null( &p, end2, &len ) ) != 0 )
+                return( POLARSSL_ERR_PK_KEY_INVALID_FORMAT + ret );
+
+            if( p + len != end2 )
+                return( POLARSSL_ERR_PK_KEY_INVALID_FORMAT +
+                        POLARSSL_ERR_ASN1_LENGTH_MISMATCH );
+
+            if( ( ret = pk_get_ecpubkey( &p, end2, eck ) ) == 0 )
+                pubkey_done = 1;
+            else
+            {
+                /*
+                 * The only acceptable failure mode of pk_get_ecpubkey() above
+                 * is if the point format is not recognized.
+                 */
+                if( ret != POLARSSL_ERR_ECP_FEATURE_UNAVAILABLE )
+                    return( POLARSSL_ERR_PK_KEY_INVALID_FORMAT );
+            }
+        }
+        else if( ret != POLARSSL_ERR_ASN1_UNEXPECTED_TAG )
         {
             ecp_keypair_free( eck );
-            return( ret );
-        }
-    }
-    else if( ret != POLARSSL_ERR_ASN1_UNEXPECTED_TAG )
-    {
-        ecp_keypair_free( eck );
-        return( POLARSSL_ERR_PK_KEY_INVALID_FORMAT + ret );
-    }
-
-    /*
-     * Is 'publickey' present? If not, or if we can't read it (eg because it
-     * is compressed), create it from the private key.
-     */
-    pubkey_done = 0;
-    if( ( ret = asn1_get_tag( &p, end, &len,
-                    ASN1_CONTEXT_SPECIFIC | ASN1_CONSTRUCTED | 1 ) ) == 0 )
-    {
-        end2 = p + len;
-
-        if( ( ret = asn1_get_bitstring_null( &p, end2, &len ) ) != 0 )
             return( POLARSSL_ERR_PK_KEY_INVALID_FORMAT + ret );
-
-        if( p + len != end2 )
-            return( POLARSSL_ERR_PK_KEY_INVALID_FORMAT +
-                    POLARSSL_ERR_ASN1_LENGTH_MISMATCH );
-
-        if( ( ret = pk_get_ecpubkey( &p, end2, eck ) ) == 0 )
-            pubkey_done = 1;
-        else
-        {
-            /*
-             * The only acceptable failure mode of pk_get_ecpubkey() above
-             * is if the point format is not recognized.
-             */
-            if( ret != POLARSSL_ERR_ECP_FEATURE_UNAVAILABLE )
-                return( POLARSSL_ERR_PK_KEY_INVALID_FORMAT );
         }
-    }
-    else if( ret != POLARSSL_ERR_ASN1_UNEXPECTED_TAG )
-    {
-        ecp_keypair_free( eck );
-        return( POLARSSL_ERR_PK_KEY_INVALID_FORMAT + ret );
     }
 
     if( ! pubkey_done &&
diff --git a/tests/data_files/ec_prv.noopt.der b/tests/data_files/ec_prv.noopt.der
new file mode 100644
index 0000000000000000000000000000000000000000..fde16a17a7551d655f8cb0e66febe4809a664845
GIT binary patch
literal 67
zcmXqTWMX765N2c7YV$Z}%f!gW0cJ2Wva_fgs4_7!vM9Vh?a{s>m+gy*#G)C0*3J3b
V_^ST<Z}ltjclJ)u+Q{o~4ggjR7r6ic

literal 0
HcmV?d00001

diff --git a/tests/suites/test_suite_pkparse.data b/tests/suites/test_suite_pkparse.data
index b502017b0..aab568d18 100644
--- a/tests/suites/test_suite_pkparse.data
+++ b/tests/suites/test_suite_pkparse.data
@@ -146,6 +146,10 @@ Parse EC Key #1 (SEC1 DER)
 depends_on:POLARSSL_PEM_PARSE_C:POLARSSL_ECP_C:POLARSSL_ECP_DP_SECP192R1_ENABLED
 pk_parse_keyfile_ec:"data_files/ec_prv.sec1.der":"NULL":0
 
+Parse EC Key #1a (SEC1 DER, no optional part)
+depends_on:POLARSSL_PEM_PARSE_C:POLARSSL_ECP_C:POLARSSL_ECP_DP_SECP256R1_ENABLED
+pk_parse_keyfile_ec:"data_files/ec_prv.noopt.der":"NULL":0
+
 Parse EC Key #2 (SEC1 PEM)
 depends_on:POLARSSL_PEM_PARSE_C:POLARSSL_ECP_C:POLARSSL_ECP_DP_SECP192R1_ENABLED
 pk_parse_keyfile_ec:"data_files/ec_prv.sec1.pem":"NULL":0