Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FFDH 3: use in TLS 1.3 #5979

Closed
7 tasks
mpg opened this issue Jun 27, 2022 · 6 comments · Fixed by #7627
Closed
7 tasks

FFDH 3: use in TLS 1.3 #5979

mpg opened this issue Jun 27, 2022 · 6 comments · Fixed by #7627
Assignees
Labels
component-tls13 enhancement size-m Estimated task size: medium (~1w)

Comments

@mpg
Copy link
Contributor

mpg commented Jun 27, 2022

Context: TLS 1.3 (RFC 8446) supports key exchange either with ECDHE or DHE (aka Finite-Field Diffie-Hellman or FFDH). Contrary to TLS 1.2 where those are seen as different key exchange mechanisms, in TLS 1.3 they are treated as variants of the same mechanisms, distinguished only by the just of "group" (an elliptic curve of finite field); this matches nicely the PSA Crypto API for key agreement, which works in the same way. Also, TLS 1.3 only supports FFDH on named groups which again matches nicely the PSA Crypto API.

This task is to implement support for DHE key exchange in TLS 1.3. Most of the code should be shared with ECDHE and only minor extensions should be needed:

  • Add the DHE groups to the default list of supported groups: ssl_preset_default_groups.
  • Ensure that values for DHE groups are correctly sent in ssl_write_supported_groups_ext().
  • Ensure that values for DHE groups are correctly parsed in ssl_tls13_parse_supported_groups_ext() (might consist of adjusting mbedtls_ssl_named_group_is_supported()).
  • The computation itself should work the same.
  • Allow configuring the groups from the command line in ssl_client2.c and ssl_server2.c - this can be done either by making the existing curve command-line option accept FFDH groups as well, or creating a new option that does.
  • Add tests in ssl-opt.sh - one positive test per group, plus a negative test for when the client and server have no group in common (for example, one only has EC the other only FF).
  • In ssl-opt.sh we also want positive tests using OpenSSL and GnuTLS for each group. (This partially makes up for the lack of official test vectors.)

Note: Currently (and until Mbed TLS 4.0) the API for configuring supported groups is not as clean as we'd like, as it consists of two functions: mbedtls_ssl_conf_curves() (deprecated but still supported until 4.0, only allows configuring curves) and mbedtls_ssl_conf_groups() (recommended, allows configuring both curves and finite fields). We jump through a few hoops internally to make this work, see the implementations of these two functions, as well as mbedtls_ssl_get_groups().

Note: Unlike elliptic curves, there is not per-group config options, if FFDH is supported then all groups are supported, see PSA conditional inclusion documentation

Depends on: #3261, #5912

@mpg mpg added enhancement size-m Estimated task size: medium (~1w) labels Jun 27, 2022
@mpg mpg added this to Use PSA: misc. gaps in EPICs for Mbed TLS Jun 27, 2022
@steven-bellock
Copy link

You may be aware of this already, but there is also the change between 1.2 and 1.3 with respect to the negotiated key:

1.2 says

A conventional Diffie-Hellman computation is performed. The
negotiated key (Z) is used as the pre_master_secret, and is converted
into the master_secret, as specified above. Leading bytes of Z that
contain all zero bits are stripped before it is used as the
pre_master_secret.

1.3 says

For finite field groups, a conventional Diffie-Hellman [DH76]
computation is performed. The negotiated key (Z) is converted to a
byte string by encoding in big-endian form and left-padded with zeros
up to the size of the prime. This byte string is used as the shared
secret in the key schedule as specified above.
Note that this construction differs from previous versions of TLS
which removed leading zeros.

@mpg
Copy link
Contributor Author

mpg commented Jul 6, 2022

Good point. Fortunately, what TLS 1.3 requires is what PSA Crypto does according to PSA_ALG_FFDH's documentation:

The shared secret produced by this key agreement algorithm is g^{ab} in big-endian format. It is ceiling(m / 8) bytes long where m is the size of the prime p in bits.

So, here again, the PSA Crypto API is a better match for TLS 1.3 than for 1.2, which is not really surprising, as both TLS 1.3 and the PSA API benefit from more accumulated knowledge than TLS 1.2.

@mprse
Copy link
Contributor

mprse commented Jul 18, 2022

The computation itself should work the same.

I'm probably missing something, but I think that some changes in computation will be also needed as we need to use FFDH keys somewhere.

For example here:

/* HRR could already have requested something else. */
group_id = ssl->handshake->offered_group_id;
if( !mbedtls_ssl_tls13_named_group_is_ecdhe( group_id ) &&
!mbedtls_ssl_tls13_named_group_is_dhe( group_id ) )
{
MBEDTLS_SSL_PROC_CHK( ssl_tls13_get_default_group_id( ssl,
&group_id ) );
}
/*
* Dispatch to type-specific key generation function.
*
* So far, we're only supporting ECDHE. With the introduction
* of PQC KEMs, we'll want to have multiple branches, one per
* type of KEM, and dispatch to the corresponding crypto. And
* only one key share entry is allowed.
*/
client_shares = p;
#if defined(MBEDTLS_ECDH_C)
if( mbedtls_ssl_tls13_named_group_is_ecdhe( group_id ) )
{
/* Pointer to group */
unsigned char *group = p;
/* Length of key_exchange */
size_t key_exchange_len = 0;
/* Check there is space for header of KeyShareEntry
* - group (2 bytes)
* - key_exchange_length (2 bytes)
*/
MBEDTLS_SSL_CHK_BUF_PTR( p, end, 4 );
p += 4;
ret = mbedtls_ssl_tls13_generate_and_write_ecdh_key_exchange(
ssl, group_id, p, end, &key_exchange_len );
p += key_exchange_len;
if( ret != 0 )
return( ret );
/* Write group */
MBEDTLS_PUT_UINT16_BE( group_id, group, 0 );
/* Write key_exchange_length */
MBEDTLS_PUT_UINT16_BE( key_exchange_len, group, 2 );
}
else
#endif /* MBEDTLS_ECDH_C */
if( 0 /* other KEMs? */ )
{
/* Do something */
}
else
return( MBEDTLS_ERR_SSL_INTERNAL_ERROR );

