Some more EC pubkey parsing refactoring

Fix a bug in pk_rsa() and pk_ec() along the way

include/polarssl/asn1.h

@@ -212,6 +212,19 @@ int asn1_get_int( unsigned char **p,
 int asn1_get_bitstring( unsigned char **p, const unsigned char *end,
                         asn1_bitstring *bs);
 
+/**
+ * Retrieve a bitstring ASN.1 tag without unused bits and its value.
+ * Updates the pointer to the beginning of the bit/octet string.
+ *
+ * \param p     The position in the ASN.1 data
+ * \param end   End of data
+ * \param len   Length of the actual bit/octect string in bytes
+ *
+ * \return      0 if successful or a specific ASN.1 error code.
+ */
+int asn1_get_bitstring_null( unsigned char **p, const unsigned char *end,
+                             size_t *len );
+
 /**
  * Parses and splits an ASN.1 "SEQUENCE OF <tag>"
  * Updated the pointer to immediately behind the full sequence tag.

include/polarssl/pk.h

@@ -43,7 +43,7 @@
  * \warning You must make sure the PK context actually holds an RSA context
  * before using this macro!
  */
-#define pk_rsa( pk )        ( (rsa_context *) pk.data )
+#define pk_rsa( pk )        ( (rsa_context *) (pk).data )
 #endif
 
 #if defined(POLARSSL_ECP_C)
@@ -53,7 +53,7 @@
  * \warning You must make sure the PK context actually holds an EC context
  * before using this macro!
  */
-#define pk_ec( pk )         ( (ecp_keypair *) pk.data )
+#define pk_ec( pk )         ( (ecp_keypair *) (pk).data )
 #endif
 
 

library/asn1parse.c

@@ -209,6 +209,24 @@ int asn1_get_bitstring( unsigned char **p, const unsigned char *end,
     return 0;
 }
 
+/*
+ * Get a bit string without unused bits
+ */
+int asn1_get_bitstring_null( unsigned char **p, const unsigned char *end,
+                             size_t *len )
+{
+    int ret;
+
+    if( ( ret = asn1_get_tag( p, end, len, ASN1_BIT_STRING ) ) != 0 )
+        return( ret );
+
+    if( --*len < 1 || *(*p)++ != 0 )
+        return( POLARSSL_ERR_ASN1_INVALID_DATA );
+
+    return( 0 );
+}
+
+
 
 /*
  *  Parses and splits an ASN.1 "SEQUENCE OF <tag>"

library/x509parse.c

@@ -262,43 +262,6 @@ static int x509_use_ecparams( const x509_buf *params, ecp_group *grp )
     return( 0 );
 }
 
-/*
- * subjectPublicKey  BIT STRING
- * -- which, in our case, contains
- * ECPoint ::= octet string (not ASN.1)
- */
-static int x509_get_subpubkey_ec( unsigned char **p, const unsigned char *end,
-                                  const ecp_group *grp, ecp_point *pt )
-{
-    int ret;
-    size_t len;
-
-    if( ( ret = asn1_get_tag( p, end, &len, ASN1_BIT_STRING ) ) != 0 )
-        return( POLARSSL_ERR_X509_KEY_INVALID_FORMAT + ret );
-
-    if( *p + len != end )
-        return( POLARSSL_ERR_X509_KEY_INVALID_FORMAT +
-                POLARSSL_ERR_ASN1_LENGTH_MISMATCH );
-
-    if( --len < 1 || *(*p)++ != 0 )
-        return( POLARSSL_ERR_X509_KEY_INVALID_FORMAT + ret );
-
-    if( ( ret = ecp_point_read_binary( grp, pt,
-                    (const unsigned char *) *p, len ) ) != 0 )
-    {
-        return( POLARSSL_ERR_X509_CERT_INVALID_PUBKEY );
-    }
-
-    *p += len;
-
-    if( ( ret = ecp_check_pubkey( grp, pt ) ) != 0 )
-    {
-        return( POLARSSL_ERR_X509_CERT_INVALID_PUBKEY );
-    }
-
-    return( 0 );
-}
-
 /*
  *  AttributeTypeAndValue ::= SEQUENCE {
  *    type     AttributeType,
@@ -556,27 +519,23 @@ static int x509_get_rsapubkey( unsigned char **p,
 }
 
 static int x509_get_ecpubkey( unsigned char **p, const unsigned char *end,
-                              x509_buf *alg_params, ecp_keypair *key )
+                              ecp_keypair *key )
 {
     int ret;
 
-    if( ( ret = x509_use_ecparams( alg_params, &key->grp ) ) != 0 )
-        return( ret );
-
     if( ( ret = ecp_point_read_binary( &key->grp, &key->Q,
-                    (const unsigned char *) *p, end - *p ) ) != 0 )
+                    (const unsigned char *) *p, end - *p ) ) != 0 ||
-    {
+        ( ret = ecp_check_pubkey( &key->grp, &key->Q ) ) != 0 )
-        return( POLARSSL_ERR_X509_CERT_INVALID_PUBKEY );
-    }
-
-    *p = (unsigned char *) end;
-
-    if( ( ret = ecp_check_pubkey( &key->grp, &key->Q ) ) != 0 )
     {
         ecp_keypair_free( key );
         return( POLARSSL_ERR_X509_CERT_INVALID_PUBKEY );
     }
 
+    /*
+     * We know ecp_point_read_binary consumed all bytes
+     */
+    *p = (unsigned char *) end;
+
     return( 0 );
 }
 
