From 0b2423403c1233c0848530dc06cf1ba36707e484 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 17 Feb 2016 10:11:21 +0000 Subject: [PATCH 1/3] x509: trailing bytes in DER: add integration tests --- tests/data_files/server5-der0.crt | Bin 0 -> 547 bytes tests/data_files/server5-der1a.crt | Bin 0 -> 548 bytes tests/data_files/server5-der1b.crt | Bin 0 -> 548 bytes tests/data_files/server5-der2.crt | Bin 0 -> 549 bytes tests/data_files/server5-der4.crt | Bin 0 -> 551 bytes tests/data_files/server5-der8.crt | Bin 0 -> 555 bytes tests/data_files/server5-der9.crt | Bin 0 -> 556 bytes tests/ssl-opt.sh | 58 +++++++++++++++++++++++++++++ 8 files changed, 58 insertions(+) create mode 100644 tests/data_files/server5-der0.crt create mode 100644 tests/data_files/server5-der1a.crt create mode 100644 tests/data_files/server5-der1b.crt create mode 100644 tests/data_files/server5-der2.crt create mode 100644 tests/data_files/server5-der4.crt create mode 100644 tests/data_files/server5-der8.crt create mode 100644 tests/data_files/server5-der9.crt diff --git a/tests/data_files/server5-der0.crt b/tests/data_files/server5-der0.crt new file mode 100644 index 0000000000000000000000000000000000000000..08d8dd311b525fd51171a1019ad3194dad91580a GIT binary patch literal 547 zcmXqLVv;v#VqCg_nTe5!iILNQi;Y98&EuRc3p0~}ogudYCmVAp3!5;LpO2xS0Y8Yt zCCm|!pOaV=9PDE#V;}_*Vipz#3l$gVD1@XImngV8D>yqE$cghB8XH&|nHZXy8X1^G ziSrtPxJFQ}feAtLg$x8B=5yxcCnx4)dUQv4h>n#0YgPGb1~*69bF+nXsE> zoN`e`cE=-i|10FZtNF<`vE;&9k*(h|lp>QR`8{R0p)C0SmHs7@*jTZ>T^)zA%Xvf3 zc4_hbVmz_s?f=D%a}642fxRp%%)(^AU?2;$U6zkUj720MacTb*_M6w67;`VyonyI+c^Rf1A}TbXwv-X(%>vG8}Y%RF~v@ z<^^)(FlR6rq%s*Y%+iUuzU=m*rzyN2cKY4Do_I~z@c8QDhTWGh7Ym21ox~lx`of;? z>-3*3RMa$`y2{Ry$w1Mpe(tf@W8ACNKdH)Ee?0%uR8{2p(~r})Mm~-ctx4NCq53$Z RfPe8Z2E`yPzK_SR0ss?|s)_&r literal 0 HcmV?d00001 diff --git a/tests/data_files/server5-der1a.crt b/tests/data_files/server5-der1a.crt new file mode 100644 index 0000000000000000000000000000000000000000..015017b17db1c360392790665896ea46dc0feac2 GIT binary patch literal 548 zcmXqLVv;v#VqCg_nTe5!iILNQi;Y98&EuRc3p0~}ogudYCmVAp3!5;LpO2xS0Y8Yt zCCm|!pOaV=9PDE#V;}_*Vipz#3l$gVD1@XImngV8D>yqE$cghB8XH&|nHZXy8X1^G ziSrtPxJFQ}feAtLg$x8B=5yxcCnx4)dUQv4h>n#0YgPGb1~*69bF+nXsE> zoN`e`cE=-i|10FZtNF<`vE;&9k*(h|lp>QR`8{R0p)C0SmHs7@*jTZ>T^)zA%Xvf3 zc4_hbVmz_s?f=D%a}642fxRp%%)(^AU?2;$U6zkUj720MacTb*_M6w67;`VyonyI+c^Rf1A}TbXwv-X(%>vG8}Y%RF~v@ z<^^)(FlR6rq%s*Y%+iUuzU=m*rzyN2cKY4Do_I~z@c8QDhTWGh7Ym21ox~lx`of;? z>-3*3RMa$`y2{Ry$w1Mpe(tf@W8ACNKdH)Ee?0%uR8{2p(~r})Mm~-ctx4NCq53$Z SfPe8Z2E`yPzK_SRG5`R+9IA={ literal 0 HcmV?d00001 diff --git a/tests/data_files/server5-der1b.crt b/tests/data_files/server5-der1b.crt new file mode 100644 index 0000000000000000000000000000000000000000..6340d9e2ed9fb5e60822f52182c08cddf98f4417 GIT binary patch literal 548 zcmXqLVv;v#VqCg_nTe5!iILNQi;Y98&EuRc3p0~}ogudYCmVAp3!5;LpO2xS0Y8Yt zCCm|!pOaV=9PDE#V;}_*Vipz#3l$gVD1@XImngV8D>yqE$cghB8XH&|nHZXy8X1^G ziSrtPxJFQ}feAtLg$x8B=5yxcCnx4)dUQv4h>n#0YgPGb1~*69bF+nXsE> zoN`e`cE=-i|10FZtNF<`vE;&9k*(h|lp>QR`8{R0p)C0SmHs7@*jTZ>T^)zA%Xvf3 zc4_hbVmz_s?f=D%a}642fxRp%%)(^AU?2;$U6zkUj720MacTb*_M6w67;`VyonyI+c^Rf1A}TbXwv-X(%>vG8}Y%RF~v@ z<^^)(FlR6rq%s*Y%+iUuzU=m*rzyN2cKY4Do_I~z@c8QDhTWGh7Ym21ox~lx`of;? z>-3*3RMa$`y2{Ry$w1Mpe(tf@W8ACNKdH)Ee?0%uR8{2p(~r})Mm~-ctx4NCq53$Z SfPe8Z2E`yPzK_SR9s~fs-K$Fg literal 0 HcmV?d00001 diff --git a/tests/data_files/server5-der2.crt b/tests/data_files/server5-der2.crt new file mode 100644 index 0000000000000000000000000000000000000000..c6e320a369c20c3ee8c54d3caa1d5af0a7225206 GIT binary patch literal 549 zcmXqLVv;v#VqCg_nTe5!iILNQi;Y98&EuRc3p0~}ogudYCmVAp3!5;LpO2xS0Y8Yt zCCm|!pOaV=9PDE#V;}_*Vipz#3l$gVD1@XImngV8D>yqE$cghB8XH&|nHZXy8X1^G ziSrtPxJFQ}feAtLg$x8B=5yxcCnx4)dUQv4h>n#0YgPGb1~*69bF+nXsE> zoN`e`cE=-i|10FZtNF<`vE;&9k*(h|lp>QR`8{R0p)C0SmHs7@*jTZ>T^)zA%Xvf3 zc4_hbVmz_s?f=D%a}642fxRp%%)(^AU?2;$U6zkUj720MacTb*_M6w67;`VyonyI+c^Rf1A}TbXwv-X(%>vG8}Y%RF~v@ z<^^)(FlR6rq%s*Y%+iUuzU=m*rzyN2cKY4Do_I~z@c8QDhTWGh7Ym21ox~lx`of;? z>-3*3RMa$`y2{Ry$w1Mpe(tf@W8ACNKdH)Ee?0%uR8{2p(~r})Mm~-ctx4NCq53$Z TfPe8Z2E`yPzK_SR?&JahYB8%# literal 0 HcmV?d00001 diff --git a/tests/data_files/server5-der4.crt b/tests/data_files/server5-der4.crt new file mode 100644 index 0000000000000000000000000000000000000000..4af05cce1ed05ea02e9fac3fed3a0904b44799b0 GIT binary patch literal 551 zcmXqLVv;v#VqCg_nTe5!iILNQi;Y98&EuRc3p0~}ogudYCmVAp3!5;LpO2xS0Y8Yt zCCm|!pOaV=9PDE#V;}_*Vipz#3l$gVD1@XImngV8D>yqE$cghB8XH&|nHZXy8X1^G ziSrtPxJFQ}feAtLg$x8B=5yxcCnx4)dUQv4h>n#0YgPGb1~*69bF+nXsE> zoN`e`cE=-i|10FZtNF<`vE;&9k*(h|lp>QR`8{R0p)C0SmHs7@*jTZ>T^)zA%Xvf3 zc4_hbVmz_s?f=D%a}642fxRp%%)(^AU?2;$U6zkUj720MacTb*_M6w67;`VyonyI+c^Rf1A}TbXwv-X(%>vG8}Y%RF~v@ z<^^)(FlR6rq%s*Y%+iUuzU=m*rzyN2cKY4Do_I~z@c8QDhTWGh7Ym21ox~lx`of;? z>-3*3RMa$`y2{Ry$w1Mpe(tf@W8ACNKdH)Ee?0%uR8{2p(~r})Mm~-ctx4NCq53$Z VfPe8Z2E`yPzK_SRE*F>*4*yqE$cghB8XH&|nHZXy8X1^G ziSrtPxJFQ}feAtLg$x8B=5yxcCnx4)dUQv4h>n#0YgPGb1~*69bF+nXsE> zoN`e`cE=-i|10FZtNF<`vE;&9k*(h|lp>QR`8{R0p)C0SmHs7@*jTZ>T^)zA%Xvf3 zc4_hbVmz_s?f=D%a}642fxRp%%)(^AU?2;$U6zkUj720MacTb*_M6w67;`VyonyI+c^Rf1A}TbXwv-X(%>vG8}Y%RF~v@ z<^^)(FlR6rq%s*Y%+iUuzU=m*rzyN2cKY4Do_I~z@c8QDhTWGh7Ym21ox~lx`of;? z>-3*3RMa$`y2{Ry$w1Mpe(tf@W8ACNKdH)Ee?0%uR8{2p(~r})Mm~-ctx4NCq53$Z ZfPe8Z2E`yPzK_SRE?NFxU9D;rKLC6Lu2cX3 literal 0 HcmV?d00001 diff --git a/tests/data_files/server5-der9.crt b/tests/data_files/server5-der9.crt new file mode 100644 index 0000000000000000000000000000000000000000..4947f1f83fad41a48cee838ccf8cfdf2f2100e29 GIT binary patch literal 556 zcmXqLVv;v#VqCg_nTe5!iILNQi;Y98&EuRc3p0~}ogudYCmVAp3!5;LpO2xS0Y8Yt zCCm|!pOaV=9PDE#V;}_*Vipz#3l$gVD1@XImngV8D>yqE$cghB8XH&|nHZXy8X1^G ziSrtPxJFQ}feAtLg$x8B=5yxcCnx4)dUQv4h>n#0YgPGb1~*69bF+nXsE> zoN`e`cE=-i|10FZtNF<`vE;&9k*(h|lp>QR`8{R0p)C0SmHs7@*jTZ>T^)zA%Xvf3 zc4_hbVmz_s?f=D%a}642fxRp%%)(^AU?2;$U6zkUj720MacTb*_M6w67;`VyonyI+c^Rf1A}TbXwv-X(%>vG8}Y%RF~v@ z<^^)(FlR6rq%s*Y%+iUuzU=m*rzyN2cKY4Do_I~z@c8QDhTWGh7Ym21ox~lx`of;? z>-3*3RMa$`y2{Ry$w1Mpe(tf@W8ACNKdH)Ee?0%uR8{2p(~r})Mm~-ctx4NCq53$Z afPe8Z2E`yPzK_SRp8sKBT=suSl_mf!qOWiO literal 0 HcmV?d00001 diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index c0b6f94d6..e1ecbca33 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1554,6 +1554,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" \ From e154f95e035ed07763a95ccda25bd7074454242b Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 17 Feb 2016 14:24:28 +0000 Subject: [PATCH 2/3] x509: trailing bytes in DER: correct a unit test One of the unit test was failing, because it was testing behavior that was part of the bug. Updated the return value to the correct one --- tests/suites/test_suite_x509parse.data | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index 2f2137f54..6b04ae37e 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -755,7 +755,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 From cc0e49ddde3d8dbfcbbdc725b3d48482fb8015f1 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 17 Feb 2016 14:34:12 +0000 Subject: [PATCH 3/3] x509: trailing bytes in DER: fix bug 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. #377 --- ChangeLog | 2 ++ library/x509_crt.c | 23 +++++++++++++++-------- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/ChangeLog b/ChangeLog index e9b67908f..ed32f0b37 100644 --- a/ChangeLog +++ b/ChangeLog @@ -11,6 +11,8 @@ Bugfix * Fix issue in Makefile that prevented building using armar. #386 * Fix memory leak that occured only when ECJPAKE was enabled and ECDHE and ECDSA was disabled in config.h . The leak didn't occur by default. + * 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 6dc5ad34f..a1ce2544e 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -680,14 +680,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; /* @@ -711,6 +706,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 { */