[nrf fromlist] Adding test cases and addressing comments

Upstream PR: Mbed-TLS/mbedtls#5061 Jira: NCSDK-16493 Signed-off-by: Pete Skeggs <peter.skeggs@nordicsemi.no>
plskeggs · Sep 12, 2022 · 874dd92 · 874dd92
1 parent e6b6eb0
commit 874dd92
Show file tree

Hide file tree

Showing 5 changed files with 117 additions and 45 deletions.
diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h
@@ -679,6 +679,12 @@
 #error "MBEDTLS_SSL_CID_OUT_LEN_MAX too large (max 255)"
 #endif
 
+#if !defined(MBEDTLS_SSL_DTLS_CONNECTION_ID)            &&                 \
+    defined(MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT)
+#error "MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT defined, but not all prerequsites"
+#endif
+
+
 #if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) &&   \
     !defined(MBEDTLS_SSL_PROTO_TLS1_2)
 #error "MBEDTLS_SSL_ENCRYPT_THEN_MAC defined, but not all prerequsites"

diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h
@@ -380,7 +380,13 @@
 #define MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY 16
 #endif
 
-/* \} name SECTION: Module settings */
+/*
+ * Default to standard CID mode
+ */
+#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) && \
+    !defined(MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT)
+#define MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT 0
+#endif
 
 /*
  * Length of the verify data for secure renegotiation
@@ -547,14 +553,11 @@
 #define MBEDTLS_TLS_EXT_SIG_ALG_CERT                50 /* RFC 8446 TLS 1.3 */
 #define MBEDTLS_TLS_EXT_KEY_SHARE                   51 /* RFC 8446 TLS 1.3 */
 
-/* The value of the CID extension is still TBD as of
- * draft-ietf-tls-dtls-connection-id-05
- * (https://tools.ietf.org/html/draft-ietf-tls-dtls-connection-id-05)
- * Latest CID value is here:
- * https://www.iana.org/assignments/tls-extensiontype-values/
- *       tls-extensiontype-values.xhtml#tls-extensiontype-values-1
- */
-#define MBEDTLS_TLS_EXT_CID                        54
+#if MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT == 0
+#define MBEDTLS_TLS_EXT_CID                         54 /* RFC 9146 DTLS 1.2 CID */
+#else
+#define MBEDTLS_TLS_EXT_CID                        254 /* Pre-RFC 9146 DTLS 1.2 CID */
+#endif
 
 #define MBEDTLS_TLS_EXT_ECJPAKE_KKPP               256 /* experimental */
 
