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

improve SSL_CTX_set_tlsext_ticket_key_cb ref impl #12063

Closed

Conversation

@gstrauss
Copy link
Contributor

@gstrauss gstrauss commented Jun 5, 2020

improve reference implementation code in
SSL_CTX_set_tlsext_ticket_key_cb man page

change EVP_aes_128_cbc() to EVP_aes_256_cbc(), with the implication
of requiring longer keys. Updating this code brings the reference
implementation in line with implementation in openssl committed in 2016:
commit 05df5c2
Use AES256 for the default encryption algoritm for TLS session tickets

add comments where user-implementation is needed to complete code

CLA: trivial

Checklist
  • documentation is added or updated
improve reference implementation code in
  SSL_CTX_set_tlsext_ticket_key_cb man page

change EVP_aes_128_cbc() to EVP_aes_256_cbc(), with the implication
of requiring longer keys.  Updating this code brings the reference
implementation in line with implementation in openssl committed in 2016:
commit 05df5c2
Use AES256 for the default encryption algoritm for TLS session tickets

add comments where user-implementation is needed to complete code

CLA: trivial
@gstrauss
Copy link
Contributor Author

@gstrauss gstrauss commented Jun 5, 2020

Should a similar patch be prepared for OpenSSL_1_1_1-stable ?

@kaduk
Copy link
Contributor

@kaduk kaduk commented Jun 6, 2020

It's probably best to wait to prepare the 1.1.1 version until the version for master is done/approved, in case anything ends up changing in it.

Also, it's interesting that the commit you linked only changed from AES128 to AES256, but left SHA256 instead of going up to SHA384 or SHA512 as is typically paired with AES256.

@gstrauss
Copy link
Contributor Author

@gstrauss gstrauss commented Jun 6, 2020

This matches the algorithms used in ssl/statem/statem_srvr.c:construct_stateless_ticket()
https://github.com/openssl/openssl/blob/master/ssl/statem/statem_srvr.c#L4015
If stronger algorithms should be recommended, then should the algorithms used in construct_stateless_ticket() also be reviewed?

@kaduk
Copy link
Contributor

@kaduk kaduk commented Jun 6, 2020

If stronger algorithms should be recommended, then should the algorithms used in construct_stateless_ticket() also be reviewed?

Yes, they should be reviewed. @mattcaswell please take a look. I'm inclined to swap out sha256 for sha384, though to be honest I'm not entirely convinced we need to be using AES256 here. If we were all TLS 1.3 I'd be more solidly in the "go back to AES128" camp, but given how stateless tickets are used in earlier versions we may want the extra protection, to prevent future computing advances from allowing for decryption of previously captured encrypted traffic.

@kaduk
Copy link
Contributor

@kaduk kaduk commented Jun 6, 2020

Also, thank you for noticing the discrepancy and submitting the patch! It is good that we are revisiting what we are actually using in construct_stateless_ticket() as a result.

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Jun 8, 2020

I'm inclined to swap out sha256 for sha384

That is my inclination too. Let's go with that.

@t8m
Copy link
Member

@t8m t8m commented Jun 8, 2020

It's probably best to wait to prepare the 1.1.1 version until the version for master is done/approved, in case anything ends up changing in it.

Also, it's interesting that the commit you linked only changed from AES128 to AES256, but left SHA256 instead of going up to SHA384 or SHA512 as is typically paired with AES256.

If we are talking about HMAC here, we are talking about preimage or second preimage attacks and there is absolutely no point in using anything stronger than SHA256 as that has 256 bit security against preimage and second preimage attacks.

@gstrauss
Copy link
Contributor Author

@gstrauss gstrauss commented Jul 2, 2020

Is there a consensus on next steps?

Would you like me to extend this patch to update construct_stateless_ticket() and the code example to use stronger algorithms, or, as @t8m suggested, is this patch to the code example acceptable?

Copy link
Member

@mattcaswell mattcaswell left a comment

Regardless of the discussion above this looks like an improvement to me, so I think we should approve as is.

However I do think this goes beyond our definition of "CLA: trivial", so we will need a CLA.

Needs second review.

@t8m
t8m approved these changes Jul 2, 2020
@kaduk
kaduk approved these changes Jul 2, 2020
@kaduk
Copy link
Contributor

@kaduk kaduk commented Jul 2, 2020

Sorry for making this more complicated of a process than it needed to be!

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Jul 3, 2020

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@gstrauss
Copy link
Contributor Author

@gstrauss gstrauss commented Jul 6, 2020

CLA emailed to legal@opensslfoundation

Please let me know if you would like me to update the commit message in this pull request to remove the CLA: trivial tag

Thank you.

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Jul 6, 2020

Close/reopen to kick CLA bot.

@mattcaswell mattcaswell closed this Jul 6, 2020
@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Jul 6, 2020

Close/reopen to kick CLA bot.

openssl-machine pushed a commit that referenced this pull request Jul 6, 2020
improve reference implementation code in
  SSL_CTX_set_tlsext_ticket_key_cb man page

change EVP_aes_128_cbc() to EVP_aes_256_cbc(), with the implication
of requiring longer keys.  Updating this code brings the reference
implementation in line with implementation in openssl committed in 2016:
commit 05df5c2
Use AES256 for the default encryption algoritm for TLS session tickets

add comments where user-implementation is needed to complete code

CLA: trivial

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #12063)
@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Jul 6, 2020

Pushed to master. Thanks! @gstrauss - if you'd like to prepare a version for 1.1.1 then that would be helpful.

@mattcaswell mattcaswell closed this Jul 6, 2020
gstrauss added a commit to gstrauss/openssl that referenced this pull request Jul 8, 2020
improve reference implementation code in
  SSL_CTX_set_tlsext_ticket_key_cb man page

change EVP_aes_128_cbc() to EVP_aes_256_cbc(), with the implication
of requiring longer keys.  Updating this code brings the reference
implementation in line with implementation in openssl committed in 2016:
commit 05df5c2
Use AES256 for the default encryption algorithm for TLS session tickets

add comments where user-implementation is needed to complete code

(backport from openssl#12063)
openssl-machine pushed a commit that referenced this pull request Jul 9, 2020
improve reference implementation code in
  SSL_CTX_set_tlsext_ticket_key_cb man page

change EVP_aes_128_cbc() to EVP_aes_256_cbc(), with the implication
of requiring longer keys.  Updating this code brings the reference
implementation in line with implementation in openssl committed in 2016:
commit 05df5c2
Use AES256 for the default encryption algorithm for TLS session tickets

add comments where user-implementation is needed to complete code

(backport from #12063)

Reviewed-by: Ben Kaduk <kaduk@mit.edu>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #12391)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants