diff --git a/ChangeLog b/ChangeLog index 55391816c3..007f60418b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -14,6 +14,8 @@ Bugfix * Fix an issue that caused valid certificates to be rejected whenever an expired or not yet valid certificate was parsed before a valid certificate in the trusted certificate list. + * Fix bug in mbedtls_x509_crt_parse that caused trailing extra data in the + buffer after DER certificates to be included in the raw representation. Changes * On ARM platforms, when compiling with -O0 with GCC, Clang or armcc5, diff --git a/library/x509_crt.c b/library/x509_crt.c index 334b8ef515..c3adf7c864 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -677,14 +677,9 @@ static int x509_crt_parse_der_core( mbedtls_x509_crt *crt, const unsigned char * if( crt == NULL || buf == NULL ) return( MBEDTLS_ERR_X509_BAD_INPUT_DATA ); - p = mbedtls_calloc( 1, len = buflen ); - if( p == NULL ) - return( MBEDTLS_ERR_X509_ALLOC_FAILED ); - - memcpy( p, buf, buflen ); - - crt->raw.p = p; - crt->raw.len = len; + // Use the original buffer until we figure out actual length + p = (unsigned char*) buf; + len = buflen; end = p + len; /* @@ -708,6 +703,18 @@ static int x509_crt_parse_der_core( mbedtls_x509_crt *crt, const unsigned char * } crt_end = p + len; + // Create and populate a new buffer for the raw field + crt->raw.len = crt_end - buf; + crt->raw.p = p = mbedtls_calloc( 1, crt->raw.len ); + if( p == NULL ) + return( MBEDTLS_ERR_X509_ALLOC_FAILED ); + + memcpy( p, buf, crt->raw.len ); + + // Direct pointers to the new buffer + p += crt->raw.len - len; + end = crt_end = p + len; + /* * TBSCertificate ::= SEQUENCE { */ diff --git a/tests/data_files/server5-der0.crt b/tests/data_files/server5-der0.crt new file mode 100644 index 0000000000..08d8dd311b Binary files /dev/null and b/tests/data_files/server5-der0.crt differ diff --git a/tests/data_files/server5-der1a.crt b/tests/data_files/server5-der1a.crt new file mode 100644 index 0000000000..015017b17d Binary files /dev/null and b/tests/data_files/server5-der1a.crt differ diff --git a/tests/data_files/server5-der1b.crt b/tests/data_files/server5-der1b.crt new file mode 100644 index 0000000000..6340d9e2ed Binary files /dev/null and b/tests/data_files/server5-der1b.crt differ diff --git a/tests/data_files/server5-der2.crt b/tests/data_files/server5-der2.crt new file mode 100644 index 0000000000..c6e320a369 Binary files /dev/null and b/tests/data_files/server5-der2.crt differ diff --git a/tests/data_files/server5-der4.crt b/tests/data_files/server5-der4.crt new file mode 100644 index 0000000000..4af05cce1e Binary files /dev/null and b/tests/data_files/server5-der4.crt differ diff --git a/tests/data_files/server5-der8.crt b/tests/data_files/server5-der8.crt new file mode 100644 index 0000000000..65be7dcaeb Binary files /dev/null and b/tests/data_files/server5-der8.crt differ diff --git a/tests/data_files/server5-der9.crt b/tests/data_files/server5-der9.crt new file mode 100644 index 0000000000..4947f1f83f Binary files /dev/null and b/tests/data_files/server5-der9.crt differ diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 8792b21c25..c08af7b04e 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1559,6 +1559,64 @@ run_test "Renego ext: gnutls client unsafe, server break legacy" \ -S "received TLS_EMPTY_RENEGOTIATION_INFO\|found renegotiation extension" \ -S "server hello, secure renegotiation extension" +# Tests for silently dropping trailing extra bytes in .der certificates + +requires_gnutls +run_test "DER format: no trailing bytes" \ + "$P_SRV crt_file=data_files/server5-der0.crt \ + key_file=data_files/server5.key" \ + "$G_CLI " \ + 0 \ + -c "Handshake was completed" \ + +requires_gnutls +run_test "DER format: with a trailing zero byte" \ + "$P_SRV crt_file=data_files/server5-der1a.crt \ + key_file=data_files/server5.key" \ + "$G_CLI " \ + 0 \ + -c "Handshake was completed" \ + +requires_gnutls +run_test "DER format: with a trailing random byte" \ + "$P_SRV crt_file=data_files/server5-der1b.crt \ + key_file=data_files/server5.key" \ + "$G_CLI " \ + 0 \ + -c "Handshake was completed" \ + +requires_gnutls +run_test "DER format: with 2 trailing random bytes" \ + "$P_SRV crt_file=data_files/server5-der2.crt \ + key_file=data_files/server5.key" \ + "$G_CLI " \ + 0 \ + -c "Handshake was completed" \ + +requires_gnutls +run_test "DER format: with 4 trailing random bytes" \ + "$P_SRV crt_file=data_files/server5-der4.crt \ + key_file=data_files/server5.key" \ + "$G_CLI " \ + 0 \ + -c "Handshake was completed" \ + +requires_gnutls +run_test "DER format: with 8 trailing random bytes" \ + "$P_SRV crt_file=data_files/server5-der8.crt \ + key_file=data_files/server5.key" \ + "$G_CLI " \ + 0 \ + -c "Handshake was completed" \ + +requires_gnutls +run_test "DER format: with 9 trailing random bytes" \ + "$P_SRV crt_file=data_files/server5-der9.crt \ + key_file=data_files/server5.key" \ + "$G_CLI " \ + 0 \ + -c "Handshake was completed" \ + # Tests for auth_mode run_test "Authentication: server badcert, client required" \ diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index b21a64090c..5c68872c04 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -771,7 +771,7 @@ X509 Certificate ASN1 (Incorrect first tag) x509parse_crt:"":"":MBEDTLS_ERR_X509_INVALID_FORMAT X509 Certificate ASN1 (Correct first tag, data length does not match) -x509parse_crt:"300000":"":MBEDTLS_ERR_X509_INVALID_FORMAT + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG +x509parse_crt:"300000":"":MBEDTLS_ERR_X509_INVALID_FORMAT + MBEDTLS_ERR_ASN1_OUT_OF_DATA X509 Certificate ASN1 (Correct first tag, no more data) x509parse_crt:"3000":"":MBEDTLS_ERR_X509_INVALID_FORMAT + MBEDTLS_ERR_ASN1_OUT_OF_DATA