From ca17ebfbc02b57e2bcb42efe64a5f2002c756ea8 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Tue, 24 Nov 2020 17:30:18 +0000 Subject: [PATCH] Add tag check to cert algorithm check Add missing tag check for algorithm parameters when comparing the signature in the description part of the cert against the actual signature whilst loading a certificate. This was found by a certificate (created by fuzzing) that openssl would not verify, but mbedtls would. Regression test added (one of the client certs modified accordingly) Signed-off-by: Paul Elliott --- .../x509-add-tag-check-to-algorithm-params | 11 +++++++++++ library/x509_crt.c | 1 + tests/data_files/Makefile | 6 +++++- tests/data_files/cli-rsa-sha256-badalg.crt.der | Bin 0 -> 835 bytes tests/suites/test_suite_x509parse.data | 4 ++++ 5 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 ChangeLog.d/x509-add-tag-check-to-algorithm-params create mode 100644 tests/data_files/cli-rsa-sha256-badalg.crt.der diff --git a/ChangeLog.d/x509-add-tag-check-to-algorithm-params b/ChangeLog.d/x509-add-tag-check-to-algorithm-params new file mode 100644 index 000000000..f2c72b0ec --- /dev/null +++ b/ChangeLog.d/x509-add-tag-check-to-algorithm-params @@ -0,0 +1,11 @@ +Security + * Fix a compliance issue whereby we were not checking the tag on the + algorithm parameters (only the size) when comparing the signature in the + description part of the cert to the real signature. This meant that a + NULL algorithm parameters entry would look identical to an array of REAL + (size zero) to the library and thus the certificate would be considered + valid. However, if the parameters do not match in *any* way then the + certificate should be considered invalid, and indeed OpenSSL marks these + certs as invalid when mbedtls did not. + Many thanks to guidovranken who found this issue via differential fuzzing + and reported it in #3629. diff --git a/library/x509_crt.c b/library/x509_crt.c index fcc2ed21e..690d99d95 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1304,6 +1304,7 @@ static int x509_crt_parse_der_core( mbedtls_x509_crt *crt, if( crt->sig_oid.len != sig_oid2.len || memcmp( crt->sig_oid.p, sig_oid2.p, crt->sig_oid.len ) != 0 || + sig_params1.tag != sig_params2.tag || sig_params1.len != sig_params2.len || ( sig_params1.len != 0 && memcmp( sig_params1.p, sig_params2.p, sig_params1.len ) != 0 ) ) diff --git a/tests/data_files/Makefile b/tests/data_files/Makefile index 40c22f53b..4c0920f39 100644 --- a/tests/data_files/Makefile +++ b/tests/data_files/Makefile @@ -206,7 +206,11 @@ cli-rsa-sha256.crt.der: cli-rsa-sha256.crt $(OPENSSL) x509 -in $< -out $@ -inform PEM -outform DER all_final += cli-rsa-sha256.crt.der - cli-rsa.key.der: $(cli_crt_key_file_rsa) +cli-rsa-sha256-badalg.crt.der: cli-rsa-sha256.crt.der + hexdump -ve '1/1 "%.2X"' $< | sed "s/06092A864886F70D01010B0500/06092A864886F70D01010B0900/2" | xxd -r -p > $@ +all_final += cli-rsa-sha256-badalg.crt.der + +cli-rsa.key.der: $(cli_crt_key_file_rsa) $(OPENSSL) pkey -in $< -out $@ -inform PEM -outform DER all_final += cli-rsa.key.der diff --git a/tests/data_files/cli-rsa-sha256-badalg.crt.der b/tests/data_files/cli-rsa-sha256-badalg.crt.der new file mode 100644 index 0000000000000000000000000000000000000000..c40ba2a44b4c7a6d511eb40815cd9be1729619ce GIT binary patch literal 835 zcmXqLVzxJEVp3ng%*4pV#K>a6%f_kI=F#?@mywa1mBGN;klTQhjX9KsO_<5g$57CK zAH?C};RwjjNh}Hu_A!(+5C;h{^9aC%6hcyqOB9?P4dldm4J{3f3=IrTOiT>SqQrTP zkhumn1PzxmkboF22shk0Co?s#M8U|QiBSpJwT!F`%uS5^3_x)%rY1&4h7%=6&g}fT zweqFwO_78RwYw*O%9fjNyq34W%O))K=^dAwXVO~Pul`Wq;AMItc^+4u^!8gH>Q=ww z5t!WODPi(?h1R9;uX8kMIOHyy2Ilpoed0q1Az+3q% z;eIPWuXuau!9jyU10#d0m%njb2=jOA`xO7vHhp4;fu7T*`?bHH@|6F7V|bvVfB&KS zQ)_b+ul3&S&g#p5F#Cr<*qP0J!lKViSkjKa`)S6PHT4_kEVlG7sf9mIE!n90;Ga#m zDNFjU2mWQPf9n63Wp90XR+l-+Z?i4K`SoQBGQ*m)F0HuoJfQH|Rx{Re*4*k7PmcWc zxvCh?#LURRxY*ag8ytbM!YoV%3=Yrt-hG+jIQyf8+q?<^%;eBSV~e{?&V$dI3qA ziOk<;ojG|Rw!vKC#I4SwD_JLsUp&FP=~`xyU)?*_k`$9|mMJl(J|{Ny`A)d;;D_s! zCL1Qt6}#MJeSd7<8`Jsy{pqBx&tdxO#f{yB8HCp<{aShJ#NW&1sqZ4Dlsx(BzfgVI zhKt;rZ-{NXo}R3Gr~0A9&0h7l(n{~|n+ZNT!N7O?<*^+%yzNaL|Noih>0jGt@19{S z-t5A^epw{fN~6<}|L(@;bt~;?oFF!li={H@zcpC^Ry|n!Cxzsm>0;Z==`%5~%Hn?y~*Z!27Na9QbN29Ig$@$X( Db~{is literal 0 HcmV?d00001 diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index 1b0316eb8..d368ef4f4 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -2644,6 +2644,10 @@ X509 File parse (trailing spaces, OK) depends_on:MBEDTLS_ECDSA_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA256_C:MBEDTLS_RSA_C x509parse_crt_file:"data_files/server7_trailing_space.crt":0 +X509 File parse (Algorithm Params Tag mismatch) +depends_on:MBEDTLS_SHA256_C:MBEDTLS_RSA_C +x509parse_crt_file:"data_files/cli-rsa-sha256-badalg.crt.der":MBEDTLS_ERR_X509_SIG_MISMATCH + X509 Get time (UTC no issues) depends_on:MBEDTLS_X509_USE_C x509_get_time:MBEDTLS_ASN1_UTC_TIME:"500101000000Z":0:1950:1:1:0:0:0