Remove CRT digest from SSL session if !RENEGO + !KEEP_PEER_CERT
If `MBEDTLS_SSL_KEEP_PEER_CERTIFICATE` is not set, `mbedtls_ssl_session`
contains the digest of the peer's certificate for the sole purpose of
detecting a CRT change on renegotiation. Hence, it is not needed if
renegotiation is disabled.
This commit removes the `peer_cert_digest` fields (and friends) from
`mbedtls_ssl_session` if
`!MBEDTLS_SSL_KEEP_PEER_CERTIFICATE + !MBEDTLS_SSL_RENEGOTIATION`,
which is a sensible configuration for constrained devices.
Apart from straightforward replacements of
`if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE)`
by
`if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) && \
defined(MBEDTLS_SSL_RENEGOTIATION)`,
there's one notable change: On the server-side, the CertificateVerify
parsing function is a no-op if the client hasn't sent a certificate.
So far, this was determined by either looking at the peer CRT or the
peer CRT digest in the SSL session structure (depending on the setting
of `MBEDTLS_SSL_KEEP_PEER_CERTIFICATE`), which now no longer works if
`MBEDTLS_SSL_KEEP_PEER_CERTIFICATE` is unset. Instead, this function
now checks whether the temporary copy of the peer's public key within
the handshake structure is initialized or not (which is also a
beneficial simplification in its own right, because the pubkey is
all the function needs anyway).
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 3642162..5ff66eb 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 */
@@ -5994,7 +5994,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. */
@@ -6003,7 +6003,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 */
@@ -6179,7 +6181,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 )
@@ -6207,7 +6209,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 */
/*
@@ -6589,6 +6591,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 )
{
@@ -6619,6 +6623,7 @@
return( ret );
}
+#endif /* MBEDTLS_SSL_RENEGOTIATION */
static int ssl_remember_peer_pubkey( mbedtls_ssl_context *ssl,
unsigned char *start, size_t len )
@@ -6733,16 +6738,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;
@@ -6753,9 +6763,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 )
@@ -9275,7 +9287,7 @@
}
}
-#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#elif defined(MBEDTLS_SSL_RENEGOTIATION)
/* Digest of peer certificate */
if( session->peer_cert_digest != NULL )
{
@@ -9298,7 +9310,7 @@
*p++ = 0;
}
}
-#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */
+#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE && MBEDTLS_SSL_RENEGOTIATION */
#endif /* MBEDTLS_X509_CRT_PARSE_C */
/*
@@ -9443,9 +9455,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;
@@ -9491,7 +9503,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 );
@@ -9520,7 +9532,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/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function
index b59c204..6acbbef 100644
--- a/tests/suites/test_suite_ssl.function
+++ b/tests/suites/test_suite_ssl.function
@@ -295,7 +295,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 );
@@ -311,14 +318,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 );
}
@@ -717,7 +717,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 ==
@@ -730,7 +730,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 );