Test TLS 1.2 builds with each encryption type #6374

mprse · 2022-09-27T13:10:25Z

Description

Resolves #6313

Status

READY

Requires Backporting

Yes/2.28
#6394

Migrations

NO

Additional comments

Any additional information that could be of interest

Todos

null cipher
only CBC-legacy (no EtM)
only CBC-legacy and CBC-EtM

Signed-off-by: Przemek Stekiel <[email protected]>

Both functions are used when MBEDTLS_SSL_SOME_SUITES_USE_MAC is defined not MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC. Signed-off-by: Przemek Stekiel <[email protected]>

…e() functions Both functions are calling mbedtls_cipher_auth_[encrypt/decrypt]_ext() functions. These functions are guarded with MBEDTLS_CIPHER_MODE_AEAD || MBEDTLS_NIST_KW_C flags - make it consistent. As a result ssl_server2 won't build now with MBEDTLS_SSL_SESSION_TICKETS enabled (mbedtls_cipher_auth_[encrypt/decrypt]_ext() functions not available). Mark MBEDTLS_SSL_SESSION_TICKETS as dependent on MBEDTLS_CIPHER_MODE_AEAD || MBEDTLS_NIST_KW_C and disable MBEDTLS_SSL_SESSION_TICKETS in stream cipher only build. Signed-off-by: Przemek Stekiel <[email protected]>

It is done to have MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH macro available (used in tests) Signed-off-by: Przemek Stekiel <[email protected]>

Signed-off-by: Przemek Stekiel <[email protected]>

mprse · 2022-09-27T13:22:20Z

@mpg I wasn't sure if this is going in the right direction a specially part with disabling MBEDTLS_SSL_SESSION_TICKETS, so I created this work in progress PR.
Currently I am able to build default configuration with stream cipher only, but still one test is failing:

DTLS Handshake with serialization, tls1_2 ......................... FAILED
  mbedtls_ssl_context_save( &(server.ssl), NULL, 0, &context_buf_len ) == MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL
  at step 1, line 2303, suites/test_suite_ssl.function
  lhs = 0xffffffffffff8f00 = -28928
  rhs = 0xffffffffffff9600 = -27136

It is failing because mbedtls_ssl_context_save() function returns MBEDTLS_ERR_SSL_BAD_INPUT_DATA due to the fillowing check:

mbedtls/library/ssl_tls.c

Lines 3807 to 3812 in 5596c74

    
           /* We must be using an AEAD ciphersuite */ 
        
           if( mbedtls_ssl_transform_uses_aead( ssl->transform ) != 1 ) 
        
           { 
        
               MBEDTLS_SSL_DEBUG_MSG( 1, ( "Only AEAD ciphersuites supported" ) ); 
        
               return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); 
        
           }

I assume that the check fails correctly as AEAD is disabled, so probably this test should be skipped.
The condition to check AEAD is also unclear to me (is MBEDTLS_SSL_SOME_SUITES_USE_MAC guard correct):

mbedtls/library/ssl_misc.h

Lines 1041 to 1050 in 5596c74

    
           static inline int mbedtls_ssl_transform_uses_aead( 
        
                   const mbedtls_ssl_transform *transform ) 
        
           { 
        
           #if defined(MBEDTLS_SSL_SOME_SUITES_USE_MAC) 
        
               return( transform->maclen == 0 && transform->taglen != 0 ); 
        
           #else 
        
               (void) transform; 
        
               return( 1 ); 
        
           #endif 
        
           }

gilles-peskine-arm · 2022-09-27T13:39:25Z

I assume that the check fails correctly as AEAD is disabled, so probably this test should be skipped.

Right, it makes sense that mbedtls_ssl_context_save returns a “I can't do this at all” error in this situation, which has precedence over the usual “I could do this if the buffer was bigger”. Since handshake serialization is only implemented with AEAD, there should probably be a requirement in check_config.h (and then MBEDTLS_SSL_CONTEXT_SERIALIZATION should be disabled in this non-AEAD build).

is MBEDTLS_SSL_SOME_SUITES_USE_MAC guard correct

