From b33dacdb5072a1a85e88f3c4e815f81b11d9d56b Mon Sep 17 00:00:00 2001 From: Werner Lewis Date: Fri, 20 May 2022 12:48:46 +0100 Subject: [PATCH 1/4] Fix parsing of special chars in X509 DN values Use escape mechanism defined in RFC 1779 when parsing commas and other special characters in X509 DN values. Resolves failures when generating a certificate with a CSR containing a comma in subject value. Fixes #769. Signed-off-by: Werner Lewis --- ChangeLog.d/fix-csr_subject_commas.txt | 3 +++ library/x509.c | 21 ++++++++++++++------- tests/data_files/Makefile | 8 +++++++- tests/data_files/server1.commas.crt | 20 ++++++++++++++++++++ tests/data_files/server1.req.commas.sha256 | 16 ++++++++++++++++ tests/suites/test_suite_x509parse.data | 8 ++++++++ tests/suites/test_suite_x509write.data | 2 +- 7 files changed, 69 insertions(+), 9 deletions(-) create mode 100644 ChangeLog.d/fix-csr_subject_commas.txt create mode 100644 tests/data_files/server1.commas.crt create mode 100644 tests/data_files/server1.req.commas.sha256 diff --git a/ChangeLog.d/fix-csr_subject_commas.txt b/ChangeLog.d/fix-csr_subject_commas.txt new file mode 100644 index 0000000000..d497b1211f --- /dev/null +++ b/ChangeLog.d/fix-csr_subject_commas.txt @@ -0,0 +1,3 @@ +Bugfix + * Fix string representation of DNs when parsing values containing commas, + conforming to RFC 1779. Fixes #769. diff --git a/library/x509.c b/library/x509.c index 17d103038c..4ff48284fd 100644 --- a/library/x509.c +++ b/library/x509.c @@ -741,7 +741,7 @@ int mbedtls_x509_get_ext( unsigned char **p, const unsigned char *end, int mbedtls_x509_dn_gets( char *buf, size_t size, const mbedtls_x509_name *dn ) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - size_t i, n; + size_t i, j, n; unsigned char c, merge = 0; const mbedtls_x509_name *name; const char *short_name = NULL; @@ -775,17 +775,24 @@ int mbedtls_x509_dn_gets( char *buf, size_t size, const mbedtls_x509_name *dn ) ret = mbedtls_snprintf( p, n, "\?\?=" ); MBEDTLS_X509_SAFE_SNPRINTF; - for( i = 0; i < name->val.len; i++ ) + for( i = 0, j = 0; i < name->val.len; i++, j++ ) { - if( i >= sizeof( s ) - 1 ) - break; + if( j >= sizeof( s ) - 1 ) + return( MBEDTLS_ERR_X509_BUFFER_TOO_SMALL ); c = name->val.p[i]; + // Special characters requiring escaping, RFC 1779 + if( c && strchr( ",=+<>#;\"\\", c ) ) + { + if( j + 1 >= sizeof( s ) - 1 ) + continue; + s[j++] = '\\'; + } if( c < 32 || c >= 127 ) - s[i] = '?'; - else s[i] = c; + s[j] = '?'; + else s[j] = c; } - s[i] = '\0'; + s[j] = '\0'; ret = mbedtls_snprintf( p, n, "%s", s ); MBEDTLS_X509_SAFE_SNPRINTF; diff --git a/tests/data_files/Makefile b/tests/data_files/Makefile index c0ad9b059e..6187d17bc3 100644 --- a/tests/data_files/Makefile +++ b/tests/data_files/Makefile @@ -909,6 +909,10 @@ server1.req.cert_type_empty: server1.key $(MBEDTLS_CERT_REQ) output_file=$@ filename=$< subject_name="C=NL,O=PolarSSL,CN=PolarSSL Server 1" md=SHA1 force_ns_cert_type=1 all_final += server1.req.cert_type_empty +server1.req.commas.sha256: server1.key + $(MBEDTLS_CERT_REQ) output_file=$@ filename=$< subject_name="C=NL,O=PolarSSL\, Commas,CN=PolarSSL Server 1" md=SHA256 +all_final += server1.req.commas.sha256 + # server2* server2_pwd_ec = PolarSSLTest @@ -966,7 +970,9 @@ server1.crt.der: server1.crt $(MBEDTLS_CERT_WRITE) request_file=server1.req.sha256 issuer_crt=$(test_ca_crt) issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) not_before=20190210144406 not_after=20290210144406 md=SHA1 authority_identifier=0 version=3 output_file=$@ server1.der: server1.crt $(OPENSSL) x509 -inform PEM -in $< -outform DER -out $@ -all_final += server1.crt server1.noauthid.crt server1.crt.der +server1.commas.crt: server1.key server1.req.commas.sha256 $(test_ca_crt) $(test_ca_key_file_rsa) + $(MBEDTLS_CERT_WRITE) request_file=server1.req.commas.sha256 issuer_crt=$(test_ca_crt) issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) version=1 not_before=20190210144406 not_after=20290210144406 md=SHA1 version=3 output_file=$@ +all_final += server1.crt server1.noauthid.crt server1.crt.der server1.commas.crt server1.key_usage.crt: server1.key server1.req.sha256 $(test_ca_crt) $(test_ca_key_file_rsa) $(MBEDTLS_CERT_WRITE) request_file=server1.req.sha256 issuer_crt=$(test_ca_crt) issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) version=1 not_before=20190210144406 not_after=20290210144406 md=SHA1 key_usage=digital_signature,non_repudiation,key_encipherment version=3 output_file=$@ diff --git a/tests/data_files/server1.commas.crt b/tests/data_files/server1.commas.crt new file mode 100644 index 0000000000..5acd2555d4 --- /dev/null +++ b/tests/data_files/server1.commas.crt @@ -0,0 +1,20 @@ +-----BEGIN CERTIFICATE----- +MIIDRzCCAi+gAwIBAgIBATANBgkqhkiG9w0BAQUFADA7MQswCQYDVQQGEwJOTDER +MA8GA1UECgwIUG9sYXJTU0wxGTAXBgNVBAMMEFBvbGFyU1NMIFRlc3QgQ0EwHhcN +MTkwMjEwMTQ0NDA2WhcNMjkwMjEwMTQ0NDA2WjBEMQswCQYDVQQGEwJOTDEZMBcG +A1UECgwQUG9sYXJTU0wsIENvbW1hczEaMBgGA1UEAwwRUG9sYXJTU0wgU2VydmVy +IDEwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCpAh89QGrVVVOL/Tbu +gmUuFWFeib+46EWQ2+6IFlLT8UNQR5YSWWSHa/0r4Eb5c77dz5LhkVvtZqBviSl5 +RYDQg2rVQUN3Xzl8CQRHgrBXOXDto+wVGR6oMwhHwQVCqf1Mw7Tf3QYfTRBRQGdz +Ew9A+G2BJV8KsVPGMH4VOaz5Wu5/kp6mBVvnE5eFtSOS2dQkBtUJJYl1B92mGo8/ +CRm+rWUsZOuVm9z+QV4XptpsW2nMAroULBYknErczdD3Umdz8S2gI/1+9DHKLXDK +iQsE2y6mT3Buns69WIniU1meblqSZeKIPwyUGaPd5eidlRPtKdurcBLcWsprF6tS +glSxAgMBAAGjTTBLMAkGA1UdEwQCMAAwHQYDVR0OBBYEFB901j8pwXR0RTsFEiw9 +qL1DWQKmMB8GA1UdIwQYMBaAFLRa5KWz3tJS9rnVppUP6z68x/3/MA0GCSqGSIb3 +DQEBBQUAA4IBAQA1Ecg+VVJRmgFF9cnlztnXj4y9QKj8MCf2uZA3nTNe1Deh9l17 +ZNNWdPkXzVzf0IeR3LQRKT+daTzxuOOCSV9OxOcN0dIODBwa97BtNQfuWw2eWC9I +3UOVXbx8Ga+bXnD8ouatpyEG0FfhLO5YgEP0K9TyyN/nFa9kkB2Kvpy8yWm3w9WG +WgsOr2fpIExfC2ZFaiu3NVGTpT9fLv8RTatSC1XLA5Sr8NNHia3zCvEJEAlTuFHs +wm8apIAHlb44bbgW+7UwBIH9r2A21gQFy3v4cTLtlbnaUBbHUJvarK4ru70J+gew +OO3NZ1ocvnV+qGIcc7LgyNA8pZW5Jbewb/gN +-----END CERTIFICATE----- diff --git a/tests/data_files/server1.req.commas.sha256 b/tests/data_files/server1.req.commas.sha256 new file mode 100644 index 0000000000..0287a31109 --- /dev/null +++ b/tests/data_files/server1.req.commas.sha256 @@ -0,0 +1,16 @@ +-----BEGIN CERTIFICATE REQUEST----- +MIICiTCCAXECAQAwRDELMAkGA1UEBhMCTkwxGTAXBgNVBAoMEFBvbGFyU1NMLCBD +b21tYXMxGjAYBgNVBAMMEVBvbGFyU1NMIFNlcnZlciAxMIIBIjANBgkqhkiG9w0B +AQEFAAOCAQ8AMIIBCgKCAQEAqQIfPUBq1VVTi/027oJlLhVhXom/uOhFkNvuiBZS +0/FDUEeWEllkh2v9K+BG+XO+3c+S4ZFb7Wagb4kpeUWA0INq1UFDd185fAkER4Kw +Vzlw7aPsFRkeqDMIR8EFQqn9TMO0390GH00QUUBncxMPQPhtgSVfCrFTxjB+FTms ++Vruf5KepgVb5xOXhbUjktnUJAbVCSWJdQfdphqPPwkZvq1lLGTrlZvc/kFeF6ba +bFtpzAK6FCwWJJxK3M3Q91Jnc/EtoCP9fvQxyi1wyokLBNsupk9wbp7OvViJ4lNZ +nm5akmXiiD8MlBmj3eXonZUT7Snbq3AS3FrKaxerUoJUsQIDAQABoAAwDQYJKoZI +hvcNAQELBQADggEBAI7ZtRJYX6cMuwVhwXOizPV+WD17wby+273V4R8e9/6QA4nY +RrSciAse+nWZz9Y6toBzLWr0K/9SCzwBX4OzMvLqu4A1G/wApODCDnbGaUPNUxRt +6qbg8y7faBWvDGjk4+OpQ0suR/pdbM/L7pImqWRNwYdSPbJumNqIdB/Ewtso0TlA +QVZ992RPe1LovXpDCfPP2p123L7/UHezNCtu5QmzLsDfQmN/rLhCJ2NZzTsnIdnP +jp6XYU4kRV2BPDL65k38k8CSVWb6fw9XwPNUiyO3q1Zs6jpGJRYMLj9qTEoRN1np +RME09CN2siMcgkv8UqDeDJ4Oa9qyXS6VXsDmSNI= +-----END CERTIFICATE REQUEST----- diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index f62fba397f..04831c981c 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -294,6 +294,10 @@ X509 CSR Information RSA with SHA512 depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_SHA512_C:MBEDTLS_RSA_C:!MBEDTLS_X509_REMOVE_INFO mbedtls_x509_csr_info:"data_files/server1.req.sha512":"CSR version \: 1\nsubject name \: C=NL, O=PolarSSL, CN=PolarSSL Server 1\nsigned using \: RSA with SHA-512\nRSA key size \: 2048 bits\n" +X509 CSR Information RSA with SHA-256, containing commas +depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_SHA256_C:MBEDTLS_RSA_C:MBEDTS_X509_INFO +mbedtls_x509_csr_info:"data_files/server1.req.commas.sha256":"CSR version \: 1\nsubject name \: C=NL, O=PolarSSL\, Commas, CN=PolarSSL Server 1\nsigned using \: RSA with SHA-256\nRSA key size \: 2048 bits\n" + X509 CSR Information EC with SHA1 depends_on:MBEDTLS_ECDSA_C:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA1_C:!MBEDTLS_X509_REMOVE_INFO mbedtls_x509_csr_info:"data_files/server5.req.sha1":"CSR version \: 1\nsubject name \: C=NL, O=PolarSSL, CN=localhost\nsigned using \: ECDSA with SHA1\nEC key size \: 256 bits\n" @@ -375,6 +379,10 @@ X509 Get Distinguished Name #4 depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_SHA1_C mbedtls_x509_dn_gets:"data_files/server2.crt":"issuer":"C=NL, O=PolarSSL, CN=PolarSSL Test CA" +X509 Get Distinguished Name #5 +depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_SHA1_C +mbedtls_x509_dn_gets:"data_files/server1.commas.crt":"subject":"C=NL, O=PolarSSL\, Commas, CN=PolarSSL Server 1" + X509 Get Next DN #1 No Multivalue RDNs mbedtls_x509_dn_get_next:"C=NL, O=PolarSSL, CN=PolarSSL Server 1":0:"C O CN":3:"C=NL, O=PolarSSL, CN=PolarSSL Server 1" diff --git a/tests/suites/test_suite_x509write.data b/tests/suites/test_suite_x509write.data index 8d9a11a99e..91fdd86d7b 100644 --- a/tests/suites/test_suite_x509write.data +++ b/tests/suites/test_suite_x509write.data @@ -139,7 +139,7 @@ depends_on:MBEDTLS_SHA256_C:MBEDTLS_ECDSA_C:MBEDTLS_ECDSA_DETERMINISTIC:MBEDTLS_ x509_crt_check:"data_files/server5.key":"":"C=NL,O=PolarSSL,CN=PolarSSL Server 1":"data_files/test-ca2.key":"PolarSSLTest":"C=NL,O=PolarSSL,CN=Polarssl Test EC CA":"1":"20190210144406":"20290210144406":MBEDTLS_MD_SHA256:0:0:0:0:1:-1:"":2:0:"data_files/test-ca2.crt" X509 String to Names #1 -mbedtls_x509_string_to_names:"C=NL,O=Offspark\, Inc., OU=PolarSSL":"C=NL, O=Offspark, Inc., OU=PolarSSL":0 +mbedtls_x509_string_to_names:"C=NL,O=Offspark\, Inc., OU=PolarSSL":"C=NL, O=Offspark\, Inc., OU=PolarSSL":0 X509 String to Names #2 mbedtls_x509_string_to_names:"C=NL, O=Offspark, Inc., OU=PolarSSL":"":MBEDTLS_ERR_X509_UNKNOWN_OID From 31ecb9600a371aca910b88361b297ecb8fce08c5 Mon Sep 17 00:00:00 2001 From: Werner Lewis Date: Fri, 17 Jun 2022 15:51:55 +0100 Subject: [PATCH 2/4] Add tests for exceeded buffer size Signed-off-by: Werner Lewis --- tests/suites/test_suite_x509parse.data | 16 +++++++++++ tests/suites/test_suite_x509parse.function | 32 +++++++++++++++++++++- 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index 04831c981c..fae0f4b570 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -383,6 +383,22 @@ X509 Get Distinguished Name #5 depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_SHA1_C mbedtls_x509_dn_gets:"data_files/server1.commas.crt":"subject":"C=NL, O=PolarSSL\, Commas, CN=PolarSSL Server 1" +X509 Get Modified DN #1 +depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_SHA1_C +mbedtls_x509_dn_gets_subject_replace:"data_files/server1.crt":"Modified":"C=NL, O=Modified, CN=PolarSSL Server 1":0 + +X509 Get Modified DN #2 Name exactly 255 bytes +depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_SHA1_C +mbedtls_x509_dn_gets_subject_replace:"data_files/server1.crt":"123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345":"C=NL, O=123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345, CN=PolarSSL Server 1":0 + +X509 Get Modified DN #3 Name exceeds 255 bytes +depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_SHA1_C +mbedtls_x509_dn_gets_subject_replace:"data_files/server1.crt":"1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456":"":MBEDTLS_ERR_X509_BUFFER_TOO_SMALL + +X509 Get Modified DN #4 Name exactly 255 bytes, with comma requiring escaping +depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_SHA1_C +mbedtls_x509_dn_gets_subject_replace:"data_files/server1.crt":"1234567890,1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234":"":MBEDTLS_ERR_X509_BUFFER_TOO_SMALL + X509 Get Next DN #1 No Multivalue RDNs mbedtls_x509_dn_get_next:"C=NL, O=PolarSSL, CN=PolarSSL Server 1":0:"C O CN":3:"C=NL, O=PolarSSL, CN=PolarSSL Server 1" diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index a9d7e47c8b..3bb68d9d53 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -757,6 +757,37 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_FS_IO:MBEDTLS_X509_CRT_PARSE_C:!MBEDTLS_X509_REMOVE_INFO */ +void mbedtls_x509_dn_gets_subject_replace( char * crt_file, char * new_subject_ou, char * result_str, int ret ) +{ + mbedtls_x509_crt crt; + char buf[2000]; + int res = 0; + + mbedtls_x509_crt_init( &crt ); + memset( buf, 0, 2000 ); + + TEST_ASSERT( mbedtls_x509_crt_parse_file( &crt, crt_file ) == 0 ); + crt.subject.next->val.p = (unsigned char *) new_subject_ou; + crt.subject.next->val.len = strlen( new_subject_ou ); + + res = mbedtls_x509_dn_gets( buf, 2000, &crt.subject ); + + if ( ret != 0 ) + { + TEST_ASSERT( res == ret ); + } + else + { + TEST_ASSERT( res != -1 ); + TEST_ASSERT( res != -2 ); + TEST_ASSERT( strcmp( buf, result_str ) == 0 ); + } +exit: + mbedtls_x509_crt_free( &crt ); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_FS_IO:MBEDTLS_X509_CRT_PARSE_C:!MBEDTLS_X509_REMOVE_INFO */ void mbedtls_x509_dn_gets( char * crt_file, char * entity, char * result_str ) { @@ -854,7 +885,6 @@ exit: mbedtls_free( parsed_prv ); } } - /* END_CASE */ /* BEGIN_CASE depends_on:MBEDTLS_FS_IO:MBEDTLS_X509_CRT_PARSE_C */ From fd8cfe4f8ed26f807ca14f026e79bf3a4dcd8d18 Mon Sep 17 00:00:00 2001 From: Werner Lewis Date: Mon, 27 Jun 2022 09:58:12 +0100 Subject: [PATCH 3/4] Replace parsing with outputting Signed-off-by: Werner Lewis --- ChangeLog.d/fix-csr_subject_commas.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ChangeLog.d/fix-csr_subject_commas.txt b/ChangeLog.d/fix-csr_subject_commas.txt index d497b1211f..e01c9a88ce 100644 --- a/ChangeLog.d/fix-csr_subject_commas.txt +++ b/ChangeLog.d/fix-csr_subject_commas.txt @@ -1,3 +1,3 @@ Bugfix - * Fix string representation of DNs when parsing values containing commas, - conforming to RFC 1779. Fixes #769. + * Fix string representation of DNs when outputting values containing commas + and other special characters, conforming to RFC 1779. Fixes #769. From 9b0e940135d5794dfaf8843e3064a398455d0ef7 Mon Sep 17 00:00:00 2001 From: Werner Lewis Date: Mon, 27 Jun 2022 12:01:22 +0100 Subject: [PATCH 4/4] Fix case where final special char exceeds buffer Signed-off-by: Werner Lewis --- library/x509.c | 2 +- tests/suites/test_suite_x509parse.data | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/library/x509.c b/library/x509.c index 4ff48284fd..55619dc5db 100644 --- a/library/x509.c +++ b/library/x509.c @@ -785,7 +785,7 @@ int mbedtls_x509_dn_gets( char *buf, size_t size, const mbedtls_x509_name *dn ) if( c && strchr( ",=+<>#;\"\\", c ) ) { if( j + 1 >= sizeof( s ) - 1 ) - continue; + return( MBEDTLS_ERR_X509_BUFFER_TOO_SMALL ); s[j++] = '\\'; } if( c < 32 || c >= 127 ) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index fae0f4b570..eb9e9aa23d 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -399,6 +399,10 @@ X509 Get Modified DN #4 Name exactly 255 bytes, with comma requiring escaping depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_SHA1_C mbedtls_x509_dn_gets_subject_replace:"data_files/server1.crt":"1234567890,1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234":"":MBEDTLS_ERR_X509_BUFFER_TOO_SMALL +X509 Get Modified DN #5 Name exactly 255 bytes, ending with comma requiring escaping +depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_RSA_C:MBEDTLS_SHA1_C +mbedtls_x509_dn_gets_subject_replace:"data_files/server1.crt":"12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234,":"":MBEDTLS_ERR_X509_BUFFER_TOO_SMALL + X509 Get Next DN #1 No Multivalue RDNs mbedtls_x509_dn_get_next:"C=NL, O=PolarSSL, CN=PolarSSL Server 1":0:"C O CN":3:"C=NL, O=PolarSSL, CN=PolarSSL Server 1"