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
TLSv1.3: fix padding extension #2968
Conversation
In OpenSSL 1.1.0 the padding extension MUST be last because it calculates the length of everything that has been written into the ClientHello to determine whether it needs to be padded or not. With TLSv1.3 that isn't possible because the specification requires that the PSK extension is last. Therefore we need to fix the padding extension to take account of any PSK extension that will be later added.
test/clienthellotest.c
Outdated
|
||
return testresult?0:1; | ||
return testresult; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the use of testresult
entirely and just:
return run_tests(argv[0]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Done.
ccdca80
to
2b2e6c9
Compare
Check that the padding extension pads correctly for various scenarios.
2b2e6c9
to
4bf3c4c
Compare
Updated to take account of feedback. Also fixed an issue with out-of-tree builds (could not find the session file) |
test/clienthellotest.c
Outdated
int test_main(int argc, char *argv[]) | ||
{ | ||
if (argc != 2) | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or return EXIT_FAILURE
, that will make it easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
New commit pushed which addresses above feedback. It also fixes a problem where TLSv1.3 is not enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one comment-update request, LGTM.
@@ -751,6 +784,12 @@ int tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context, X509 *x, | |||
s->session->ext.tick_identity = TLSEXT_PSK_BAD_IDENTITY; | |||
|
|||
/* | |||
* Note: At this stage we only support adding a single resumption PSK. If |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at this stage of the code, or the protocol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
+1 |
Pushed. Thanks. |
In OpenSSL 1.1.0 the padding extension MUST be last because it calculates the length of everything that has been written into the ClientHello to determine whether it needs to be padded or not. With TLSv1.3 that isn't possible because the specification requires that the PSK extension is last. Therefore we need to fix the padding extension to take account of any PSK extension that will be later added. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2968)
Check that the padding extension pads correctly for various scenarios. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2968)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2968)
Checklist
Description of change
In OpenSSL 1.1.0 the padding extension MUST be last because it calculates
the length of everything that has been written into the ClientHello to
determine whether it needs to be padded or not. With TLSv1.3 that isn't
possible because the specification requires that the PSK extension is last.
Therefore we need to fix the padding extension to take account of any PSK
extension that will be later added.