From c058074836c35a8f6a10533442202fcc8a32f977 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 19 Nov 2015 10:46:07 +0100 Subject: [PATCH 1/3] Add test case for first intermediate max_pathlen=0 !!! This test case is currently failing !!! (See fix in next-next commit.) Test certificates generated with the following script: programs/pkey/gen_key type=ec ec_curve=secp256r1 filename=cert81.key programs/pkey/gen_key type=ec ec_curve=secp256r1 filename=cert82.key programs/pkey/gen_key type=ec ec_curve=secp256r1 filename=cert83.key programs/x509/cert_write serial=81 output_file=cert81.crt is_ca=1 \ issuer_key=cert81.key issuer_name="CN=Root 8,O=mbed TLS,C=UK" \ selfsign=1 programs/x509/cert_write serial=82 output_file=cert82.crt is_ca=1 \ issuer_key=cert81.key issuer_name="CN=Root 8,O=mbed TLS,C=UK" \ subject_key=cert82.key subject_name="CN=Int 82,O=mbed TLS,C=UK" \ max_pathlen=0 programs/x509/cert_write serial=83 output_file=cert83.crt \ issuer_key=cert82.key issuer_name="CN=Int 82,O=mbed TLS,C=UK" \ subject_key=cert83.key subject_name="CN=EE 83,O=mbed TLS,C=UK" mv cert8?.crt tests/data_files/dir4 rm cert8?.key --- tests/data_files/dir4/Readme | 4 ++++ tests/data_files/dir4/cert81.crt | 11 +++++++++++ tests/data_files/dir4/cert82.crt | 11 +++++++++++ tests/data_files/dir4/cert83.crt | 11 +++++++++++ tests/suites/test_suite_x509parse.data | 4 ++++ 5 files changed, 41 insertions(+) create mode 100644 tests/data_files/dir4/cert81.crt create mode 100644 tests/data_files/dir4/cert82.crt create mode 100644 tests/data_files/dir4/cert83.crt diff --git a/tests/data_files/dir4/Readme b/tests/data_files/dir4/Readme index 5732a6463..7217b75eb 100644 --- a/tests/data_files/dir4/Readme +++ b/tests/data_files/dir4/Readme @@ -36,3 +36,7 @@ cert61.crt (max_pathlen=1) -> cert62.crt -> cert63.crt cert71.crt (max_pathlen=1) -> cert72.crt -> cert73.crt (self signed) -> cert74.crt -> cert74.crt ``` +8. zero pathlen constraint on first intermediate CA (valid) +``` +cert81.crt -> cert82.crt (max_pathlen=0) -> cert83.crt +``` diff --git a/tests/data_files/dir4/cert81.crt b/tests/data_files/dir4/cert81.crt new file mode 100644 index 000000000..26b2bd555 --- /dev/null +++ b/tests/data_files/dir4/cert81.crt @@ -0,0 +1,11 @@ +-----BEGIN CERTIFICATE----- +MIIBpTCCAUmgAwIBAgIBUTAMBggqhkjOPQQDAgUAMDExDzANBgNVBAMTBlJvb3Qg +ODERMA8GA1UEChMIbWJlZCBUTFMxCzAJBgNVBAYTAlVLMB4XDTAxMDEwMTAwMDAw +MFoXDTMwMTIzMTIzNTk1OVowMTEPMA0GA1UEAxMGUm9vdCA4MREwDwYDVQQKEwht +YmVkIFRMUzELMAkGA1UEBhMCVUswWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAT1 +GuTQ9vgf2l3oLM25r78cvIAQqE02GzQGjp/WWw3CysEwTwNEuZGhRiD5lDmkbUGW +UNxv/7uJjy7k3K3fDNdko1AwTjAMBgNVHRMEBTADAQH/MB0GA1UdDgQWBBTHFA2h +Au0tPnzeYnLcmlTQj4FAajAfBgNVHSMEGDAWgBTHFA2hAu0tPnzeYnLcmlTQj4FA +ajAMBggqhkjOPQQDAgUAA0gAMEUCIH7Z/HNb/Pwbs40iNll1a9gmgAbYOgdlVPWo +nSdcb7cZAiEAlhVb6CdBXsjOfAWWEET/QP74z608PKFccCIFPCDLkxo= +-----END CERTIFICATE----- diff --git a/tests/data_files/dir4/cert82.crt b/tests/data_files/dir4/cert82.crt new file mode 100644 index 000000000..d49ecc9f3 --- /dev/null +++ b/tests/data_files/dir4/cert82.crt @@ -0,0 +1,11 @@ +-----BEGIN CERTIFICATE----- +MIIBqDCCAUygAwIBAgIBUjAMBggqhkjOPQQDAgUAMDExDzANBgNVBAMTBlJvb3Qg +ODERMA8GA1UEChMIbWJlZCBUTFMxCzAJBgNVBAYTAlVLMB4XDTAxMDEwMTAwMDAw +MFoXDTMwMTIzMTIzNTk1OVowMTEPMA0GA1UEAxMGSW50IDgyMREwDwYDVQQKEwht +YmVkIFRMUzELMAkGA1UEBhMCVUswWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAS2 +giYQt4HVfQ2t8eTS0bvISwp7ol2x17umbllBxwzGDFEUQ00JL1/SStezecK0lNhE +0AvY8Ez2soQEtdSeQGkCo1MwUTAPBgNVHRMECDAGAQH/AgEAMB0GA1UdDgQWBBS3 ++nsv3nQknSg4aDjlTiRpCPo7XzAfBgNVHSMEGDAWgBTHFA2hAu0tPnzeYnLcmlTQ +j4FAajAMBggqhkjOPQQDAgUAA0gAMEUCIQDus2Lvx3yyvaViY1s334uMm6ge484X +oktMyxLVjkAMiAIgehTHiJJaT9PnlVa+hUpxsIfVAuMexrm5fw/bDF5Nxzw= +-----END CERTIFICATE----- diff --git a/tests/data_files/dir4/cert83.crt b/tests/data_files/dir4/cert83.crt new file mode 100644 index 000000000..21a748e32 --- /dev/null +++ b/tests/data_files/dir4/cert83.crt @@ -0,0 +1,11 @@ +-----BEGIN CERTIFICATE----- +MIIBoDCCAUWgAwIBAgIBUzAMBggqhkjOPQQDAgUAMDExDzANBgNVBAMTBkludCA4 +MjERMA8GA1UEChMIbWJlZCBUTFMxCzAJBgNVBAYTAlVLMB4XDTAxMDEwMTAwMDAw +MFoXDTMwMTIzMTIzNTk1OVowMDEOMAwGA1UEAxMFRUUgODMxETAPBgNVBAoTCG1i +ZWQgVExTMQswCQYDVQQGEwJVSzBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABMSy +6X5iBYrdxxOMfdcA23pLBoJCeyEjiWfALxTm80MJGBdRNVdnT50xNU3SDDwHWPda +/EQqHq+itsqkUeyAGAyjTTBLMAkGA1UdEwQCMAAwHQYDVR0OBBYEFGsFH/KsvM4n +r+i1gI2iCVXi3KtFMB8GA1UdIwQYMBaAFLf6ey/edCSdKDhoOOVOJGkI+jtfMAwG +CCqGSM49BAMCBQADRwAwRAIgQURH8DHWFHVK38+znWc85G1P+g4ocdkA5Gt0LbOg +SJMCIBsacOLFywxZYF8atizw6zMRw+QeHR2514JIhJUck2kd +-----END CERTIFICATE----- diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index a540db256..255c4e19d 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -1156,6 +1156,10 @@ X509 CRT verify chain #8 (self signed maxpathlen root) depends_on:POLARSSL_SHA256_C:POLARSSL_RSA_C x509_crt_verify_chain:"data_files/dir4/cert61.crt data_files/dir4/cert63.crt data_files/dir4/cert62.crt":"data_files/dir4/cert61.crt":0 +X509 CRT verify chain #9 (self signed maxpathlen root) +depends_on:POLARSSL_SHA256_C:POLARSSL_ECDSA_C:POLARSSL_ECP_DP_SECP256R1_ENABLED +x509_crt_verify_chain:"data_files/dir4/cert83.crt data_files/dir4/cert82.crt":"data_files/dir4/cert81.crt":0 + X509 OID description #1 x509_oid_desc:"2B06010505070301":"TLS Web Server Authentication" From 6ad4f65780d1afdb4970b957855c1ec68483edd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 19 Nov 2015 10:52:12 +0100 Subject: [PATCH 2/3] Add test case for root with max_pathlen=0 This was already working but not tested so far (Test case from previous commit still failing.) Test certificates generated with: programs/pkey/gen_key type=ec ec_curve=secp256r1 filename=cert91.key programs/pkey/gen_key type=ec ec_curve=secp256r1 filename=cert92.key programs/x509/cert_write serial=91 output_file=cert91.crt is_ca=1 \ issuer_key=cert91.key issuer_name="CN=Root 9,O=mbed TLS,C=UK" \ selfsign=1 max_pathlen=0 programs/x509/cert_write serial=92 output_file=cert92.crt \ issuer_key=cert91.key issuer_name="CN=Root 9,O=mbed TLS,C=UK" \ subject_key=cert92.key subject_name="CN=EE 92,O=mbed TLS,C=UK" mv cert9?.crt tests/data_files/dir4 rm cert9?.key --- tests/data_files/dir4/Readme | 5 +++++ tests/data_files/dir4/cert91.crt | 11 +++++++++++ tests/data_files/dir4/cert92.crt | 11 +++++++++++ tests/suites/test_suite_x509parse.data | 6 +++++- 4 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 tests/data_files/dir4/cert91.crt create mode 100644 tests/data_files/dir4/cert92.crt diff --git a/tests/data_files/dir4/Readme b/tests/data_files/dir4/Readme index 7217b75eb..3f1f610b9 100644 --- a/tests/data_files/dir4/Readme +++ b/tests/data_files/dir4/Readme @@ -40,3 +40,8 @@ cert71.crt (max_pathlen=1) -> cert72.crt -> cert73.crt (self signed) -> cert74.c ``` cert81.crt -> cert82.crt (max_pathlen=0) -> cert83.crt ``` + +9. zero pathlen constraint on trusted root (valid) +``` +cert91.crt (max_pathlen=0) -> cert92.crt +``` diff --git a/tests/data_files/dir4/cert91.crt b/tests/data_files/dir4/cert91.crt new file mode 100644 index 000000000..6d4605a7c --- /dev/null +++ b/tests/data_files/dir4/cert91.crt @@ -0,0 +1,11 @@ +-----BEGIN CERTIFICATE----- +MIIBqTCCAUygAwIBAgIBWzAMBggqhkjOPQQDAgUAMDExDzANBgNVBAMTBlJvb3Qg +OTERMA8GA1UEChMIbWJlZCBUTFMxCzAJBgNVBAYTAlVLMB4XDTAxMDEwMTAwMDAw +MFoXDTMwMTIzMTIzNTk1OVowMTEPMA0GA1UEAxMGUm9vdCA5MREwDwYDVQQKEwht +YmVkIFRMUzELMAkGA1UEBhMCVUswWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAATh +D2SmdS6D7cYi2vGMyuCdol/OOUN2di2pS2wfSI/MsY/Z4O9iNHqbXQP6l+hcT5ap +daycs7r6ZPNqmWM7b16go1MwUTAPBgNVHRMECDAGAQH/AgEAMB0GA1UdDgQWBBRb +zVrcAxddj0i0DEqvTGT8F37bizAfBgNVHSMEGDAWgBRbzVrcAxddj0i0DEqvTGT8 +F37bizAMBggqhkjOPQQDAgUAA0kAMEYCIQDbrSV4ndH0vAR3HqJfBn8NT8zdvMjB +qSJes6Qwa42b2wIhAKyoH0H+b1Svw8pMkvUYF4ElH5Cnn7gxb7Wl3arc0+hQ +-----END CERTIFICATE----- diff --git a/tests/data_files/dir4/cert92.crt b/tests/data_files/dir4/cert92.crt new file mode 100644 index 000000000..49b53a5bc --- /dev/null +++ b/tests/data_files/dir4/cert92.crt @@ -0,0 +1,11 @@ +-----BEGIN CERTIFICATE----- +MIIBoTCCAUWgAwIBAgIBXDAMBggqhkjOPQQDAgUAMDExDzANBgNVBAMTBlJvb3Qg +OTERMA8GA1UEChMIbWJlZCBUTFMxCzAJBgNVBAYTAlVLMB4XDTAxMDEwMTAwMDAw +MFoXDTMwMTIzMTIzNTk1OVowMDEOMAwGA1UEAxMFRUUgOTIxETAPBgNVBAoTCG1i +ZWQgVExTMQswCQYDVQQGEwJVSzBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABC9E +tK1pE8Ei8vgScunyjx50C+qDsQS8D2RhGHC4VkE2yyiFxJA/ynhoeXTKZsHuEWI9 +CfOSvk0RrTWf9nr0pTGjTTBLMAkGA1UdEwQCMAAwHQYDVR0OBBYEFLqsN52tAf1k +XlzxQmdD5qG6Sy6PMB8GA1UdIwQYMBaAFFvNWtwDF12PSLQMSq9MZPwXftuLMAwG +CCqGSM49BAMCBQADSAAwRQIgXlfKqhkhXgK112Eycl+Z5NHM+6aqXE7i9j7IyGfk +ikICIQDBYNGbpSx82XG+IS/h4AWNTa4Hs6rmWvQDWJum7NrzMQ== +-----END CERTIFICATE----- diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index 255c4e19d..cdfc9bae9 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -1156,10 +1156,14 @@ X509 CRT verify chain #8 (self signed maxpathlen root) depends_on:POLARSSL_SHA256_C:POLARSSL_RSA_C x509_crt_verify_chain:"data_files/dir4/cert61.crt data_files/dir4/cert63.crt data_files/dir4/cert62.crt":"data_files/dir4/cert61.crt":0 -X509 CRT verify chain #9 (self signed maxpathlen root) +X509 CRT verify chain #9 (zero pathlen first intermediate, valid) depends_on:POLARSSL_SHA256_C:POLARSSL_ECDSA_C:POLARSSL_ECP_DP_SECP256R1_ENABLED x509_crt_verify_chain:"data_files/dir4/cert83.crt data_files/dir4/cert82.crt":"data_files/dir4/cert81.crt":0 +X509 CRT verify chain #10 (zero pathlen root, valid) +depends_on:POLARSSL_SHA256_C:POLARSSL_ECDSA_C:POLARSSL_ECP_DP_SECP256R1_ENABLED +x509_crt_verify_chain:"data_files/dir4/cert92.crt":"data_files/dir4/cert91.crt":0 + X509 OID description #1 x509_oid_desc:"2B06010505070301":"TLS Web Server Authentication" From c4a47e3483a00d0d60d5f87a758d3b0ad0d41f30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 19 Nov 2015 09:23:06 +0100 Subject: [PATCH 3/3] Fix bug checking pathlen on first intermediate Remove check on the pathLenConstraint value when looking for a parent to the EE cert, as the constraint is on the number of intermediate certs below the parent, and that number is always 0 at that point, so the constraint is always satisfied. The check was actually off-by-one, which caused valid chains to be rejected under the following conditions: - the parent certificate is not a trusted root, and - it has pathLenConstraint == 0 (max_pathlen == 1 in our representation) fixes #280 --- ChangeLog | 7 +++++++ library/x509_crt.c | 10 ---------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/ChangeLog b/ChangeLog index bf31b70c8..ea2dd730f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,12 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS 1.3.16 released 2015-11-xx + +Bugfix + * Fix bug in certificate validation that caused valid chains to be rejected + when the first intermediate certificate has pathLenConstraint=0. Found by + Nicholas Wilson. Introduced in mbed TLS 1.3.15. #280 + = mbed TLS 1.3.15 released 2015-11-04 Security diff --git a/library/x509_crt.c b/library/x509_crt.c index 92a9b17d0..5a15c74fd 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -2092,18 +2092,8 @@ int x509_crt_verify( x509_crt *crt, { /* Look for a parent upwards the chain */ for( parent = crt->next; parent != NULL; parent = parent->next ) - { - /* +2 because the current step is not yet accounted for - * and because max_pathlen is one higher than it should be */ - if( parent->max_pathlen > 0 && - parent->max_pathlen < 2 + pathlen ) - { - continue; - } - if( x509_crt_check_parent( crt, parent, 0, pathlen == 0 ) == 0 ) break; - } /* Are we part of the chain or at the top? */ if( parent != NULL )