From a2d4e644ac5092ca240f1355b8d0b5c16930e048 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 11 Jul 2013 13:59:02 +0200 Subject: [PATCH] Some more EC pubkey parsing refactoring Fix a bug in pk_rsa() and pk_ec() along the way --- include/polarssl/asn1.h | 13 ++++ include/polarssl/pk.h | 4 +- library/asn1parse.c | 18 ++++++ library/x509parse.c | 90 ++++++-------------------- tests/suites/test_suite_x509parse.data | 4 +- 5 files changed, 54 insertions(+), 75 deletions(-) diff --git a/include/polarssl/asn1.h b/include/polarssl/asn1.h index ae498d0cd..195ebcb84 100644 --- a/include/polarssl/asn1.h +++ b/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 " * Updated the pointer to immediately behind the full sequence tag. diff --git a/include/polarssl/pk.h b/include/polarssl/pk.h index f5797f373..48bff9b01 100644 --- a/include/polarssl/pk.h +++ b/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 diff --git a/library/asn1parse.c b/library/asn1parse.c index 5b86aa60e..f6b79efd7 100644 --- a/library/asn1parse.c +++ b/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 " diff --git a/library/x509parse.c b/library/x509parse.c index ee9fe24b4..4eafd7edb 100644 --- a/library/x509parse.c +++ b/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 ) - { - return( POLARSSL_ERR_X509_CERT_INVALID_PUBKEY ); - } - - *p = (unsigned char *) end; - - if( ( ret = ecp_check_pubkey( &key->grp, &key->Q ) ) != 0 ) + (const unsigned char *) *p, end - *p ) ) != 0 || + ( 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 ) ) - != 0 ) - { - ecp_keypair_free( eck ); - return( ret ); - } + if( ( ret = asn1_get_bitstring_null( &p, end2, &len ) ) != 0 ) + return( POLARSSL_ERR_X509_KEY_INVALID_FORMAT + ret ); - if( p != end2 ) - return( POLARSSL_ERR_X509_CERT_INVALID_PUBKEY + + if( p + len != end2 ) + 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 ) { diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index bbfbed40b..77f31d42b 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/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