Looks correct to me: all supported cipher suites either have a MAC or an AEAD tag (we don't implement the NULL_NULL cipher suite which has no security at all, only cipher suites with authentication but where the encryption may be NULL). If there are MAC cipher suites in the build then we check whether the selected cipher suite uses AEAD; if there are no MAC cipher suites then the selected cipher suite must be AEAD.

Signed-off-by: Przemek Stekiel <[email protected]>

…cipher only Signed-off-by: Przemek Stekiel <[email protected]>

In full config TLS 1.2 is disabled. Signed-off-by: Przemek Stekiel <[email protected]>

mpg

Just a first quick review for early feedback.

mpg · 2022-09-28T08:02:55Z

tests/scripts/all.sh

@@ -1274,6 +1274,38 @@ component_test_crypto_full_no_cipher () {
    make test
 }

+component_test_crypto_default_stream_cipher_only () {


Naming: I'm not sure "crypto" is meaningful here. Since this test (and the other two that are coming) are motivated by the risk for issues with TLS 1.2 when only one of its encryption modes is active, I think tls1_2 would make more sense.

mpg · 2022-09-28T08:04:13Z

tests/scripts/all.sh

+    scripts/config.py unset MBEDTLS_CHACHAPOLY_C
+    # CBC-legacy (controlled by MBEDTLS_CIPHER_MODE_CBC plus at least one block cipher (AES, ARIA, Camellia, DES))
+    scripts/config.py unset MBEDTLS_CIPHER_MODE_CBC
+    scripts/config.py unset MBEDTLS_AES_C


I don't think you need to disable all those ciphers, just disabling CIPHER_MODE_CBC is enough. I think also disabling the ciphers just add clutter (especially with things that depend on AES).

mpg · 2022-09-28T08:08:19Z

tests/scripts/all.sh

+    # CBC-EtM (controlled by the same as CBC-legacy plus MBEDTLS_SSL_ENCRYPT_THEN_MAC)
+    scripts/config.py unset MBEDTLS_SSL_ENCRYPT_THEN_MAC
+    # stream (currently that's just the NULL pseudo-cipher (controlled by MBEDTLS_CIPHER_NULL_CIPHER)
+    scripts/config.py unset MBEDTLS_CIPHER_NULL_CIPHER


I find it potentially confusing that unset this here and set is a few lines below.

tests/scripts/all.sh

library/ssl_ticket.c

library/psa_crypto.c

tests/scripts/all.sh

Signed-off-by: Przemek Stekiel <[email protected]>

mprse · 2022-09-29T07:17:47Z

While addressing review comments and optimizing components setup I encountered strange failure:

CMAC init #7 Camellia: wrong cipher ............................... FAILED
  ( cipher_info = mbedtls_cipher_info_from_type( cipher_type ) ) != NULL
  at line 112, suites/test_suite_cmac.function

Test case:

mbedtls/tests/suites/test_suite_cmac.data

Lines 31 to 33 in 77c691f

    
           CMAC init #7 Camellia: wrong cipher 
        
           depends_on:MBEDTLS_CAMELLIA_C 
        
           mbedtls_cmac_setkey:MBEDTLS_CIPHER_ID_CAMELLIA:128:MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA

MBEDTLS_CIPHER_ID_CAMELLIA is cipher id and it is used as cipher type? This is wrong usage. It is probably done to achieve MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA returned by mbedtls_cipher_cmac(), but now mbedtls_cipher_info_from_type() fails.
MBEDTLS_CIPHER_ID_CAMELLIA is treated as MBEDTLS_CIPHER_AES_128_CBC which in stream cipher only config is disabled.

The test case description says CMAC init #7 Camellia: wrong cipher , but it is unclear to me that we are testing here.

mpg · 2022-09-29T07:27:13Z

Ah, each time we add tests we uncover new and in this case pretty unexpected bugs!

I think test cases 5 to 7 are wrong in that they use a cipher ID where a cipher type is expected, as you say. I think cases 5 and 6 should use MBEDTLS_CIPHER_AES_128_ECB and case 7 should use MBEDTLS_CIPHER_CAMELLIA_128_ECB - this test is about checking that only AES and 3DES are accepted (because CMAC is defined by NIST, and AES and 3DES are the only NIST-approved block ciphers).

Signed-off-by: Przemek Stekiel <[email protected]>

… cipher components Signed-off-by: Przemek Stekiel <[email protected]>

…ket_parse() functions" This reverts commit a82290b. Signed-off-by: Przemek Stekiel <[email protected]>

Signed-off-by: Przemek Stekiel <[email protected]>

mpg

LGTM

AndrzejKurek · 2022-10-10T19:17:58Z

include/mbedtls/check_config.h

@@ -874,6 +874,11 @@
 #error "MBEDTLS_SSL_TICKET_C defined, but not all prerequisites"
 #endif

+#if defined(MBEDTLS_SSL_TICKET_C) && \
+    !( defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CCM_C) || defined(MBEDTLS_CHACHAPOLY_C) )
+#error "MBEDTLS_SSL_TICKET_C  defined, but not all prerequisites"


Minor: double space

AndrzejKurek · 2022-10-10T19:22:07Z

programs/ssl/ssl_server2.c

@@ -1404,7 +1404,7 @@ int main( int argc, char *argv[] )
 #if defined(MBEDTLS_SSL_CACHE_C)
    mbedtls_ssl_cache_context cache;
 #endif
-#if defined(MBEDTLS_SSL_SESSION_TICKETS)
+#if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_TICKET_C)


Ifdef at line 286 - missing MBEDTLS_SSL_TICKET_C

@mprse I believe it's about the guards around USAGE_TICKETS near the top of the file. It's not a major issue (the worst that will happen is that the usage message will mention options that are not actually available in some configs), but not that Andrzej spotted it, we might as well fix it.

Yup. I hope it is ok now (force-pushed the last commit).

AndrzejKurek

Please fix the missing ifdef around the usage text.

Signed-off-by: Przemek Stekiel <[email protected]>

mpg

LGTM

mprse · 2022-10-12T08:06:49Z

This one together with backport seems to be ready for merge, but I'm not sure about the CI. There is windows build failure, but looks unrelated.

gilles-peskine-arm · 2022-10-12T10:45:21Z

The Windows failure on OpenCI is an infrastructure glitch and the armcc failure is a known infrastructure issue. The Arm CI passed so this PR has passed the CI.

mprse added 5 commits September 27, 2022 15:04

Add component to build and test default config with stream cipher only

11c362a

Signed-off-by: Przemek Stekiel <[email protected]>

Fix guards for mbedtls_ct_size_mask() and mbedtls_ct_memcpy_if_eq()

89ad623

Both functions are used when MBEDTLS_SSL_SOME_SUITES_USE_MAC is defined not MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC. Signed-off-by: Przemek Stekiel <[email protected]>

test_suite_cipher.function: always include aes.h

6f29a6c

It is done to have MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH macro available (used in tests) Signed-off-by: Przemek Stekiel <[email protected]>

Fix unused variables warnings in default + stream cipher only build

4c49927

Signed-off-by: Przemek Stekiel <[email protected]>

mprse added needs-work needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon labels Sep 27, 2022

mprse added 5 commits September 28, 2022 07:59

Make MBEDTLS_SSL_CONTEXT_SERIALIZATION dependent on AEAD

d582a01

Signed-off-by: Przemek Stekiel <[email protected]>

Use basic symbols instead MBEDTLS_CIPHER_MODE_AEAD in check config

e31ba83

Signed-off-by: Przemek Stekiel <[email protected]>

Add component to build and test full config with stream cipher only

9550c05

Signed-off-by: Przemek Stekiel <[email protected]>

Add components to build and test default/full config with legacy-ccm …

b0de1c0

…cipher only Signed-off-by: Przemek Stekiel <[email protected]>

Change testing strategy to default + one cypher only (psa/no psa)

0cc3466

In full config TLS 1.2 is disabled. Signed-off-by: Przemek Stekiel <[email protected]>

mprse changed the title ~~Test TLS 1.2 builds with stream(null) encryption type~~ Test TLS 1.2 builds with each encryption type Sep 28, 2022

mprse added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-work labels Sep 28, 2022

mpg requested changes Sep 28, 2022

View reviewed changes

Optimize one cipher only components and adapt nemes

68db0d2

Signed-off-by: Przemek Stekiel <[email protected]>

mprse added 4 commits September 29, 2022 09:53

test_suite_cmac.data: fix bug: use cipher type instead cipher id

a891a09

Signed-off-by: Przemek Stekiel <[email protected]>

Remove redundant indirect dependencies after optimizing setup for one…

8d4b241

… cipher components Signed-off-by: Przemek Stekiel <[email protected]>

Revert "Fix guards for mbedtls_ssl_ticket_write() and mbedtls_ssl_tic…

ce5b68c

…ket_parse() functions" This reverts commit a82290b. Signed-off-by: Przemek Stekiel <[email protected]>

Add ssl-opt tls 1.2 tests for single cipher builds

48a6a66

Signed-off-by: Przemek Stekiel <[email protected]>

mprse force-pushed the enc_types branch from d0b53d7 to 48a6a66 Compare September 29, 2022 13:29

mpg previously approved these changes Oct 10, 2022

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports needs-backports Backports are missing or are pending review and approval. and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 10, 2022

AndrzejKurek reviewed Oct 10, 2022

View reviewed changes

AndrzejKurek added needs-work and removed approved Design and code approved - may be waiting for CI or backports labels Oct 10, 2022

AndrzejKurek suggested changes Oct 10, 2022

View reviewed changes

mprse dismissed mpg’s stale review via 20c4283 October 11, 2022 06:40

mprse added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Oct 11, 2022

mprse requested review from AndrzejKurek and mpg October 11, 2022 06:42

Fix missing guard and double-space

d61a4d3

Signed-off-by: Przemek Stekiel <[email protected]>

mprse force-pushed the enc_types branch from 20c4283 to d61a4d3 Compare October 11, 2022 07:41

AndrzejKurek approved these changes Oct 11, 2022

View reviewed changes

mpg approved these changes Oct 11, 2022

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. labels Oct 11, 2022

mprse mentioned this pull request Oct 12, 2022

Enable testing of AEAD drivers with libtestdriver1 #6383

Merged

gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Oct 12, 2022

gilles-peskine-arm merged commit 8fd3254 into Mbed-TLS:development Oct 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test TLS 1.2 builds with each encryption type #6374

Test TLS 1.2 builds with each encryption type #6374

mprse commented Sep 27, 2022 •

edited

Loading

mprse commented Sep 27, 2022

gilles-peskine-arm commented Sep 27, 2022

mpg left a comment

mpg Sep 28, 2022

mpg Sep 28, 2022

mpg Sep 28, 2022

mprse commented Sep 29, 2022

mpg commented Sep 29, 2022

mpg left a comment

AndrzejKurek Oct 10, 2022

AndrzejKurek Oct 10, 2022

mpg Oct 11, 2022

mprse Oct 11, 2022 •

edited

Loading

AndrzejKurek left a comment

mpg left a comment

mprse commented Oct 12, 2022

gilles-peskine-arm commented Oct 12, 2022

Test TLS 1.2 builds with each encryption type #6374

Test TLS 1.2 builds with each encryption type #6374

Conversation

mprse commented Sep 27, 2022 • edited Loading

Description

Status

Requires Backporting

Migrations

Additional comments

Todos

mprse commented Sep 27, 2022

gilles-peskine-arm commented Sep 27, 2022

mpg left a comment

Choose a reason for hiding this comment

mpg Sep 28, 2022

Choose a reason for hiding this comment

mpg Sep 28, 2022

Choose a reason for hiding this comment

mpg Sep 28, 2022

Choose a reason for hiding this comment

mprse commented Sep 29, 2022

mpg commented Sep 29, 2022

mpg left a comment

Choose a reason for hiding this comment

AndrzejKurek Oct 10, 2022

Choose a reason for hiding this comment

AndrzejKurek Oct 10, 2022

Choose a reason for hiding this comment

mpg Oct 11, 2022

Choose a reason for hiding this comment

mprse Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

AndrzejKurek left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mprse commented Oct 12, 2022

gilles-peskine-arm commented Oct 12, 2022

mprse commented Sep 27, 2022 •

edited

Loading

mprse Oct 11, 2022 •

edited

Loading