@@ -605,20 +564,13 @@ static int x509_get_pubkey( unsigned char **p,
     if( ( ret = x509_get_pk_alg( p, end, &pk_alg, &alg_params ) ) != 0 )
         return( ret );
 
-    if( ( ret = asn1_get_tag( p, end, &len, ASN1_BIT_STRING ) ) != 0 )
+    if( ( ret = asn1_get_bitstring_null( p, end, &len ) ) != 0 )
         return( POLARSSL_ERR_X509_CERT_INVALID_PUBKEY + ret );
 
-    if( ( end - *p ) < 1 )
-        return( POLARSSL_ERR_X509_CERT_INVALID_PUBKEY +
-                POLARSSL_ERR_ASN1_OUT_OF_DATA );
-
     if( *p + len != end )
         return( POLARSSL_ERR_X509_CERT_INVALID_PUBKEY +
                 POLARSSL_ERR_ASN1_LENGTH_MISMATCH );
 
-    if( --len < 1 || *(*p)++ != 0 )
-        return( POLARSSL_ERR_X509_CERT_INVALID_PUBKEY );
-
     if( ( ret = pk_set_type( pk, pk_alg ) ) != 0 )
         return( ret );
 
@@ -639,7 +591,8 @@ static int x509_get_pubkey( unsigned char **p,
             /* FALLTHROUGH */
 
         case POLARSSL_PK_ECKEY:
-            ret = x509_get_ecpubkey( p, end, &alg_params, pk->data );
+            ret = x509_use_ecparams( &alg_params, &pk_ec( *pk )->grp ) ||
+                  x509_get_ecpubkey( p, end, pk->data );
             break;
     }
 
@@ -666,13 +619,9 @@ static int x509_get_sig( unsigned char **p,
 
     sig->tag = **p;
 
-    if( ( ret = asn1_get_tag( p, end, &len, ASN1_BIT_STRING ) ) != 0 )
+    if( ( ret = asn1_get_bitstring_null( p, end, &len ) ) != 0 )
         return( POLARSSL_ERR_X509_CERT_INVALID_SIGNATURE + ret );
 
-
-    if( --len < 1 || *(*p)++ != 0 )
-        return( POLARSSL_ERR_X509_CERT_INVALID_SIGNATURE );
-
     sig->len = len;
     sig->p = *p;
 
@@ -2696,16 +2645,15 @@ static int x509parse_key_sec1_der( ecp_keypair *eck,
     {
         end2 = p + len;
 
-        if( ( ret = x509_get_subpubkey_ec( &p, end2, &eck->grp, &eck->Q ) )
+        if( ( ret = asn1_get_bitstring_null( &p, end2, &len ) ) != 0 )
-                != 0 )
+            return( POLARSSL_ERR_X509_KEY_INVALID_FORMAT + ret );
-        {
-            ecp_keypair_free( eck );
-            return( ret );
-        }
 
-        if( p != end2 )
+        if( p + len != end2 )
-            return( POLARSSL_ERR_X509_CERT_INVALID_PUBKEY +
+            return( POLARSSL_ERR_X509_KEY_INVALID_FORMAT +
                     POLARSSL_ERR_ASN1_LENGTH_MISMATCH );
+
+        if( ( ret = x509_get_ecpubkey( &p, end2, eck ) ) != 0 )
+            return( ret );
     }
     else if ( ret != POLARSSL_ERR_ASN1_UNEXPECTED_TAG )
     {

tests/suites/test_suite_x509parse.data

@@ -523,7 +523,7 @@ X509 Certificate ASN1 (TBSCertificate, pubkey, no bitstring data)
 x509parse_crt:"30693067a0030201028204deadbeef300d06092a864886f70d0101020500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a300806001304546573743011300d06092A864886F70D01010105000300":"":POLARSSL_ERR_X509_CERT_INVALID_PUBKEY + POLARSSL_ERR_ASN1_OUT_OF_DATA
 
 X509 Certificate ASN1 (TBSCertificate, pubkey, invalid bitstring start)
-x509parse_crt:"306a3068a0030201028204deadbeef300d06092a864886f70d0101020500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a300806001304546573743012300d06092A864886F70D0101010500030101":"":POLARSSL_ERR_X509_CERT_INVALID_PUBKEY
+x509parse_crt:"306a3068a0030201028204deadbeef300d06092a864886f70d0101020500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a300806001304546573743012300d06092A864886F70D0101010500030101":"":POLARSSL_ERR_X509_CERT_INVALID_PUBKEY + POLARSSL_ERR_ASN1_INVALID_DATA
 
 X509 Certificate ASN1 (TBSCertificate, pubkey, invalid internal bitstring length)
 x509parse_crt:"306d306ba0030201028204deadbeef300d06092a864886f70d0101020500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a300806001304546573743015300d06092A864886F70D0101010500030400300000":"":POLARSSL_ERR_X509_CERT_INVALID_PUBKEY + POLARSSL_ERR_ASN1_LENGTH_MISMATCH
@@ -595,7 +595,7 @@ X509 Certificate ASN1 (sig_alg, no sig)
 x509parse_crt:"308192308180a0030201008204deadbeef300d06092a864886f70d0101020500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092A864886F70D010101050003190030160210ffffffffffffffffffffffffffffffff0202ffff300d06092a864886f70d0101020500":"":POLARSSL_ERR_X509_CERT_INVALID_SIGNATURE + POLARSSL_ERR_ASN1_OUT_OF_DATA
 
 X509 Certificate ASN1 (signature, invalid sig data)
-x509parse_crt:"308195308180a0030201008204deadbeef300d06092a864886f70d0101020500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092A864886F70D010101050003190030160210ffffffffffffffffffffffffffffffff0202ffff300d06092a864886f70d0101020500030100":"":POLARSSL_ERR_X509_CERT_INVALID_SIGNATURE
+x509parse_crt:"308195308180a0030201008204deadbeef300d06092a864886f70d0101020500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092A864886F70D010101050003190030160210ffffffffffffffffffffffffffffffff0202ffff300d06092a864886f70d0101020500030100":"":POLARSSL_ERR_X509_CERT_INVALID_SIGNATURE + POLARSSL_ERR_ASN1_INVALID_DATA
 
 X509 Certificate ASN1 (signature, data left)
 x509parse_crt:"308197308180a0030201008204deadbeef300d06092a864886f70d0101020500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092A864886F70D010101050003190030160210ffffffffffffffffffffffffffffffff0202ffff300d06092a864886f70d0101020500030200ff00":"":POLARSSL_ERR_X509_CERT_INVALID_FORMAT + POLARSSL_ERR_ASN1_LENGTH_MISMATCH