From my point of view ssl_tls13_get_default_group_id() needs to be adapted and we need implementation when named_group is dhe (ffdh).

@mpg
Copy link
Contributor Author

mpg commented Jul 19, 2022

Yes, in my mind that's the negotiation, and clearly that part has to change. When I said the computation itself should work the same, I meant things like generating a key pair, sending the public part and computing the shared secret - I think those parts will use the same PSA functions, just with different arguments for key type / alg / size / etc. How we choose those arguments and communicate with our peer about it, indeed needs to change.

I mean if you look at the comment below about dispatching based on the key exchange mechanism (KEM) and the fact that each will have its own branch, it is my initial impression (which may be wrong) that ECDH and FFDH should be able to use the same function, just with different parameters. As opposed to (future) PQC key exchanges which are likely to need entirely different code.

Does that make sense? Do you think I'm missing something? (I'm not super familiar with the 1.3 code.)

@mprse
Copy link
Contributor

mprse commented Dec 19, 2022

The FFDH PR is still under review, but I refreshed PR #6102. The ssl-opt tests that are using our implementation for client/server are passing, but I have a problem with adding test that use openssl/gnutls. @mpg can you please give a hint how openssl/gnutls should be configured for client/server FFDHE tests?

For example to test our implementation with gnutls client I'm running the following config:

run_test    "TLS 1.3 G->m  FFDHE2048" \
            "$P_SRV debug_level=5 force_version=tls13 curves=ffdhe2048" \
            "$G_NEXT_CLI localhost --debug=999 --priority=NORMAL:-GROUP-ALL:+GROUP-FFDHE2048:%NO_TICKETS:%DISABLE_TLS13_COMPAT_MODE -V" \

The handshake fails, but I'm not sure if this is correct usage of gnutls client.

|<4>| HSK[0x556c67529780]: SERVER HELLO (2) was received. Length 310[310], frag offset 0, frag length: 310, sequence: 0
|<3>| ASSERT: buffers.c[get_last_packet]:1176
|<3>| ASSERT: buffers.c[_gnutls_handshake_io_recv_int]:1428
|<4>| HSK[0x556c67529780]: Server's version: 3.3
|<4>| EXT[0x556c67529780]: Parsing extension 'Supported Versions/43' (2 bytes)
|<4>| EXT[0x556c67529780]: Negotiated version: 3.4
|<4>| HSK[0x556c67529780]: Selected cipher suite: GNUTLS_AES_128_CCM_SHA256
|<4>| EXT[0x556c67529780]: Parsing extension 'Key Share/51' (260 bytes)
|<4>| HSK[0x556c67529780]: Selected group FFDHE2048 (256)
|<3>| ASSERT: key_share.c[client_use_key_share]:492
|<3>| ASSERT: key_share.c[key_share_recv_params]:653
|<3>| ASSERT: hello_ext.c[hello_ext_parse]:275
|<3>| ASSERT: extv.c[_gnutls_extv_parse]:69
|<3>| ASSERT: hello_ext.c[_gnutls_parse_hello_extensions]:308
|<3>| ASSERT: handshake.c[read_server_hello]:2080
|<3>| ASSERT: handshake.c[_gnutls_recv_handshake]:1648
|<3>| ASSERT: handshake.c[handshake_client]:3055
|<13>| BUF[HSK]: Emptied buffer
*** Fatal error: An illegal parameter has been received.

EDIT:
Tried with openssl:

run_test    "TLS 1.3 O->m  FFDHE2048" \
            "$P_SRV debug_level=5 force_version=tls13 curves=ffdhe2048" \
            "$O_NEXT_CLI -msg -debug -groups ffdhe2048" \

# TLS 1.3 O->m  FFDHE2048
echo 'GET / HTTP/1.0' | /usr/local/openssl-1.1.1a/bin/openssl s_client -CAfile data_files/test-ca_cat12.crt -connect 127.0.0.1:11360 -msg -debug -groups ffdhe2048
Error with command: "-groups ffdhe2048"

It seem that our version of `OpenSSL 1.1.1a 20 Nov 2018' does not support ffhde groups for tls 1.3. It was added later(openssl/openssl#8178).

EDIT:
To use gnutls as server I need certificate and key file with FFDH key. I don't see such keys in our .key files in tests/data_files directory. How these key files are generated?

@mpg
Copy link
Contributor Author

mpg commented Dec 19, 2022

@mprse Considering this is about TLS 1.3, I guess @ronald-cron-arm or @yuhaoth may be more able to help than me. I'm not seeing anything obviously wrong in the above call.

@daverodgman daverodgman added this to Use PSA: misc. gaps in Backlog for Mbed TLS Feb 22, 2023
@daverodgman daverodgman removed this from Use PSA: misc. gaps in EPICs for Mbed TLS Feb 22, 2023
@daverodgman daverodgman added this to Use PSA: misc. gaps in EPICs for Mbed TLS Apr 4, 2023
@daverodgman daverodgman removed this from Use PSA: misc. gaps in Backlog for Mbed TLS Apr 4, 2023
@mpg mpg closed this as completed in #7627 Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-tls13 enhancement size-m Estimated task size: medium (~1w)
Projects
None yet
3 participants