From 5af02ce55b90d94a477446fce426cded8f7b431d Mon Sep 17 00:00:00 2001
From: Paul Elliott <paul.elliott@arm.com>
Date: Wed, 2 Dec 2020 15:56:03 +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 <paul.elliott@arm.com>
---
.../x509-add-tag-check-to-algorithm-params | 11 +++++++++++
library/x509_crt.c | 1 +
tests/data_files/Makefile | 8 ++++++++
tests/data_files/cli-rsa-sha256-badalg.crt.der | Bin 0 -> 905 bytes
tests/suites/test_suite_x509parse.data | 4 ++++
5 files changed, 24 insertions(+)
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 acda1f22c..2334a35dc 100644
--- a/library/x509_crt.c
+++ b/library/x509_crt.c
@@ -929,6 +929,7 @@ static int x509_crt_parse_der_core( mbedtls_x509_crt *crt, const unsigned char *
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 61edad1d4..d76aa5d7f 100644
--- a/tests/data_files/Makefile
+++ b/tests/data_files/Makefile
@@ -129,6 +129,14 @@ cli-rsa-sha256.crt: $(cli_crt_key_file_rsa) test-ca-sha256.crt cli-rsa.csr
$(OPENSSL) x509 -req -extfile $(cli_crt_extensions_file) -extensions cli-rsa -CA test-ca-sha256.crt -CAkey $(test_ca_key_file_rsa) -passin "pass:$(test_ca_pwd_rsa)" -set_serial 4 -days 3653 -sha256 -in cli-rsa.csr -out $@
all_final += cli-rsa-sha256.crt
+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-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
+
server2-rsa.csr: server2.key
$(OPENSSL) req -new -key server2.key -passin "pass:$(test_ca_pwd_rsa)" -subj "/C=NL/O=PolarSSL/CN=localhost" -out $@
all_intermediate += server2-rsa.csr
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..be75255ac3f2a326cab22bc556736410a378c851
GIT binary patch
literal 905
zcmXqLVs15PV#-~>%*4pV#K>a6%f_kI=F#?@mywa1mBGN;klTQhjX9KsO_<5g$57CK
zAH?C};RwjjNh}Hu_A!(+5C;h{^9aC%6hcyqOB9?P4dldmjSLKp42%qoje&HOI4_7Y
zFotpsYzP`IWgr1DTo7)!b53SzUWtN{K@+1AvTGSx8JL?G`5A!XTue=jj0`7Aj-1)~
zb8F>G*P9{<8)|n?{*)~@-*_!=f0s>I>eD+eH_xQCvS0n7zQN1%Lh?MWJn8MXKGdy#
z$0IPg%~QhU^9rp?;a~Nx=$C7@B>FZ5^e><4QSdd-chgnj!<VGqK68HMB=WlC)q%J2
zSHk^PeqQnR(u0Erg$70jS1*6#xDe*=*7qs?r)~Pg4g)=>P4{bmKjkU^|Hkk@MgRUo
z^{3Y6C|>Km*`3vw|6ukHfv_{1`-DZGnXsfCfA`ajFKg;I&RJ~fUs4NyoLaI`^}#=z
zZc~=@T@U=rTL0AlG0Wci@~kd%lHX=qhV$#o7G#DsXI)xx<#|Biv#n;V<*d2YC!QSn
z>vL5xo{5=}fpKx;B!k9&16g1w%JQ*@v4|8dWGF6j&<`n#GyW6QzvtuKY&S)NWRSEn
zON>EOgUFVsCrdZqyA<?o=hbCX`Cr@ZIsW(mLi;7Q6eWcwMh0*)k`-oQGGIWCHf~N(
zv@tTUZqt9rZ6UWvW$WscZPHI$xg08Q9oku(Bo(sWU#70M&~n$s^H%K9v7R+EE<K*v
z?sP#qOnlB%6Ytx0fsY>T4m=R#U9I$`qNV$ed)#!EJND)`e3wYQy}%o$>DM3haF<=(
zG8L=RI1`Dr;&0u;bPB$e96ETRe6ES4V@my*y0A0JL0gsON<Q9s(s=JrY5SU+M(%Ml
zPbfy%Fc(gkJ+p{sY1W*{`>I~*C9;@ymxo9Osc#S5%Xm{`=8NYwQ{+Ccm~rLoj1qx8
zcfOV6^L#MWyYO;_#~RP41})zLwXDBcE5tp_^8eGqEaz0^^utlt%T49g;aLjWS<QN#
Q{Phk`XF30q|0Q4m0On>~kN^Mx
literal 0
HcmV?d00001
diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data
index b87859ec6..97eeac87f 100644
--- a/tests/suites/test_suite_x509parse.data
+++ b/tests/suites/test_suite_x509parse.data
@@ -1864,6 +1864,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