@@ -1882,8 +1885,9 @@ void mbedtls_ssl_set_bio( mbedtls_ssl_context *ssl,
  * \brief             Configure the use of the Connection ID (CID)
  *                    extension in the next handshake.
  *
- *                    Reference: draft-ietf-tls-dtls-connection-id-05
+ *                    Reference: RFC 9146 (or draft-ietf-tls-dtls-connection-id-05
  *                    https://tools.ietf.org/html/draft-ietf-tls-dtls-connection-id-05
+ *                    for legacy version)
  *
  *                    The DTLS CID extension allows the reliable association of
  *                    DTLS records to DTLS connections across changes in the

diff --git a/library/ssl_msg.c b/library/ssl_msg.c
@@ -520,9 +520,10 @@ static void ssl_extract_add_data_from_record( unsigned char* add_data,
     unsigned char *cur = add_data;
     size_t ad_len_field = rec->data_len;
 
-#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID)
+#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) && \
+    MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT == 0
     const unsigned char seq_num_placeholder[] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
-#endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */
+#endif
 
 #if defined(MBEDTLS_SSL_PROTO_TLS1_3)
     if( minor_ver == MBEDTLS_SSL_MINOR_VERSION_4 )
@@ -541,25 +542,34 @@ static void ssl_extract_add_data_from_record( unsigned char* add_data,
 
 
 #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID)
+
+#if MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT == 0
         if( rec->cid_len != 0 )
         {
             // seq_num_placeholder
-            memcpy(cur, seq_num_placeholder, sizeof(seq_num_placeholder));
-            cur += sizeof(seq_num_placeholder);
+            memcpy( cur, seq_num_placeholder, sizeof(seq_num_placeholder) );
+            cur += sizeof( seq_num_placeholder );
 
-            // type
+            // tls12_cid type
             *cur = rec->type;
             cur++;
 
             // cid_length
             *cur = rec->cid_len;
             cur++;
         }
+        else
+        {
+            // epoch + sequence number
+            memcpy( cur, rec->ctr, sizeof( rec->ctr ) );
+            cur += sizeof( rec->ctr );
+        }
+#endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT == 0 */
 #else
-        memcpy( cur, rec->ctr, sizeof( rec->ctr ) );
-        cur += sizeof( rec->ctr );
+        // epoch + sequence number
+        memcpy(cur, rec->ctr, sizeof(rec->ctr));
+        cur += sizeof(rec->ctr);
 #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */
-
     }
 
     // type
@@ -570,7 +580,9 @@ static void ssl_extract_add_data_from_record( unsigned char* add_data,
     memcpy( cur, rec->ver, sizeof( rec->ver ) );
     cur += sizeof( rec->ver );
 
-#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID_LEGACY)
+#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) && \
+    MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT == 1
+
     if (rec->cid_len != 0)
     {
         // CID
@@ -581,11 +593,14 @@ static void ssl_extract_add_data_from_record( unsigned char* add_data,
         *cur = rec->cid_len;
         cur++;
 
-        MBEDTLS_PUT_UINT16_BE( ad_len_field, cur, 0 );
+        // length of inner plaintext
+        MBEDTLS_PUT_UINT16_BE(ad_len_field, cur, 0);
         cur += 2;
     }
     else
-#elif defined(MBEDTLS_SSL_DTLS_CONNECTION_ID)
+#elif defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) && \
+    MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT == 0
+
     if( rec->cid_len != 0 )
     {
         // epoch + sequence number
@@ -1072,30 +1087,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl,
         {
             unsigned char mac[MBEDTLS_SSL_MAC_ADD];
 
-            /* MAC computation (without CID):
-             *
-             * MAC(MAC_write_key, seq_num +
-             *     TLSCipherText.type +
-             *     TLSCipherText.version +
-             *     length_of( (IV +) ENC(...) ) +
-             *     IV +
-             *     ENC(content + padding + padding_length));
-             *
-             * MAC calculation (with CID):
-             *
-             *     MAC(MAC_write_key,
-             *         seq_num_placeholder +
-             *         tls12_cid +
-             *         cid_length +
-             *         tls12_cid +
-             *         DTLSCiphertext.version +
-             *         epoch +
-             *         sequence_number +
-             *         cid +
-             *         DTLSCiphertext.length +
-             *         IV +
-             *         ENC(content + padding + padding_length)
-             *        );
+            /* MAC(MAC_write_key, add_data, IV, ENC(content + padding + padding_length))
              */
 
             if( post_avail < transform->maclen)

diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh
@@ -2269,6 +2269,25 @@ component_test_CID_no_debug() {
     make test
 }
 
+component_test_variable_ssl_in_out_buffer_len_CID_legacy () {
+    msg "build: MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH and MBEDTLS_SSL_DTLS_CONNECTION_ID (legacy) enabled (ASan build)"
+    scripts/config.py set MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH
+    scripts/config.py set MBEDTLS_SSL_DTLS_CONNECTION_ID
+    scripts/config.py set MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT 1
+
+    CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan .
+    make
+
+    msg "test: MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH and MBEDTLS_SSL_DTLS_CONNECTION_ID"
+    make test
+
+    msg "test: ssl-opt.sh, MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH and MBEDTLS_SSL_DTLS_CONNECTION_ID enabled"
+    tests/ssl-opt.sh
+
+    msg "test: compat.sh, MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH and MBEDTLS_SSL_DTLS_CONNECTION_ID enabled"
+    tests/compat.sh
+}
+
 component_test_ssl_alloc_buffer_and_mfl () {
     msg "build: default config with memory buffer allocator and MFL extension"
     scripts/config.py set MBEDTLS_MEMORY_BUFFER_ALLOC_C

diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh
@@ -314,6 +314,14 @@ requires_max_content_len() {
     requires_config_value_at_least "MBEDTLS_SSL_OUT_CONTENT_LEN" $1
 }
 
+CID_MODE=$( get_config_value_or_default "MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT" )
+
+requires_cid_compat() {
+    if [ "$CID_MODE" = "0" ]; then
+        SKIP_NEXT="YES"
+    fi
+}
+
 # skip next test if GnuTLS isn't available
 requires_gnutls() {
     if [ -z "${GNUTLS_AVAILABLE:-}" ]; then
@@ -1790,6 +1798,17 @@ run_test    "Context serialization, client serializes, with CID" \
             -c "Deserializing connection..." \
             -S "Deserializing connection..."
 
+requires_config_enabled MBEDTLS_SSL_CONTEXT_SERIALIZATION
+requires_config_enabled MBEDTLS_SSL_DTLS_CONNECTION_ID
+requires_cid_compat
+run_test    "Context serialization, client serializes, with CID (legacy)" \
+            "$P_SRV dtls=1 serialize=0 exchanges=2 cid=1 cid_val=dead" \
+            "$P_CLI dtls=1 serialize=1 exchanges=2 cid=1 cid_val=beef" \
+            0 \
+            -c "Deserializing connection..." \
+            -S "Deserializing connection..."
+
+
 requires_config_enabled MBEDTLS_SSL_CONTEXT_SERIALIZATION
 run_test    "Context serialization, server serializes, CCM" \
             "$P_SRV dtls=1 serialize=1 exchanges=2" \
@@ -1823,6 +1842,17 @@ run_test    "Context serialization, server serializes, with CID" \
             -C "Deserializing connection..." \
             -s "Deserializing connection..."
 
+requires_config_enabled MBEDTLS_SSL_CONTEXT_SERIALIZATION
+requires_config_enabled MBEDTLS_SSL_DTLS_CONNECTION_ID
+requires_cid_compat
+run_test    "Context serialization, server serializes, with CID (legacy)" \
+            "$P_SRV dtls=1 serialize=1 exchanges=2 cid=1 cid_val=dead" \
+            "$P_CLI dtls=1 serialize=0 exchanges=2 cid=1 cid_val=beef" \
+            0 \
+            -C "Deserializing connection..." \
+            -s "Deserializing connection..."
+
+requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2
 requires_config_enabled MBEDTLS_SSL_CONTEXT_SERIALIZATION
 run_test    "Context serialization, both serialize, CCM" \
             "$P_SRV dtls=1 serialize=1 exchanges=2" \
@@ -1856,6 +1886,17 @@ run_test    "Context serialization, both serialize, with CID" \
             -c "Deserializing connection..." \
             -s "Deserializing connection..."
 
+requires_config_enabled MBEDTLS_SSL_CONTEXT_SERIALIZATION
+requires_config_enabled MBEDTLS_SSL_DTLS_CONNECTION_ID
+requires_cid_compat
+run_test    "Context serialization, both serialize, with CID (legacy)" \
+            "$P_SRV dtls=1 serialize=1 exchanges=2 cid=1 cid_val=dead" \
+            "$P_CLI dtls=1 serialize=1 exchanges=2 cid=1 cid_val=beef" \
+            0 \
+            -c "Deserializing connection..." \
+            -s "Deserializing connection..."
+
+
 requires_config_enabled MBEDTLS_SSL_CONTEXT_SERIALIZATION
 run_test    "Context serialization, re-init, client serializes, CCM" \
             "$P_SRV dtls=1 serialize=0 exchanges=2" \
@@ -1889,6 +1930,16 @@ run_test    "Context serialization, re-init, client serializes, with CID" \
             -c "Deserializing connection..." \
             -S "Deserializing connection..."
 
+requires_config_enabled MBEDTLS_SSL_CONTEXT_SERIALIZATION
+requires_config_enabled MBEDTLS_SSL_DTLS_CONNECTION_ID
+requires_cid_compat
+run_test    "Context serialization, re-init, client serializes, with CID (legacy)" \
+            "$P_SRV dtls=1 serialize=0 exchanges=2 cid=1 cid_val=dead" \
+            "$P_CLI dtls=1 serialize=2 exchanges=2 cid=1 cid_val=beef" \
+            0 \
+            -c "Deserializing connection..." \
+            -S "Deserializing connection..."
+
 requires_config_enabled MBEDTLS_SSL_CONTEXT_SERIALIZATION
 run_test    "Context serialization, re-init, server serializes, CCM" \
             "$P_SRV dtls=1 serialize=2 exchanges=2" \