[3.6] TLS 1.3: call psa_crypto_init

[gilles-peskine-arm]

An application connecting to a peer that supports both TLS 1.2 and TLS 1.3 did not need to call psa_crypto_init in Mbed TLS 3.5, so it must not be required to call psa_crypto_init in Mbed TLS 3.6. We broke this in Mbed TLS 3.6.0. Fix it. Fixes #9072.

PR checklist

• changelog provided
• development PR not required. Since development will be released as Mbed TLS 4.0 which will require having called psa_crypto_init before any TLS connection, there is nothing to do.
• framework PR not required
• 3.6 PR here
• 2.28 PR not required because: bug introduced in 3.6.0 (also TLS 1.3 not in 2.28)
• tests provided; also run programs/ssl/ssl_client1 against a dual 1.2/1.3 server, which in 3.6.0 failed in with an internal error in psa_generate_key. This can still fail, but later at the end of the handshake, due to How to handle TLS 1.3 new session ticket errors #8749.

[mpg]

LGTM if the CI agrees, just suggesting an improvement to some comments.

I wonder a bit about the placement of calls to mbedtls_ssl_tls13_crypto_init(): I believe the ones you picked are correct, but I'm wondering about taking a simpler approach can just putting a single call neat the top of mbedtls_ssl_handshake_step(), guarded by

#if defined(MBEDTLS_SSL_PROTO_TLS1_3)
if (ssl->conf->max_version >= MBEDTLS_SSL_VERSION_TLS1_3)

Wdyt?

It seems that the intention was to only init PSA when we're actually negotiationg TLS 1.3, but on the client when writing the ClientHello extensions we still don't know if the server will pick 1.3, and on the server-side, currently the call is too early in ClientHello parsing for the server to know if this was a 1.2 or 1.3 hello (that check is a few lines below). So, wouldn't it be simpler to just base the call on whether 1.3 is enabled on our side or not (since the call has to occur pretty early in the handshake, before any crypto is done)?

[gilles-peskine-arm]

Otherwise, the handshake may call psa_crypto_init() if it ends up negotiating TLS 1.3.

Actually that's not quite accurate: the handshake may call psa_crypto_init if it tries to negotiate TLS 1.3. I left it more open in mbedtls_config.h.

So, wouldn't it be simpler to just base the call on whether 1.3 is enabled on our side or not

Since this is an update to an LTS, I wanted to minimize behavior changes. It's impossible to only add crypto init calls to workflows that do negotiate TLS 1.3, because the negotiation might involve crypto. The most obvious example is that a client needs PSA crypto to send a 1.3 ClientHello, and the server might be 1.2-only. But I wanted to avoid changes that weren't necessary. In particular, a server getting a 1.2-only ClientHello won't start calling psa_crypto_init.

[gilles-peskine-arm]

putting a single call neat the top of mbedtls_ssl_handshake_step()

I wanted to only change 1.3-specific files, so it would be obvious that builds without 1.3 weren't affected. Sure, an #if defined(MBEDTLS_SSL_PROTO_TLS1_3) also makes it locally obvious, but that means yet another ifdef so the code is more complex.

[gilles-peskine-arm]

Can we be more precise and say:

• A client calls psa_crypto_init if it sends a 1.3 hello;
• A server calls psa_crypto_init if it has 1.3 enabled and receives a 1.3 hello?

I wasn't sure whether we could reliably commit to that. I'm not even 100% sure that this is what's currently happening in all cases.

[mpg]

Can we be more precise

I'm actually inclined to be less precise and give ourselves more freedom to change things in the future if needed or just if we want to simplify the code.

I think "may call psa_crypto_init()` if TLS 1.3 is enabled" is good enough. I don't think a user needs to know more than that.

[mpg]

putting a single call neat the top of mbedtls_ssl_handshake_step()

I wanted to only change 1.3-specific files, so it would be obvious that builds without 1.3 weren't affected.

Yes, and that's indeed one thing I like about your solution. Mostly I wanted to make the thinking explicit about why they're placed here.

[mpg]

In particular, a server getting a 1.2-only ClientHello won't start calling psa_crypto_init.

Is that so? When looking at the code, I see PSA being initialized a few lines before we check if the parse ClientHello was 1.2 or 1.3. (Edit: in a build and SSL config where both 1.2 and 1.3 are enabled of course.)

[gilles-peskine-arm]

Ah, so we do. In an earlier attempt I had the initialization later, but it was too late for when the client sends a key share in ClientHello for PSK or session resumption.

I think it would be nice if a server operating in a 1.2-only environment would not change its behavior, but that would be more complicated to arrange. I think we'd have to have multiple places calling crypto_init in the server code.

[mpg]

I think it would be nice if a server operating in a 1.2-only environment would not change its behavior, but that would be more complicated to arrange. I think we'd have to have multiple places calling crypto_init in the server code.

I agree it would be nice, especially in an LTS where we're supposed to minimize changes. I was also afraid that would mean having to call crypto_init in more places, but after taking a look, I think there's an ideal place in ssl_tls13_parse_client_hello(): just when we've determined this is a 1.3 Hello - before that the function only does minimal parsing (memorising pointers and lengths to interesting parts of the message to come back to later when we know the negotiated version).

Wdyt?

[gilles-peskine-arm]

Thanks. I'll try that, with the current code as a fallback.

[mpg]

