Add remaining TLS 1.3 ciphersuites #2550
Conversation
snhenson
force-pushed
the
tls13_ciphersuite
branch
from
ee07d10
to
038776c
Compare
snhenson
changed the title
snhenson
force-pushed
the
tls13_ciphersuite
branch
from
038776c
to
9503e3d
Compare
ssl/record/ssl3_record_tls13.c
Outdated
| NULL) <= 0) | ||
| return -1; | ||
| } else { | ||
| taglen = EVP_GCM_TLS_TAG_LEN; |
There was a problem hiding this comment.
I kind of think we should have a branch here for chacha/poly...even it happens to be the same value. Failing that we should at least have a comment about it.
ssl/record/ssl3_record_tls13.c
Outdated
| * byte that we know must have been encrypted. | ||
| */ | ||
| if (rec->length < EVP_GCM_TLS_TAG_LEN) | ||
| if (rec->length < taglen) |
There was a problem hiding this comment.
Should this now be if (rec->length < taglen + 1) to account for the second TODO item you removed here? We've done the work to swap over the record layer.
ssl/s3_lib.c
Outdated
| SSL_kANY, | ||
| SSL_aANY, | ||
| SSL_HIGH, | ||
| SSL_HANDSHAKE_MAC_SHA256 | TLS1_PRF_SHA256, |
There was a problem hiding this comment.
Can't we drop the TLS1_PRF_SHA256 bit?
There was a problem hiding this comment.
Well this is presumably (haven't checked the code) used to determine which algorithm to use for the HKDF: maybe change it to TLS13_HKDF_SHA256 etc?
There was a problem hiding this comment.
Ah I see we're using the handshake MD for that instead. I'll remove that PRF bit.
There was a problem hiding this comment.
Yeah, I think they're always the same in TLS1.3, so there doesn't seem any reason to have two flags which are always going to indicate the same hash.
| @@ -2127,6 +2127,10 @@ static int aes_ccm_cipher(EVP_CIPHER_CTX *ctx, unsigned char *out, | |||
| if (cctx->tls_aad_len >= 0) | |||
| return aes_ccm_tls_cipher(ctx, out, in, len); | |||
|
|
|||
| /* EVP_*Final() doesn't return any data */ | |||
| if (in == NULL && out != NULL) | |||
| return 0; | |||
There was a problem hiding this comment.
I'm not sure I understand this change
There was a problem hiding this comment.
Due to a bug in the CCM code EVP_*Final() used to always return an error if any data had been processed: that was because EVP_*Update() in CCM mode resets the iv_set, tag_set and len_set fields in (as CCM mode can only call EVP_*Update once).
There was a problem hiding this comment.
So is this a bug fix for backport?
There was a problem hiding this comment.
Yes. I'll create a PR for 1.1.0.
Add SSL_kANY and SSL_aANY contants for TLS 1.3 ciphersuites. Return appropriate text strings when they are used.
snhenson
force-pushed
the
tls13_ciphersuite
branch
from
6d1f9d2
to
776c21a
Compare
|
Update: rebased and addressed comments so far. |
| SSL_AEAD, | ||
| TLS1_3_VERSION, TLS1_3_VERSION, | ||
| 0, 0, | ||
| SSL_NOT_DEFAULT | SSL_HIGH, |
There was a problem hiding this comment.
Why are we marking the CCM ciphersuites as SSL_NOT_DEFAULT?
There was a problem hiding this comment.
We've done the same with the other CCM ciphersuites.
There was a problem hiding this comment.
I found it here: a556f34
The rationale provided for removing CCM from the default cipherlist is that it's "rarely used" (which is both subjective and circular) and to "reduce ClientHello bloat" (which is hardly applicable to TLS 1.3).
Add SSL_kANY and SSL_aANY contants for TLS 1.3 ciphersuites. Return appropriate text strings when they are used. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #2550)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #2550)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #2550)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #2550)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #2550)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #2550)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #2550)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #2550)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #2550)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #2550)
Checklist
Description of change
This adds the remaining TLS 1.3 ciphersuites and fixes the code to support CCM. They all connect with OpenSSL itself and GCM-256 and Chacha/Poly connects to the google test server.
Q: anyone have an implementation that supports CCM and CCM8 we can test against?
Added entries to the cipherlist test. The existing tests will check a handshake can be completed with the new ciphersuites,
The Chacha/Poly code might need to be more explicit too: it works but that's seems to be coincidence because the GCM parameters work with it.