Merge remote-tracking branch 'origin/misc-2.1' into mbedtls-2.1
diff --git a/ChangeLog b/ChangeLog
index 4ea2592..3a21a9a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -5,6 +5,14 @@
Bugfix
* Fix over-restricive length limit in GCM. Found by Andreas-N. #362
+Changes
+ * To avoid dropping an entire DTLS datagram if a single record in a datagram
+ is invalid, we now only drop the record and look at subsequent records (if
+ any are present) in the same datagram to avoid interoperability issues.
+ Previously the library was dropping the entire datagram, Where a record is
+ unexpected, the function mbedtls_ssl_read_record() will now return
+ MBEDTLS_ERR_SSL_UNEXPECTED_RECORD.
+
= mbed TLS 2.1.3 released 2015-11-04
Security
diff --git a/include/mbedtls/error.h b/include/mbedtls/error.h
index a182713..5e549f6 100644
--- a/include/mbedtls/error.h
+++ b/include/mbedtls/error.h
@@ -79,7 +79,7 @@
* ECP 4 8 (Started from top)
* MD 5 4
* CIPHER 6 6
- * SSL 6 16 (Started from top)
+ * SSL 6 17 (Started from top)
* SSL 7 31
*
* Module dependent error code (5 bits 0x.00.-0x.F8.)
diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h
index 7343047..73e96dd 100644
--- a/include/mbedtls/ssl.h
+++ b/include/mbedtls/ssl.h
@@ -106,6 +106,7 @@
#define MBEDTLS_ERR_SSL_WANT_WRITE -0x6880 /**< Connection requires a write call. */
#define MBEDTLS_ERR_SSL_TIMEOUT -0x6800 /**< The operation timed out. */
#define MBEDTLS_ERR_SSL_CLIENT_RECONNECT -0x6780 /**< The client initiated a reconnect from the same port. */
+#define MBEDTLS_ERR_SSL_UNEXPECTED_RECORD -0x6700 /**< Record header looks valid but is not expected. */
/*
* Various constants
diff --git a/library/error.c b/library/error.c
index a1cf83a..debda1d 100644
--- a/library/error.c
+++ b/library/error.c
@@ -430,6 +430,8 @@
mbedtls_snprintf( buf, buflen, "SSL - The operation timed out" );
if( use_ret == -(MBEDTLS_ERR_SSL_CLIENT_RECONNECT) )
mbedtls_snprintf( buf, buflen, "SSL - The client initiated a reconnect from the same port" );
+ if( use_ret == -(MBEDTLS_ERR_SSL_UNEXPECTED_RECORD) )
+ mbedtls_snprintf( buf, buflen, "SSL - Record header looks valid but is not expected" );
#endif /* MBEDTLS_SSL_TLS_C */
#if defined(MBEDTLS_X509_USE_C) || defined(MBEDTLS_X509_CREATE_C)
diff --git a/library/ssl_tls.c b/library/ssl_tls.c
index 04d6981..ddc7bdc 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -3455,6 +3455,18 @@
* uint16 epoch; // DTLS only
* uint48 sequence_number; // DTLS only
* uint16 length;
+ *
+ * Return 0 if header looks sane (and, for DTLS, the record is expected)
+ * MBEDTLS_ERR_SSL_INVALID_RECORD if the header looks bad,
+ * MBEDTLS_ERR_SSL_UNEXPECTED_RECORD (DTLS only) if sane but unexpected.
+ *
+ * With DTLS, mbedtls_ssl_read_record() will:
+ * 1. proceed with the record if this function returns 0
+ * 2. drop only the current record if this function returns UNEXPECTED_RECORD
+ * 3. return CLIENT_RECONNECT if this function returns that value
+ * 4. drop the whole datagram if this function returns anything else.
+ * Point 2 is needed when the peer is resending, and we have already received
+ * the first record from a datagram but are still waiting for the others.
*/
static int ssl_parse_record_header( mbedtls_ssl_context *ssl )
{
@@ -3490,34 +3502,6 @@
return( MBEDTLS_ERR_SSL_INVALID_RECORD );
}
-#if defined(MBEDTLS_SSL_PROTO_DTLS)
- if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM )
- {
- /* Drop unexpected ChangeCipherSpec messages */
- if( ssl->in_msgtype == MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC &&
- ssl->state != MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC &&
- ssl->state != MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC )
- {
- MBEDTLS_SSL_DEBUG_MSG( 1, ( "dropping unexpected ChangeCipherSpec" ) );
- return( MBEDTLS_ERR_SSL_INVALID_RECORD );
- }
-
- /* Drop unexpected ApplicationData records,
- * except at the beginning of renegotiations */
- if( ssl->in_msgtype == MBEDTLS_SSL_MSG_APPLICATION_DATA &&
- ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER
-#if defined(MBEDTLS_SSL_RENEGOTIATION)
- && ! ( ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_IN_PROGRESS &&
- ssl->state == MBEDTLS_SSL_SERVER_HELLO )
-#endif
- )
- {
- MBEDTLS_SSL_DEBUG_MSG( 1, ( "dropping unexpected ApplicationData" ) );
- return( MBEDTLS_ERR_SSL_INVALID_RECORD );
- }
- }
-#endif
-
/* Check version */
if( major_ver != ssl->major_ver )
{
@@ -3531,53 +3515,6 @@
return( MBEDTLS_ERR_SSL_INVALID_RECORD );
}
- /* Check epoch (and sequence number) with DTLS */
-#if defined(MBEDTLS_SSL_PROTO_DTLS)
- if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM )
- {
- unsigned int rec_epoch = ( ssl->in_ctr[0] << 8 ) | ssl->in_ctr[1];
-
- if( rec_epoch != ssl->in_epoch )
- {
- MBEDTLS_SSL_DEBUG_MSG( 1, ( "record from another epoch: "
- "expected %d, received %d",
- ssl->in_epoch, rec_epoch ) );
-
-#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && defined(MBEDTLS_SSL_SRV_C)
- /*
- * Check for an epoch 0 ClientHello. We can't use in_msg here to
- * access the first byte of record content (handshake type), as we
- * have an active transform (possibly iv_len != 0), so use the
- * fact that the record header len is 13 instead.
- */
- if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER &&
- ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER &&
- rec_epoch == 0 &&
- ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE &&
- ssl->in_left > 13 &&
- ssl->in_buf[13] == MBEDTLS_SSL_HS_CLIENT_HELLO )
- {
- MBEDTLS_SSL_DEBUG_MSG( 1, ( "possible client reconnect "
- "from the same port" ) );
- return( ssl_handle_possible_reconnect( ssl ) );
- }
- else
-#endif /* MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE && MBEDTLS_SSL_SRV_C */
- return( MBEDTLS_ERR_SSL_INVALID_RECORD );
- }
-
-#if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY)
- /* Replay detection only works for the current epoch */
- if( rec_epoch == ssl->in_epoch &&
- mbedtls_ssl_dtls_replay_check( ssl ) != 0 )
- {
- MBEDTLS_SSL_DEBUG_MSG( 1, ( "replayed record" ) );
- return( MBEDTLS_ERR_SSL_INVALID_RECORD );
- }
-#endif
- }
-#endif /* MBEDTLS_SSL_PROTO_DTLS */
-
/* Check length against the size of our buffer */
if( ssl->in_msglen > MBEDTLS_SSL_BUFFER_LEN
- (size_t)( ssl->in_msg - ssl->in_buf ) )
@@ -3627,6 +3564,82 @@
#endif
}
+ /*
+ * DTLS-related tests done last, because most of them may result in
+ * silently dropping the record (but not the whole datagram), and we only
+ * want to consider that after ensuring that the "basic" fields (type,
+ * version, length) are sane.
+ */
+#if defined(MBEDTLS_SSL_PROTO_DTLS)
+ if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM )
+ {
+ unsigned int rec_epoch = ( ssl->in_ctr[0] << 8 ) | ssl->in_ctr[1];
+
+ /* Drop unexpected ChangeCipherSpec messages */
+ if( ssl->in_msgtype == MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC &&
+ ssl->state != MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC &&
+ ssl->state != MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC )
+ {
+ MBEDTLS_SSL_DEBUG_MSG( 1, ( "dropping unexpected ChangeCipherSpec" ) );
+ return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD );
+ }
+
+ /* Drop unexpected ApplicationData records,
+ * except at the beginning of renegotiations */
+ if( ssl->in_msgtype == MBEDTLS_SSL_MSG_APPLICATION_DATA &&
+ ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER
+#if defined(MBEDTLS_SSL_RENEGOTIATION)
+ && ! ( ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_IN_PROGRESS &&
+ ssl->state == MBEDTLS_SSL_SERVER_HELLO )
+#endif
+ )
+ {
+ MBEDTLS_SSL_DEBUG_MSG( 1, ( "dropping unexpected ApplicationData" ) );
+ return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD );
+ }
+
+ /* Check epoch (and sequence number) with DTLS */
+ if( rec_epoch != ssl->in_epoch )
+ {
+ MBEDTLS_SSL_DEBUG_MSG( 1, ( "record from another epoch: "
+ "expected %d, received %d",
+ ssl->in_epoch, rec_epoch ) );
+
+#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && defined(MBEDTLS_SSL_SRV_C)
+ /*
+ * Check for an epoch 0 ClientHello. We can't use in_msg here to
+ * access the first byte of record content (handshake type), as we
+ * have an active transform (possibly iv_len != 0), so use the
+ * fact that the record header len is 13 instead.
+ */
+ if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER &&
+ ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER &&
+ rec_epoch == 0 &&
+ ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE &&
+ ssl->in_left > 13 &&
+ ssl->in_buf[13] == MBEDTLS_SSL_HS_CLIENT_HELLO )
+ {
+ MBEDTLS_SSL_DEBUG_MSG( 1, ( "possible client reconnect "
+ "from the same port" ) );
+ return( ssl_handle_possible_reconnect( ssl ) );
+ }
+ else
+#endif /* MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE && MBEDTLS_SSL_SRV_C */
+ return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD );
+ }
+
+#if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY)
+ /* Replay detection only works for the current epoch */
+ if( rec_epoch == ssl->in_epoch &&
+ mbedtls_ssl_dtls_replay_check( ssl ) != 0 )
+ {
+ MBEDTLS_SSL_DEBUG_MSG( 1, ( "replayed record" ) );
+ return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD );
+ }
+#endif
+ }
+#endif /* MBEDTLS_SSL_PROTO_DTLS */
+
return( 0 );
}
@@ -3752,13 +3765,26 @@
if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM &&
ret != MBEDTLS_ERR_SSL_CLIENT_RECONNECT )
{
- /* Ignore bad record and get next one; drop the whole datagram
- * since current header cannot be trusted to find the next record
- * in current datagram */
- ssl->next_record_offset = 0;
- ssl->in_left = 0;
+ if( ret == MBEDTLS_ERR_SSL_UNEXPECTED_RECORD )
+ {
+ /* Skip unexpected record (but not whole datagram) */
+ ssl->next_record_offset = ssl->in_msglen
+ + mbedtls_ssl_hdr_len( ssl );
- MBEDTLS_SSL_DEBUG_MSG( 1, ( "discarding invalid record (header)" ) );
+ MBEDTLS_SSL_DEBUG_MSG( 1, ( "discarding unexpected record "
+ "(header)" ) );
+ }
+ else
+ {
+ /* Skip invalid record and the rest of the datagram */
+ ssl->next_record_offset = 0;
+ ssl->in_left = 0;
+
+ MBEDTLS_SSL_DEBUG_MSG( 1, ( "discarding invalid record "
+ "(header)" ) );
+ }
+
+ /* Get next record */
goto read_record_header;
}
#endif