From aa8665ac262298b4272d9a665d87fc04c563020d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 31 Jan 2019 08:57:44 +0000 Subject: [PATCH 1/5] Add a new X.509 API call for copy-less parsing of CRTs Context: The existing API `mbedtls_x509_parse_crt_der()` for parsing DER encoded X.509 CRTs unconditionally makes creates a copy of the input buffer in RAM. While this comes at the benefit of easy use, -- specifically: allowing the user to free or re-use the input buffer right after the call -- it creates a significant memory overhead, as the CRT is duplicated in memory (at least temporarily). This might not be tolerable a resource constrained device. As a remedy, this commit adds a new X.509 API call `mbedtls_x509_parse_crt_der_nocopy()` which has the same signature as `mbedtls_x509_parse_crt_der()` and almost the same semantics, with one difference: The input buffer must persist and be unmodified for the lifetime of the established instance of `mbedtls_x509_crt`, that is, until `mbedtls_x509_crt_free()` is called. --- include/mbedtls/x509_crt.h | 58 ++++++++++++++++++++++++++---- library/x509_crt.c | 73 ++++++++++++++++++++++++-------------- 2 files changed, 98 insertions(+), 33 deletions(-) diff --git a/include/mbedtls/x509_crt.h b/include/mbedtls/x509_crt.h index 670bd10d8..62c3c2e54 100644 --- a/include/mbedtls/x509_crt.h +++ b/include/mbedtls/x509_crt.h @@ -52,6 +52,8 @@ extern "C" { */ typedef struct mbedtls_x509_crt { + int own_buffer; /**< Indicates if \c raw is owned + * by the structure or not. */ mbedtls_x509_buf raw; /**< The raw certificate data (DER). */ mbedtls_x509_buf tbs; /**< The raw certificate body (DER). The part that is To Be Signed. */ @@ -220,16 +222,58 @@ extern const mbedtls_x509_crt_profile mbedtls_x509_crt_profile_suiteb; /** * \brief Parse a single DER formatted certificate and add it - * to the chained list. + * to the end of the provided chained list. * - * \param chain points to the start of the chain - * \param buf buffer holding the certificate DER data - * \param buflen size of the buffer + * \param chain The pointer to the start of the CRT chain to attach to. + * When parsing the first CRT in a chain, this should point + * to an instance of ::mbedtls_x509_crt initialized through + * mbedtls_x509_crt_init(). + * \param buf The buffer holding the DER encoded certificate. + * \param buflen The size in Bytes of \p buf. * - * \return 0 if successful, or a specific X509 or PEM error code + * \note This function makes an internal copy of the CRT buffer + * \p buf. In particular, \p buf may be destroyed or reused + * after this call returns. To avoid duplicating the CRT + * buffer (at the cost of stricter lifetime constraints), + * use mbedtls_x509_crt_parse_der_nocopy() instead. + * + * \return \c 0 if successful. + * \return A negative error code on failure. */ -int mbedtls_x509_crt_parse_der( mbedtls_x509_crt *chain, const unsigned char *buf, - size_t buflen ); +int mbedtls_x509_crt_parse_der( mbedtls_x509_crt *chain, + const unsigned char *buf, + size_t buflen ); + +/** + * \brief Parse a single DER formatted certificate and add it + * to the end of the provided chained list. This is a + * variant of mbedtls_x509_crt_parse_der() which takes + * temporary ownership of the CRT buffer until the CRT + * is destroyed. + * + * \param chain The pointer to the start of the CRT chain to attach to. + * When parsing the first CRT in a chain, this should point + * to an instance of ::mbedtls_x509_crt initialized through + * mbedtls_x509_crt_init(). + * \param buf The address of the readable buffer holding the DER encoded + * certificate to use. On success, this buffer must be + * retained and not be changed for the liftetime of the + * CRT chain \p chain, that is, until \p chain is destroyed + * through a call to mbedtls_x509_crt_free(). + * \param buflen The size in Bytes of \p buf. + * + * \note This call is functionally equivalent to + * mbedtls_x509_crt_parse_der(), but it avoids creating a + * copy of the input buffer at the cost of stronger lifetime + * constraints. This is useful in constrained environments + * where duplication of the CRT cannot be tolerated. + * + * \return \c 0 if successful. + * \return A negative error code on failure. + */ +int mbedtls_x509_crt_parse_der_nocopy( mbedtls_x509_crt *chain, + const unsigned char *buf, + size_t buflen ); /** * \brief Parse one DER-encoded or one or more concatenated PEM-encoded diff --git a/library/x509_crt.c b/library/x509_crt.c index ebd118db0..2c93311fe 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -829,8 +829,10 @@ static int x509_get_crt_ext( unsigned char **p, /* * Parse and fill a single X.509 certificate in DER format */ -static int x509_crt_parse_der_core( mbedtls_x509_crt *crt, const unsigned char *buf, - size_t buflen ) +static int x509_crt_parse_der_core( mbedtls_x509_crt *crt, + const unsigned char *buf, + size_t buflen, + int make_copy ) { int ret; size_t len; @@ -847,7 +849,7 @@ 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 ); - // Use the original buffer until we figure out actual length + /* Use the original buffer until we figure out actual length. */ p = (unsigned char*) buf; len = buflen; end = p + len; @@ -865,25 +867,26 @@ static int x509_crt_parse_der_core( mbedtls_x509_crt *crt, const unsigned char * return( MBEDTLS_ERR_X509_INVALID_FORMAT ); } - if( len > (size_t) ( end - p ) ) - { - mbedtls_x509_crt_free( crt ); - return( MBEDTLS_ERR_X509_INVALID_FORMAT + - MBEDTLS_ERR_ASN1_LENGTH_MISMATCH ); - } - 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; + crt->raw.len = crt_end - buf; + if( make_copy != 0 ) + { + /* Create and populate a new buffer for the raw field. */ + crt->raw.p = p = mbedtls_calloc( 1, crt->raw.len ); + if( crt->raw.p == NULL ) + return( MBEDTLS_ERR_X509_ALLOC_FAILED ); + + memcpy( crt->raw.p, buf, crt->raw.len ); + crt->own_buffer = 1; + + p += crt->raw.len - len; + end = crt_end = p + len; + } + else + { + crt->raw.p = (unsigned char*) buf; + crt->own_buffer = 0; + } /* * TBSCertificate ::= SEQUENCE { @@ -1086,8 +1089,10 @@ static int x509_crt_parse_der_core( mbedtls_x509_crt *crt, const unsigned char * * Parse one X.509 certificate in DER format from a buffer and add them to a * chained list */ -int mbedtls_x509_crt_parse_der( mbedtls_x509_crt *chain, const unsigned char *buf, - size_t buflen ) +static int mbedtls_x509_crt_parse_der_internal( mbedtls_x509_crt *chain, + const unsigned char *buf, + size_t buflen, + int make_copy ) { int ret; mbedtls_x509_crt *crt = chain, *prev = NULL; @@ -1119,7 +1124,7 @@ int mbedtls_x509_crt_parse_der( mbedtls_x509_crt *chain, const unsigned char *bu crt = crt->next; } - if( ( ret = x509_crt_parse_der_core( crt, buf, buflen ) ) != 0 ) + if( ( ret = x509_crt_parse_der_core( crt, buf, buflen, make_copy ) ) != 0 ) { if( prev ) prev->next = NULL; @@ -1133,11 +1138,27 @@ int mbedtls_x509_crt_parse_der( mbedtls_x509_crt *chain, const unsigned char *bu return( 0 ); } +int mbedtls_x509_crt_parse_der_nocopy( mbedtls_x509_crt *chain, + const unsigned char *buf, + size_t buflen ) +{ + return( mbedtls_x509_crt_parse_der_internal( chain, buf, buflen, 0 ) ); +} + +int mbedtls_x509_crt_parse_der( mbedtls_x509_crt *chain, + const unsigned char *buf, + size_t buflen ) +{ + return( mbedtls_x509_crt_parse_der_internal( chain, buf, buflen, 1 ) ); +} + /* * Parse one or more PEM certificates from a buffer and add them to the chained * list */ -int mbedtls_x509_crt_parse( mbedtls_x509_crt *chain, const unsigned char *buf, size_t buflen ) +int mbedtls_x509_crt_parse( mbedtls_x509_crt *chain, + const unsigned char *buf, + size_t buflen ) { #if defined(MBEDTLS_PEM_PARSE_C) int success = 0, first_error = 0, total_failed = 0; @@ -2675,7 +2696,7 @@ void mbedtls_x509_crt_free( mbedtls_x509_crt *crt ) mbedtls_free( seq_prv ); } - if( cert_cur->raw.p != NULL ) + if( cert_cur->raw.p != NULL && cert_cur->own_buffer ) { mbedtls_platform_zeroize( cert_cur->raw.p, cert_cur->raw.len ); mbedtls_free( cert_cur->raw.p ); From d58b1332761618c7724aa59267a619c9ec8b0927 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 31 Jan 2019 10:55:42 +0000 Subject: [PATCH 2/5] Add test for mbedtls_x509_parse_file() with DER encoded CRT --- tests/data_files/Makefile | 8 ++++++-- tests/data_files/server1.der | Bin 0 -> 835 bytes tests/data_files/server2.der | Bin 0 -> 827 bytes tests/data_files/test-ca.der | Bin 0 -> 837 bytes tests/suites/test_suite_x509parse.data | 12 ++++++++++++ 5 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 tests/data_files/server1.der create mode 100644 tests/data_files/server2.der create mode 100644 tests/data_files/test-ca.der diff --git a/tests/data_files/Makefile b/tests/data_files/Makefile index d023c8d0c..acf8e9bac 100644 --- a/tests/data_files/Makefile +++ b/tests/data_files/Makefile @@ -44,7 +44,9 @@ all_intermediate += test-ca.req.sha256 test-ca.crt: $(test_ca_key_file_rsa) test-ca.req.sha256 $(MBEDTLS_CERT_WRITE) is_ca=1 serial=3 request_file=test-ca.req.sha256 selfsign=1 issuer_name="C=NL,O=PolarSSL,CN=PolarSSL Test CA" issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) not_before=20110212144400 not_after=20210212144400 md=SHA1 version=3 output_file=$@ -all_final += test-ca.crt +test-ca.der: test-ca.crt + $(OPENSSL) x509 -inform PEM -in $< -outform DER -out $@ +all_final += test-ca.crt test-ca.der test-ca-sha1.crt: $(test_ca_key_file_rsa) test-ca.req.sha256 $(MBEDTLS_CERT_WRITE) is_ca=1 serial=3 request_file=test-ca.req.sha256 selfsign=1 issuer_name="C=NL,O=PolarSSL,CN=PolarSSL Test CA" issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) not_before=20110212144400 not_after=20210212144400 md=SHA1 version=3 output_file=$@ @@ -903,7 +905,9 @@ server1_all: server1.crt server1.noauthid.crt server1.crt.openssl server1.v1.crt server2.crt: server2.req.sha256 $(MBEDTLS_CERT_WRITE) request_file=server2.req.sha256 serial=2 issuer_crt=$(test_ca_crt) issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) not_before=20110212144406 not_after=20210212144406 md=SHA1 version=3 output_file=$@ -all_final += server2.crt +server2.der: server2.crt + $(OPENSSL) x509 -inform PEM -in $< -outform DER -out $@ +all_final += server2.crt server2.der server2-sha256.crt: server2.req.sha256 $(MBEDTLS_CERT_WRITE) request_file=server2.req.sha256 serial=2 issuer_crt=$(test_ca_crt) issuer_key=$(test_ca_key_file_rsa) issuer_pwd=$(test_ca_pwd_rsa) not_before=20110212144406 not_after=20210212144406 md=SHA256 version=3 output_file=$@ diff --git a/tests/data_files/server1.der b/tests/data_files/server1.der new file mode 100644 index 0000000000000000000000000000000000000000..fcf45cd7cc3886757acfafa089118900184195e4 GIT binary patch literal 835 zcmXqLVzxJEVp3ng%*4pV#K>sC%f_kI=F#?@mywZ`mBGN;klTQhjX9KsO_<5g$57CK zAH?C};RwjjNh}Hu_A!(+5C;h{^9aC%6hcyqOB9?P4dldm4Gj&942=v;OiT>SqQrTP zkhumn1PzxmkboF22sb=9wWut$NWsvciBSpJwT!F`%uS5^3_x)%rY1&4hLue6whmcW zLxa2jn!RgE)e}vO>)gNNh3kad?>fYSE`M|maGxd=nbMy9SNnn6&*FV|&rfK`77Q9{jMpRO6g)xWwK~|@ge|-*bxqp{U-d7;d zA-!0b-{D7YqiQ_Y#^7THb)uGQen!2kpEPe7YxHyB>8)FpC*8cF!giHYwX>A{?lP%< zdrrxHYg2VnUQeBU=bvMo__A9$(V1tMc8TbSsm$@ZbN0gbp!DL8x(k&5)_pNNrCV^S zlbhwX-ZKA!ym{yLMsz+3j+~blH7WH`hds{}$;Ee{zL+~z_^syc)dfO#qE2OtuMTPo z*~rAq$iTSR*T5SbfwICZOa=@FvcTY!FH%ccPe~-etutVQ*+wqDnp6m_JcfzA8T@! z2_4#?pxHEGwy}oaoy!&9XSv6&+kN)tBZ-cE+n*oVexg{pW2@8JB_Rtuw_Njkbn3>( zwTEW^|4}IR@x>9_+>%qXcl|gs)nv~rr=WGNJp6ong3V9&{W4z|@u*t-)+TEX&O+Zs z<;vTH)_U*$ac;Ll%;nGDGb%sa5T9cm5d2X3+f|2et~z@4x6W7p4 literal 0 HcmV?d00001 diff --git a/tests/data_files/server2.der b/tests/data_files/server2.der new file mode 100644 index 0000000000000000000000000000000000000000..ec03190e12610688838c1ff3f27b0fb26632885d GIT binary patch literal 827 zcmXqLVm3EuVv=9L%*4n9LZ#K0^{oYx4M zYhXgqa3KQ$h~b<$`N@en8TrK}22G4g$gX5$WngY%%5h)*oCMkXBRVcEPp4LJ#Bf=mI?25i>mBYX8qY_vPR`=`2)3- zo;e#dY8T#m)${D6%(OlK4zdJoHoLO;*TaovHzpiR+>#b#wn!~_)#{Qs_FBoN+gdl| z7u@8P(e+IG9<5sJ_JX_1Ka*!G!-R*ongr5n*M(?zr&dl}_$cx4SqD#!cNsh%yW1|g z?Z2>Nl_0ZReb@>qITs0j{?_hW-7ayDB#tHNA5ZK36?>!hvwEi{6u~JpTQa5Tn6# zCT2zk#>Kt{-r$In6=q>FU@(vc2BRz=ix`W@Qq~tKJMP?1;13Y;O<0k#-nZL%vVlBE zTA4+{K&(MzOVpF4o9|r;`nL1xvZ?&9?e-l1`yV;Lfyn|G;6OWyxxJ2_UU2VYvP>C^ zwlMF37Qv(aR?CmhF|8`!p&-)qF66_f4MC?X&PB5O2WI}etpAepdF!MbyEeW)S{9qA z`?788J*}Vi!5U6&&Be|SSmN0yh@{TX6R6q~A*poewPjJ@r0ZK`OZ6{XX)`{*9kA}v z$1A?kHoG0QwU#{cVtSe&qBBQ+*%>x()lR=@21;{cB76_ux{wzj*OU77rnu>{2Wt7#C=BTy4jaQ zS-KblwmeR%$PpA>swH^PQ)c!Nfz*Wdn{t_ve*M|65B|%3lw2`2?}6_1<>o5@#6dhp literal 0 HcmV?d00001 diff --git a/tests/data_files/test-ca.der b/tests/data_files/test-ca.der new file mode 100644 index 0000000000000000000000000000000000000000..039fb9e43004e622bd1404116f68208800005c6d GIT binary patch literal 837 zcmXqLVs&G%f_kI=F#?@mywZ`mBGN;klTQhjX9KsO_<5g$57CK zAH?C};RwjjNh}Hu_A!(+5C;h{^9aC%6hcyqOB9?P4dldm4Gj&942=v;OiT<6qQrTP zkhzo@-o&Vc>{v!t2IeM4eg=akMlPl%Mn;AM_s#!^?|v|Cu6^6RX-2g!OT`wPRs1;f z%9~fGYa}8#rYwCk`)K!lDY=;zGu!2=5A<5zw}>sMV81-?=HwSUivo|HTWk=t^3!vN z0+G`$i;B1pJ$3kL_jDQG=AUo8k`L_AWGI;vZoOhD%Y?#@dz)|CUt9XfMyvn5dcxsj z^H1-3lTf?;S&Pv=|KAa6O3cw$wp{)F_3<>lf&)+V_Wsd(_sB8yfQeqMN>S!%_l+VB z&9&)Y+P)dC{#dzW(^fs9pDp4alJeExvi&hv5v`G zd4cFBi_{d(S3G%r(&7sWPi&rja`nr@pU$^W>u+E(nm02df6-MYW=00a#Q_F>20Xy{ zkrifPHDG3B{BIx&;_TP!7QC4hV&D%ba$=(yp zSf5T{U|xOMYODB`ORhngYUdu$ke${2W5Fa@4<>WHgK<+21^?T)$E3-`#B5?uM^*ZC z6Nm2K9(kA78#MN@`c78-w( Date: Thu, 31 Jan 2019 09:15:53 +0000 Subject: [PATCH 3/5] Modify existing X.509 test for also test new copyless API The existing test `x509parse_crt()` for X.509 CRT parsing so far used the generic parsing API `mbedtls_x509_crt_parse()` capable of parsing both PEM encoded and DER encoded certficates, but was actually only used with DER encoded input data. Moreover, as the purpose of the test is the testing of the core DER X.509 parsing functionality, not the PEM vs. DER dispatch (which is now already tested in the various `x509_crt_info()` tests), the call can be replaced with a direct call to `mbedtls_x509_parse_crt_der()`. This commit does that, and further adds to the test an analogous call to the new API `mbedtls_x509_parse_crt_der_nocopy()` to test copyless parsing of X.509 certificates. --- tests/suites/test_suite_x509parse.function | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index 552c494b0..a921310c6 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -505,8 +505,22 @@ void x509parse_crt( data_t * buf, char * result_str, int result ) mbedtls_x509_crt_init( &crt ); memset( output, 0, 2000 ); + TEST_ASSERT( mbedtls_x509_crt_parse_der( &crt, buf->x, buf->len ) == ( result ) ); + if( ( result ) == 0 ) + { + res = mbedtls_x509_crt_info( (char *) output, 2000, "", &crt ); - TEST_ASSERT( mbedtls_x509_crt_parse( &crt, buf->x, buf->len ) == ( result ) ); + TEST_ASSERT( res != -1 ); + TEST_ASSERT( res != -2 ); + + TEST_ASSERT( strcmp( (char *) output, result_str ) == 0 ); + } + + mbedtls_x509_crt_free( &crt ); + mbedtls_x509_crt_init( &crt ); + memset( output, 0, 2000 ); + + TEST_ASSERT( mbedtls_x509_crt_parse_der_nocopy( &crt, buf->x, buf->len ) == ( result ) ); if( ( result ) == 0 ) { res = mbedtls_x509_crt_info( (char *) output, 2000, "", &crt ); From 80e92ad43a380e15abe51ede57738471d9353b16 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 31 Jan 2019 09:27:04 +0000 Subject: [PATCH 4/5] Adapt ChangeLog --- ChangeLog | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ChangeLog b/ChangeLog index e769dc27a..1b60a00eb 100644 --- a/ChangeLog +++ b/ChangeLog @@ -10,6 +10,10 @@ Security an error or a meaningless output from mbedtls_ecdh_get_params. In the latter case, this could expose at most 5 bits of the private key. +API Changes + * Add a new X.509 API call `mbedtls_x509_parse_der_nocopy()`. + See the Features section for more information. + Features * Add support for draft-05 of the Connection ID extension, as specified in https://tools.ietf.org/html/draft-ietf-tls-dtls-connection-id-05. @@ -24,6 +28,10 @@ Features mbedtls_ssl_session_load() to allow serializing a session, for example to store it in non-volatile storage, and later using it for TLS session resumption. + * Add a new X.509 API call `mbedtls_x509_parse_der_nocopy()` + which allows copy-less parsing of DER encoded X.509 CRTs, + at the cost of additional lifetime constraints on the input + buffer, but at the benefit of reduced RAM consumption. Bugfix * Server's RSA certificate in certs.c was SHA-1 signed. In the default From 49f83e674803ad751ebbe467bd19c20615864720 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 12 Jun 2019 12:43:19 +0100 Subject: [PATCH 5/5] Change order of ChangeLog sections --- ChangeLog | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index 1b60a00eb..c25bed4c3 100644 --- a/ChangeLog +++ b/ChangeLog @@ -10,10 +10,6 @@ Security an error or a meaningless output from mbedtls_ecdh_get_params. In the latter case, this could expose at most 5 bits of the private key. -API Changes - * Add a new X.509 API call `mbedtls_x509_parse_der_nocopy()`. - See the Features section for more information. - Features * Add support for draft-05 of the Connection ID extension, as specified in https://tools.ietf.org/html/draft-ietf-tls-dtls-connection-id-05. @@ -33,6 +29,10 @@ Features at the cost of additional lifetime constraints on the input buffer, but at the benefit of reduced RAM consumption. +API Changes + * Add a new X.509 API call `mbedtls_x509_parse_der_nocopy()`. + See the Features section for more information. + Bugfix * Server's RSA certificate in certs.c was SHA-1 signed. In the default mbedTLS configuration only SHA-2 signed certificates are accepted.