diff --git a/ChangeLog b/ChangeLog index 83eb55426..eb0fe907a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,15 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS x.x.x branch released xxxx-xx-xx +Security + * Fix overly strict DN comparison when looking for CRLs + belonging to a particular CA. This previously lead to + ignoring CRLs when the CRL's issuer name and the CA's + subject name differed in their string encoding (e.g., + one using PrintableString and the other UTF8String) or + in the choice of upper and lower case. Reported by + HenrikRosenquistAndersson in #1784. + Features * Add support for temporarily suspending expensive ECC computations after some configurable amount of operations, to be used in single-threaded diff --git a/library/x509_crt.c b/library/x509_crt.c index 25aaff3b0..97a4bbf24 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -227,6 +227,136 @@ static int x509_profile_check_key( const mbedtls_x509_crt_profile *profile, return( -1 ); } +/* + * Like memcmp, but case-insensitive and always returns -1 if different + */ +static int x509_memcasecmp( const void *s1, const void *s2, size_t len ) +{ + size_t i; + unsigned char diff; + const unsigned char *n1 = s1, *n2 = s2; + + for( i = 0; i < len; i++ ) + { + diff = n1[i] ^ n2[i]; + + if( diff == 0 ) + continue; + + if( diff == 32 && + ( ( n1[i] >= 'a' && n1[i] <= 'z' ) || + ( n1[i] >= 'A' && n1[i] <= 'Z' ) ) ) + { + continue; + } + + return( -1 ); + } + + return( 0 ); +} + +/* + * Return 0 if name matches wildcard, -1 otherwise + */ +static int x509_check_wildcard( const char *cn, const mbedtls_x509_buf *name ) +{ + size_t i; + size_t cn_idx = 0, cn_len = strlen( cn ); + + /* We can't have a match if there is no wildcard to match */ + if( name->len < 3 || name->p[0] != '*' || name->p[1] != '.' ) + return( -1 ); + + for( i = 0; i < cn_len; ++i ) + { + if( cn[i] == '.' ) + { + cn_idx = i; + break; + } + } + + if( cn_idx == 0 ) + return( -1 ); + + if( cn_len - cn_idx == name->len - 1 && + x509_memcasecmp( name->p + 1, cn + cn_idx, name->len - 1 ) == 0 ) + { + return( 0 ); + } + + return( -1 ); +} + +/* + * Compare two X.509 strings, case-insensitive, and allowing for some encoding + * variations (but not all). + * + * Return 0 if equal, -1 otherwise. + */ +static int x509_string_cmp( const mbedtls_x509_buf *a, const mbedtls_x509_buf *b ) +{ + if( a->tag == b->tag && + a->len == b->len && + memcmp( a->p, b->p, b->len ) == 0 ) + { + return( 0 ); + } + + if( ( a->tag == MBEDTLS_ASN1_UTF8_STRING || a->tag == MBEDTLS_ASN1_PRINTABLE_STRING ) && + ( b->tag == MBEDTLS_ASN1_UTF8_STRING || b->tag == MBEDTLS_ASN1_PRINTABLE_STRING ) && + a->len == b->len && + x509_memcasecmp( a->p, b->p, b->len ) == 0 ) + { + return( 0 ); + } + + return( -1 ); +} + +/* + * Compare two X.509 Names (aka rdnSequence). + * + * See RFC 5280 section 7.1, though we don't implement the whole algorithm: + * we sometimes return unequal when the full algorithm would return equal, + * but never the other way. (In particular, we don't do Unicode normalisation + * or space folding.) + * + * Return 0 if equal, -1 otherwise. + */ +static int x509_name_cmp( const mbedtls_x509_name *a, const mbedtls_x509_name *b ) +{ + /* Avoid recursion, it might not be optimised by the compiler */ + while( a != NULL || b != NULL ) + { + if( a == NULL || b == NULL ) + return( -1 ); + + /* type */ + if( a->oid.tag != b->oid.tag || + a->oid.len != b->oid.len || + memcmp( a->oid.p, b->oid.p, b->oid.len ) != 0 ) + { + return( -1 ); + } + + /* value */ + if( x509_string_cmp( &a->val, &b->val ) != 0 ) + return( -1 ); + + /* structure of the list of sets */ + if( a->next_merged != b->next_merged ) + return( -1 ); + + a = a->next; + b = b->next; + } + + /* a == NULL == b */ + return( 0 ); +} + /* * Reset (init or clear) a verify_chain */ @@ -1684,9 +1814,7 @@ static int x509_crt_verifycrl( mbedtls_x509_crt *crt, mbedtls_x509_crt *ca, while( crl_list != NULL ) { if( crl_list->version == 0 || - crl_list->issuer_raw.len != ca->subject_raw.len || - memcmp( crl_list->issuer_raw.p, ca->subject_raw.p, - crl_list->issuer_raw.len ) != 0 ) + x509_name_cmp( &crl_list->issuer, &ca->subject ) != 0 ) { crl_list = crl_list->next; continue; @@ -1696,7 +1824,8 @@ static int x509_crt_verifycrl( mbedtls_x509_crt *crt, mbedtls_x509_crt *ca, * Check if the CA is configured to sign CRLs */ #if defined(MBEDTLS_X509_CHECK_KEY_USAGE) - if( mbedtls_x509_crt_check_key_usage( ca, MBEDTLS_X509_KU_CRL_SIGN ) != 0 ) + if( mbedtls_x509_crt_check_key_usage( ca, + MBEDTLS_X509_KU_CRL_SIGN ) != 0 ) { flags |= MBEDTLS_X509_BADCRL_NOT_TRUSTED; break; @@ -1756,136 +1885,6 @@ static int x509_crt_verifycrl( mbedtls_x509_crt *crt, mbedtls_x509_crt *ca, } #endif /* MBEDTLS_X509_CRL_PARSE_C */ -/* - * Like memcmp, but case-insensitive and always returns -1 if different - */ -static int x509_memcasecmp( const void *s1, const void *s2, size_t len ) -{ - size_t i; - unsigned char diff; - const unsigned char *n1 = s1, *n2 = s2; - - for( i = 0; i < len; i++ ) - { - diff = n1[i] ^ n2[i]; - - if( diff == 0 ) - continue; - - if( diff == 32 && - ( ( n1[i] >= 'a' && n1[i] <= 'z' ) || - ( n1[i] >= 'A' && n1[i] <= 'Z' ) ) ) - { - continue; - } - - return( -1 ); - } - - return( 0 ); -} - -/* - * Return 0 if name matches wildcard, -1 otherwise - */ -static int x509_check_wildcard( const char *cn, const mbedtls_x509_buf *name ) -{ - size_t i; - size_t cn_idx = 0, cn_len = strlen( cn ); - - /* We can't have a match if there is no wildcard to match */ - if( name->len < 3 || name->p[0] != '*' || name->p[1] != '.' ) - return( -1 ); - - for( i = 0; i < cn_len; ++i ) - { - if( cn[i] == '.' ) - { - cn_idx = i; - break; - } - } - - if( cn_idx == 0 ) - return( -1 ); - - if( cn_len - cn_idx == name->len - 1 && - x509_memcasecmp( name->p + 1, cn + cn_idx, name->len - 1 ) == 0 ) - { - return( 0 ); - } - - return( -1 ); -} - -/* - * Compare two X.509 strings, case-insensitive, and allowing for some encoding - * variations (but not all). - * - * Return 0 if equal, -1 otherwise. - */ -static int x509_string_cmp( const mbedtls_x509_buf *a, const mbedtls_x509_buf *b ) -{ - if( a->tag == b->tag && - a->len == b->len && - memcmp( a->p, b->p, b->len ) == 0 ) - { - return( 0 ); - } - - if( ( a->tag == MBEDTLS_ASN1_UTF8_STRING || a->tag == MBEDTLS_ASN1_PRINTABLE_STRING ) && - ( b->tag == MBEDTLS_ASN1_UTF8_STRING || b->tag == MBEDTLS_ASN1_PRINTABLE_STRING ) && - a->len == b->len && - x509_memcasecmp( a->p, b->p, b->len ) == 0 ) - { - return( 0 ); - } - - return( -1 ); -} - -/* - * Compare two X.509 Names (aka rdnSequence). - * - * See RFC 5280 section 7.1, though we don't implement the whole algorithm: - * we sometimes return unequal when the full algorithm would return equal, - * but never the other way. (In particular, we don't do Unicode normalisation - * or space folding.) - * - * Return 0 if equal, -1 otherwise. - */ -static int x509_name_cmp( const mbedtls_x509_name *a, const mbedtls_x509_name *b ) -{ - /* Avoid recursion, it might not be optimised by the compiler */ - while( a != NULL || b != NULL ) - { - if( a == NULL || b == NULL ) - return( -1 ); - - /* type */ - if( a->oid.tag != b->oid.tag || - a->oid.len != b->oid.len || - memcmp( a->oid.p, b->oid.p, b->oid.len ) != 0 ) - { - return( -1 ); - } - - /* value */ - if( x509_string_cmp( &a->val, &b->val ) != 0 ) - return( -1 ); - - /* structure of the list of sets */ - if( a->next_merged != b->next_merged ) - return( -1 ); - - a = a->next; - b = b->next; - } - - /* a == NULL == b */ - return( 0 ); -} - /* * Check the signature of a certificate by its parent */ diff --git a/tests/data_files/test-ca_printable.crt b/tests/data_files/test-ca_printable.crt new file mode 100644 index 000000000..d16da09a2 --- /dev/null +++ b/tests/data_files/test-ca_printable.crt @@ -0,0 +1,21 @@ +-----BEGIN CERTIFICATE----- +MIIDhzCCAm+gAwIBAgIBADANBgkqhkiG9w0BAQUFADA7MQswCQYDVQQGEwJOTDER +MA8GA1UEChMIUG9sYXJTU0wxGTAXBgNVBAMTEFBvbGFyU1NMIFRlc3QgQ0EwHhcN +MTEwMjEyMTQ0NDAwWhcNMjEwMjEyMTQ0NDAwWjA7MQswCQYDVQQGEwJOTDERMA8G +A1UEChMIUG9sYXJTU0wxGTAXBgNVBAMTEFBvbGFyU1NMIFRlc3QgQ0EwggEiMA0G +CSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDA3zf8F7vglp0/ht6WMn1EpRagzSHx +mdTs6st8GFgIlKXsm8WL3xoemTiZhx57wI053zhdcHgH057Zk+i5clHFzqMwUqny +50BwFMtEonILwuVA+T7lpg6z+exKY8C4KQB0nFc7qKUEkHHxvYPZP9al4jwqj+8n +YMPGn8u67GB9t+aEMr5P+1gmIgNb1LTV+/Xjli5wwOQuvfwu7uJBVcA0Ln0kcmnL +R7EUQIN9Z/SG9jGr8XmksrUuEvmEF/Bibyc+E1ixVA0hmnM3oTDPb5Lc9un8rNsu +KNF+AksjoBXyOGVkCeoMbo4bF6BxyLObyavpw/LPh5aPgAIynplYb6LVAgMBAAGj +gZUwgZIwDAYDVR0TBAUwAwEB/zAdBgNVHQ4EFgQUtFrkpbPe0lL2udWmlQ/rPrzH +/f8wYwYDVR0jBFwwWoAUtFrkpbPe0lL2udWmlQ/rPrzH/f+hP6Q9MDsxCzAJBgNV +BAYTAk5MMREwDwYDVQQKEwhQb2xhclNTTDEZMBcGA1UEAxMQUG9sYXJTU0wgVGVz +dCBDQYIBADANBgkqhkiG9w0BAQUFAAOCAQEAuP1U2ABUkIslsCfdlc2i94QHHYeJ +SsR4EdgHtdciUI5I62J6Mom+Y0dT/7a+8S6MVMCZP6C5NyNyXw1GWY/YR82XTJ8H +DBJiCTok5DbZ6SzaONBzdWHXwWwmi5vg1dxn7YxrM9d0IjxM27WNKs4sDQhZBQkF +pjmfs2cb4oPl4Y9T9meTx/lvdkRYEug61Jfn6cA+qHpyPYdTH+UshITnmp5/Ztkf +m/UTSLBNFNHesiTZeH31NcxYGdHSme9Nc/gfidRa0FLOCfWxRlFqAI47zG9jAQCZ +7Z2mCGDNMhjQc+BYcdnl0lPXjdDK6V0qCg1dVewhUBcW5gZKzV7e9+DpVA== +-----END CERTIFICATE----- diff --git a/tests/data_files/test-ca_uppercase.crt b/tests/data_files/test-ca_uppercase.crt new file mode 100644 index 000000000..e8b537c72 --- /dev/null +++ b/tests/data_files/test-ca_uppercase.crt @@ -0,0 +1,20 @@ +-----BEGIN CERTIFICATE----- +MIIDQTCCAimgAwIBAgIBAzANBgkqhkiG9w0BAQUFADA7MQswCQYDVQQGEwJOTDER +MA8GA1UECgwIUG9sYXJTU0wxGTAXBgNVBAMMEFBvbGFyU1NMIFRlc3QgQ0EwHhcN +MTEwMjEyMTQ0NDAwWhcNMjEwMjEyMTQ0NDAwWjA7MQswCQYDVQQGEwJOTDERMA8G +A1UECgwIUG9sYXJTU0wxGTAXBgNVBAMMEFBvbGFyU1NMIFRlc3QgQ0EwggEiMA0G +CSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDA3zf8F7vglp0/ht6WMn1EpRagzSHx +mdTs6st8GFgIlKXsm8WL3xoemTiZhx57wI053zhdcHgH057Zk+i5clHFzqMwUqny +50BwFMtEonILwuVA+T7lpg6z+exKY8C4KQB0nFc7qKUEkHHxvYPZP9al4jwqj+8n +YMPGn8u67GB9t+aEMr5P+1gmIgNb1LTV+/Xjli5wwOQuvfwu7uJBVcA0Ln0kcmnL +R7EUQIN9Z/SG9jGr8XmksrUuEvmEF/Bibyc+E1ixVA0hmnM3oTDPb5Lc9un8rNsu +KNF+AksjoBXyOGVkCeoMbo4bF6BxyLObyavpw/LPh5aPgAIynplYb6LVAgMBAAGj +UDBOMAwGA1UdEwQFMAMBAf8wHQYDVR0OBBYEFLRa5KWz3tJS9rnVppUP6z68x/3/ +MB8GA1UdIwQYMBaAFLRa5KWz3tJS9rnVppUP6z68x/3/MA0GCSqGSIb3DQEBBQUA +A4IBAQABE3OEPfEd/bcJW5ZdU3/VgPNS4tMzh8gnJP/V2FcvFtGylMpQq6YnEBYI +yBHAL4DRvlMY5rnXGBp3ODR8MpqHC6AquRTCLzjS57iYff//4QFQqW9n92zctspv +czkaPKgjqo1No3Uq0Xaz10rcxyTUPrf5wNVRZ2V0KvllvAAVSzbI4mpdUXztjhST +S5A2BeWQAAOr0zq1F7TSRVJpJs7jmB2ai/igkh1IAjcuwV6VwlP+sbw0gjQ0NpGM +iHpnlzRAi/tIbtOvMIGOBU2TIfax/5jq1agUx5aPmT5TWAiJPOOP6l5xXnDwxeYS +NWqiX9GyusBZjezaCaHabjDLU0qQ +-----END CERTIFICATE----- diff --git a/tests/data_files/test-ca_utf8.crt b/tests/data_files/test-ca_utf8.crt new file mode 100644 index 000000000..e8b537c72 --- /dev/null +++ b/tests/data_files/test-ca_utf8.crt @@ -0,0 +1,20 @@ +-----BEGIN CERTIFICATE----- +MIIDQTCCAimgAwIBAgIBAzANBgkqhkiG9w0BAQUFADA7MQswCQYDVQQGEwJOTDER +MA8GA1UECgwIUG9sYXJTU0wxGTAXBgNVBAMMEFBvbGFyU1NMIFRlc3QgQ0EwHhcN +MTEwMjEyMTQ0NDAwWhcNMjEwMjEyMTQ0NDAwWjA7MQswCQYDVQQGEwJOTDERMA8G +A1UECgwIUG9sYXJTU0wxGTAXBgNVBAMMEFBvbGFyU1NMIFRlc3QgQ0EwggEiMA0G +CSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDA3zf8F7vglp0/ht6WMn1EpRagzSHx +mdTs6st8GFgIlKXsm8WL3xoemTiZhx57wI053zhdcHgH057Zk+i5clHFzqMwUqny +50BwFMtEonILwuVA+T7lpg6z+exKY8C4KQB0nFc7qKUEkHHxvYPZP9al4jwqj+8n +YMPGn8u67GB9t+aEMr5P+1gmIgNb1LTV+/Xjli5wwOQuvfwu7uJBVcA0Ln0kcmnL +R7EUQIN9Z/SG9jGr8XmksrUuEvmEF/Bibyc+E1ixVA0hmnM3oTDPb5Lc9un8rNsu +KNF+AksjoBXyOGVkCeoMbo4bF6BxyLObyavpw/LPh5aPgAIynplYb6LVAgMBAAGj +UDBOMAwGA1UdEwQFMAMBAf8wHQYDVR0OBBYEFLRa5KWz3tJS9rnVppUP6z68x/3/ +MB8GA1UdIwQYMBaAFLRa5KWz3tJS9rnVppUP6z68x/3/MA0GCSqGSIb3DQEBBQUA +A4IBAQABE3OEPfEd/bcJW5ZdU3/VgPNS4tMzh8gnJP/V2FcvFtGylMpQq6YnEBYI +yBHAL4DRvlMY5rnXGBp3ODR8MpqHC6AquRTCLzjS57iYff//4QFQqW9n92zctspv +czkaPKgjqo1No3Uq0Xaz10rcxyTUPrf5wNVRZ2V0KvllvAAVSzbI4mpdUXztjhST +S5A2BeWQAAOr0zq1F7TSRVJpJs7jmB2ai/igkh1IAjcuwV6VwlP+sbw0gjQ0NpGM +iHpnlzRAi/tIbtOvMIGOBU2TIfax/5jq1agUx5aPmT5TWAiJPOOP6l5xXnDwxeYS +NWqiX9GyusBZjezaCaHabjDLU0qQ +-----END CERTIFICATE----- diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index 80e49d226..85f0d6556 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -399,6 +399,18 @@ X509 Certificate verification #5 (Revoked Cert) depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_SHA1_C:MBEDTLS_RSA_C:MBEDTLS_PKCS1_V15:MBEDTLS_HAVE_TIME_DATE x509_verify:"data_files/server1.crt":"data_files/test-ca.crt":"data_files/crl.pem":"NULL":MBEDTLS_ERR_X509_CERT_VERIFY_FAILED:MBEDTLS_X509_BADCERT_REVOKED:"compat":"NULL" +X509 Certificate verification #5' (Revoked Cert, differing DN string formats #1) +depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_SHA1_C:MBEDTLS_RSA_C:MBEDTLS_PKCS1_V15:MBEDTLS_HAVE_TIME_DATE +x509_verify:"data_files/server1.crt":"data_files/test-ca_utf8.crt":"data_files/crl.pem":"NULL":MBEDTLS_ERR_X509_CERT_VERIFY_FAILED:MBEDTLS_X509_BADCERT_REVOKED:"compat":"NULL" + +X509 Certificate verification #5'' (Revoked Cert, differing DN string formats #2) +depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_SHA1_C:MBEDTLS_RSA_C:MBEDTLS_PKCS1_V15:MBEDTLS_HAVE_TIME_DATE +x509_verify:"data_files/server1.crt":"data_files/test-ca_printable.crt":"data_files/crl.pem":"NULL":MBEDTLS_ERR_X509_CERT_VERIFY_FAILED:MBEDTLS_X509_BADCERT_REVOKED:"compat":"NULL" + +X509 Certificate verification #5''' (Revoked Cert, differing upper and lower case) +depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_SHA1_C:MBEDTLS_RSA_C:MBEDTLS_PKCS1_V15:MBEDTLS_HAVE_TIME_DATE +x509_verify:"data_files/server1.crt":"data_files/test-ca_uppercase.crt":"data_files/crl.pem":"NULL":MBEDTLS_ERR_X509_CERT_VERIFY_FAILED:MBEDTLS_X509_BADCERT_REVOKED:"compat":"NULL" + X509 Certificate verification #6 (Revoked Cert) depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_SHA1_C:MBEDTLS_RSA_C:MBEDTLS_PKCS1_V15:MBEDTLS_HAVE_TIME_DATE x509_verify:"data_files/server1.crt":"data_files/test-ca.crt":"data_files/crl.pem":"PolarSSL Server 1":MBEDTLS_ERR_X509_CERT_VERIFY_FAILED:MBEDTLS_X509_BADCERT_REVOKED:"compat":"NULL"