Merge pull request #586 from ARMmbed/remove_peer_crt_after_handshake_no_digest-baremetal
[Baremetal] Don't store peer CRT digest if renegotiation is disabled
diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h
index 9a36009..eeb03e1 100644
--- a/include/mbedtls/ssl.h
+++ b/include/mbedtls/ssl.h
@@ -856,13 +856,13 @@
#if defined(MBEDTLS_X509_CRT_PARSE_C)
#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE)
mbedtls_x509_crt *peer_cert; /*!< peer X.509 cert chain */
-#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#elif defined(MBEDTLS_SSL_RENEGOTIATION)
/*! The digest of the peer's end-CRT. This must be kept to detect CRT
* changes during renegotiation, mitigating the triple handshake attack. */
unsigned char *peer_cert_digest;
size_t peer_cert_digest_len;
mbedtls_md_type_t peer_cert_digest_type;
-#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */
#endif /* MBEDTLS_X509_CRT_PARSE_C */
uint32_t verify_result; /*!< verification result */
diff --git a/library/ssl_cli.c b/library/ssl_cli.c
index e5185d4..174e8b1 100644
--- a/library/ssl_cli.c
+++ b/library/ssl_cli.c
@@ -2288,7 +2288,7 @@
int ret;
size_t len_bytes = ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ? 0 : 2;
unsigned char *p = ssl->handshake->premaster + pms_offset;
- mbedtls_pk_context * peer_pk;
+ mbedtls_pk_context *peer_pk = NULL;
if( offset + len_bytes > MBEDTLS_SSL_OUT_CONTENT_LEN )
{
@@ -2315,16 +2315,23 @@
ssl->handshake->pmslen = 48;
#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE)
- peer_pk = &ssl->handshake->peer_pubkey;
+ /* Because the peer CRT pubkey is embedded into the handshake
+ * params currently, and there's no 'is_init' functions for PK
+ * contexts, we need to break the abstraction and peek into
+ * the PK context to see if it has been initialized. */
+ if( ssl->handshake->peer_pubkey.pk_info != NULL )
+ peer_pk = &ssl->handshake->peer_pubkey;
#else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
- if( ssl->session_negotiate->peer_cert == NULL )
+ if( ssl->session_negotiate->peer_cert != NULL )
+ peer_pk = &ssl->session_negotiate->peer_cert->pk;
+#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+
+ if( peer_pk == NULL )
{
/* Should never happen */
MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) );
return( MBEDTLS_ERR_SSL_INTERNAL_ERROR );
}
- peer_pk = &ssl->session_negotiate->peer_cert->pk;
-#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
/*
* Now write it out, encrypted
diff --git a/library/ssl_srv.c b/library/ssl_srv.c
index 06fe3ee..a8821f3 100644
--- a/library/ssl_srv.c
+++ b/library/ssl_srv.c
@@ -4161,7 +4161,7 @@
mbedtls_md_type_t md_alg;
const mbedtls_ssl_ciphersuite_t *ciphersuite_info =
ssl->handshake->ciphersuite_info;
- mbedtls_pk_context * peer_pk;
+ mbedtls_pk_context *peer_pk = NULL;
MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate verify" ) );
@@ -4172,21 +4172,30 @@
return( 0 );
}
-#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE)
- if( ssl->session_negotiate->peer_cert == NULL )
+ /* Skip if we haven't received a certificate from the client.
+ * If MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is set, this can be
+ * inferred from the setting of mbedtls_ssl_session::peer_cert.
+ * If MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is not set, it can
+ * be inferred from whether we've held back the peer CRT's
+ * public key in mbedtls_ssl_handshake_params::peer_pubkey. */
+#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE)
+ /* Because the peer CRT pubkey is embedded into the handshake
+ * params currently, and there's no 'is_init' functions for PK
+ * contexts, we need to break the abstraction and peek into
+ * the PK context to see if it has been initialized. */
+ if( ssl->handshake->peer_pubkey.pk_info != NULL )
+ peer_pk = &ssl->handshake->peer_pubkey;
+#else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+ if( ssl->session_negotiate->peer_cert != NULL )
+ peer_pk = &ssl->session_negotiate->peer_cert->pk;
+#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+
+ if( peer_pk == NULL )
{
MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) );
ssl->state++;
return( 0 );
}
-#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
- if( ssl->session_negotiate->peer_cert_digest == NULL )
- {
- MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) );
- ssl->state++;
- return( 0 );
- }
-#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
/* Read the message without adding it to the checksum */
ret = mbedtls_ssl_read_record( ssl, 0 /* no checksum update */ );
@@ -4208,17 +4217,6 @@
i = mbedtls_ssl_hs_hdr_len( ssl );
-#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE)
- peer_pk = &ssl->handshake->peer_pubkey;
-#else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
- if( ssl->session_negotiate->peer_cert == NULL )
- {
- /* Should never happen */
- return( MBEDTLS_ERR_SSL_INTERNAL_ERROR );
- }
- peer_pk = &ssl->session_negotiate->peer_cert->pk;
-#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
-
/*
* struct {
* SignatureAndHashAlgorithm algorithm; -- TLS 1.2 only
diff --git a/library/ssl_tls.c b/library/ssl_tls.c
index 84a9b3b..3257732 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -395,7 +395,7 @@
return( ret );
}
}
-#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#elif defined(MBEDTLS_SSL_RENEGOTIATION)
if( src->peer_cert_digest != NULL )
{
dst->peer_cert_digest =
@@ -408,7 +408,7 @@
dst->peer_cert_digest_type = src->peer_cert_digest_type;
dst->peer_cert_digest_len = src->peer_cert_digest_len;
}
-#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */
#endif /* MBEDTLS_X509_CRT_PARSE_C */
@@ -5999,7 +5999,7 @@
mbedtls_free( session->peer_cert );
session->peer_cert = NULL;
}
-#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#elif defined(MBEDTLS_SSL_RENEGOTIATION)
if( session->peer_cert_digest != NULL )
{
/* Zeroization is not necessary. */
@@ -6008,7 +6008,9 @@
session->peer_cert_digest_type = MBEDTLS_MD_NONE;
session->peer_cert_digest_len = 0;
}
-#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#else
+ ((void) session);
+#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */
}
#endif /* MBEDTLS_X509_CRT_PARSE_C */
@@ -6184,7 +6186,7 @@
return( memcmp( peer_crt->raw.p, crt_buf, crt_buf_len ) );
}
-#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#elif defined(MBEDTLS_SSL_RENEGOTIATION)
static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl,
unsigned char *crt_buf,
size_t crt_buf_len )
@@ -6212,7 +6214,7 @@
return( memcmp( tmp_digest, peer_cert_digest, digest_len ) );
}
-#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */
#endif /* MBEDTLS_SSL_RENEGOTIATION && MBEDTLS_SSL_CLI_C */
/*
@@ -6594,6 +6596,8 @@
}
#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE)
+
+#if defined(MBEDTLS_SSL_RENEGOTIATION)
static int ssl_remember_peer_crt_digest( mbedtls_ssl_context *ssl,
unsigned char *start, size_t len )
{
@@ -6624,6 +6628,7 @@
return( ret );
}
+#endif /* MBEDTLS_SSL_RENEGOTIATION */
static int ssl_remember_peer_pubkey( mbedtls_ssl_context *ssl,
unsigned char *start, size_t len )
@@ -6738,16 +6743,21 @@
#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE)
{
- unsigned char *crt_start, *pk_start;
- size_t crt_len, pk_len;
+ size_t pk_len;
+ unsigned char *pk_start;
/* We parse the CRT chain without copying, so
* these pointers point into the input buffer,
* and are hence still valid after freeing the
* CRT chain. */
+#if defined(MBEDTLS_SSL_RENEGOTIATION)
+ unsigned char *crt_start;
+ size_t crt_len;
+
crt_start = chain->raw.p;
crt_len = chain->raw.len;
+#endif /* MBEDTLS_SSL_RENEGOTIATION */
pk_start = chain->pk_raw.p;
pk_len = chain->pk_raw.len;
@@ -6758,9 +6768,11 @@
mbedtls_free( chain );
chain = NULL;
+#if defined(MBEDTLS_SSL_RENEGOTIATION)
ret = ssl_remember_peer_crt_digest( ssl, crt_start, crt_len );
if( ret != 0 )
goto exit;
+#endif /* MBEDTLS_SSL_RENEGOTIATION */
ret = ssl_remember_peer_pubkey( ssl, pk_start, pk_len );
if( ret != 0 )
@@ -9280,7 +9292,7 @@
}
}
-#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#elif defined(MBEDTLS_SSL_RENEGOTIATION)
/* Digest of peer certificate */
if( session->peer_cert_digest != NULL )
{
@@ -9303,7 +9315,7 @@
*p++ = 0;
}
}
-#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */
#endif /* MBEDTLS_X509_CRT_PARSE_C */
/*
@@ -9448,9 +9460,9 @@
#if defined(MBEDTLS_X509_CRT_PARSE_C)
#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE)
session->peer_cert = NULL;
-#else
+#elif defined(MBEDTLS_SSL_RENEGOTIATION)
session->peer_cert_digest = NULL;
-#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */
#endif
#if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C)
session->ticket = NULL;
@@ -9496,7 +9508,7 @@
p += cert_len;
}
-#else /* defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#elif defined(MBEDTLS_SSL_RENEGOTIATION)
/* Deserialize CRT digest from the end of the ticket. */
if( 2 > (size_t)( end - p ) )
return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA );
@@ -9525,7 +9537,7 @@
session->peer_cert_digest_len );
p += session->peer_cert_digest_len;
}
-#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */
#endif /* MBEDTLS_X509_CRT_PARSE_C */
/*
diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh
index 83e33fd..2f1a1b5 100755
--- a/tests/scripts/all.sh
+++ b/tests/scripts/all.sh
@@ -901,6 +901,23 @@
if_build_succeeded tests/compat.sh
}
+component_test_asan_remove_peer_certificate_no_renego () {
+ msg "build: default config with MBEDTLS_SSL_KEEP_PEER_CERTIFICATE and MBEDTLS_SSL_RENEGOTIATION disabled (ASan build)"
+ scripts/config.pl unset MBEDTLS_SSL_KEEP_PEER_CERTIFICATE
+ scripts/config.pl unset MBEDTLS_SSL_RENEGOTIATION
+ CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan .
+ make
+
+ msg "test: !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE"
+ make test
+
+ msg "test: ssl-opt.sh, !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE + !MBEDTLS_SSL_RENEGOTIATION"
+ if_build_succeeded tests/ssl-opt.sh
+
+ msg "test: compat.sh, !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE + !MBEDTLS_SSL_RENEGOTIATION"
+ if_build_succeeded tests/compat.sh
+}
+
component_test_no_max_fragment_length_small_ssl_out_content_len () {
msg "build: no MFL extension, small SSL_OUT_CONTENT_LEN (ASan build)"
scripts/config.pl unset MBEDTLS_SSL_MAX_FRAGMENT_LENGTH
diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function
index fb77102..8271b23 100644
--- a/tests/suites/test_suite_ssl.function
+++ b/tests/suites/test_suite_ssl.function
@@ -296,7 +296,14 @@
if( ret != 0 )
return( ret );
-#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE)
+#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE)
+ /* Move temporary CRT. */
+ session->peer_cert = mbedtls_calloc( 1, sizeof( *session->peer_cert ) );
+ if( session->peer_cert == NULL )
+ return( -1 );
+ *session->peer_cert = tmp_crt;
+ memset( &tmp_crt, 0, sizeof( tmp_crt ) );
+#elif defined(MBEDTLS_SSL_RENEGOTIATION)
/* Calculate digest of temporary CRT. */
session->peer_cert_digest =
mbedtls_calloc( 1, MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN );
@@ -312,14 +319,7 @@
MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE;
session->peer_cert_digest_len =
MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN;
-#else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
- /* Move temporary CRT. */
- session->peer_cert = mbedtls_calloc( 1, sizeof( *session->peer_cert ) );
- if( session->peer_cert == NULL )
- return( -1 );
- *session->peer_cert = tmp_crt;
- memset( &tmp_crt, 0, sizeof( tmp_crt ) );
-#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */
mbedtls_x509_crt_free( &tmp_crt );
}
@@ -718,7 +718,7 @@
restored.peer_cert->raw.p,
original.peer_cert->raw.len ) == 0 );
}
-#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#elif defined(MBEDTLS_SSL_RENEGOTIATION)
TEST_ASSERT( original.peer_cert_digest_type ==
restored.peer_cert_digest_type );
TEST_ASSERT( original.peer_cert_digest_len ==
@@ -731,7 +731,7 @@
restored.peer_cert_digest,
original.peer_cert_digest_len ) == 0 );
}
-#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */
#endif /* MBEDTLS_X509_CRT_PARSE_C */
TEST_ASSERT( original.verify_result == restored.verify_result );