Can we be more precise and say:
A client calls psa_crypto_init if it sends a 1.3 hello;
A server calls psa_crypto_init if it has 1.3 enabled and receives a 1.3 hello?

In short I'd say, yes let's definitely try to do that, but no let's not announce it in the documentation - I don't think we'll have time to do enough testing to ensure we succeeded, and I'm rather not tie our hands if we want/need to change things in the future.

[gilles-peskine-arm]

What do you think about development? We can forward-port this, change nothing, or make a different patch. We don't need this at all in 4.0, which is an argument to do nothing. On the other hand, tests/include/test/psa_crypto_helpers.h is going to be shared between 3.6 and development, and the patch I made there must not be done without the rest. On the gripping hand, that file has a lot of complexity around when to initialize PSA that we won't want in 4.x anyway, so we might as well start diverging.

[gilles-peskine-arm]

Should we have a changelog entry announcing that psa_crypto_init will now sometimes be called when starting a TLS connection (even a 1.2 connection)?

[mpg]

Depends, what's observable from a user perspective about psa_crypto_init() being called?

[gilles-peskine-arm]

what's observable from a user perspective about psa_crypto_init() being called?

• It can fail. Unlikely in an application that's doing TLS, but possible.
  • Entropy failure (but how are you getting entropy for the RNG passed to TLS).
  • Stray file that screws up PSA storage initialization (but how would that stray file have been created).
  • Low resources (but you don't really stand a chance of doing a TLS connection then).
• It may leave resources around at the end, so a memory leak detector would complain. (That's why we're calling mbedtls_psa_crypto_free unconditionally in the tests.)

Those are pretty exotic concerns in an application that's doing a TLS connection, which is why I'm not sure.

[mpg]

Then I think as a precaution we should add a brief ChangeLog entry about it.

[mpg]

What do you think about development?

Let's get this merged in 3.6 first (hopefully today) and have a discussion about development later (say, tomorrow).

[mpg]

LGTM, thanks! Let's hope the CI agrees.

[ronald-cron-arm]

Only one suggestion, otherwise this looks good to me.

[ronald-cron-arm]

I think this would more naturally fit in ssl_prepare_client_hello() in ssl_client.c where we know for the first time if we are going to propose TLS 1.3 to the server or not: after lines 771 to 783 if ssl->tls_version == MBEDTLS_SSL_VERSION_TLS1_3.

After the TLS 1.2 specific code (from line 815 to 844), instead of:

#if defined(MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE)                              
    if (ssl->tls_version == MBEDTLS_SSL_VERSION_TLS1_3) {                       
        /*                                                                      
         * Create a legacy session identifier for the purpose of middlebox      
         * compatibility only if one has not been created already, which is     
         * the case if we are here for the TLS 1.3 second ClientHello.          
         *                                                                      
         * Versions of TLS before TLS 1.3 supported a "session resumption"      
         * feature which has been merged with pre-shared keys in TLS 1.3        
         * version. A client which has a cached session ID set by a pre-TLS 1.3 
         * server SHOULD set this field to that value. In compatibility mode,   
         * this field MUST be non-empty, so a client not offering a pre-TLS 1.3 
         * session MUST generate a new 32-byte value. This value need not be    
         * random but SHOULD be unpredictable to avoid implementations fixating 
         * on a specific value (also known as ossification). Otherwise, it MUST 
         * be set as a zero-length vector ( i.e., a zero-valued single byte     
         * length field ).                                                      
         */                                                                     
        session_id_len = 32;                                                    
    }                                                                           
#endif /* MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE */

we could have something like (not tested):

#if defined(MBEDTLS_SSL_PROTO_TLS1_3)                              
    if (ssl->tls_version == MBEDTLS_SSL_VERSION_TLS1_3) { 
        ret = mbedtls_ssl_tls13_crypto_init(ssl);
        if (ret != 0) {
            return ret;
        }
            
#if defined(MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE)
        /*                                                                      
         * Create a legacy session identifier for the purpose of middlebox      
         * compatibility only if one has not been created already, which is     
         * the case if we are here for the TLS 1.3 second ClientHello.          
         *                                                                      
         * Versions of TLS before TLS 1.3 supported a "session resumption"      
         * feature which has been merged with pre-shared keys in TLS 1.3        
         * version. A client which has a cached session ID set by a pre-TLS 1.3 
         * server SHOULD set this field to that value. In compatibility mode,   
         * this field MUST be non-empty, so a client not offering a pre-TLS 1.3 
         * session MUST generate a new 32-byte value. This value need not be    
         * random but SHOULD be unpredictable to avoid implementations fixating 
         * on a specific value (also known as ossification). Otherwise, it MUST 
         * be set as a zero-length vector ( i.e., a zero-valued single byte     
         * length field ).                                                      
         */                                                                     
        session_id_len = 32;
#endif                                                    
    }                                                                           
#endif /* MBEDTLS_SSL_PROTO_TLS1_3 */

Wdyt?

[gilles-peskine-arm]

So a little earlier, that should work. But it means changing a non-13 file, which I'd prefer to avoid. Also it's buried right in the middle of a function, so a failure there (admittedly unlikely) could leave the context in a worse state. On balance I prefer the place where it is now. And I'd rather avoid one more round of rework given the urgency.

[ronald-cron-arm]

Okay, fair enough, let's go with what we have.

[ronald-cron-arm]

LGTM, thanks.