-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
Don't change client random in Client Hello in its second flight #4490
Conversation
ssl/statem/statem_clnt.c
Outdated
@@ -1036,7 +1036,7 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt) | |||
} | |||
} | |||
} else | |||
i = 1; | |||
i = s->hello_retry_request == 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.
While we are touching this line anyway, could we enclose it in {}
. The style guide says that if one branch of an if
uses {}
then both should.
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.
"{}" added in 5187e42
Note also that this is only a partial fix for #4292. It addresses the random problem, but not the changing of extensions. That's ok though - we can address that in some later PR. |
Yes, this is a partial fix. |
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.
committer should squash, of course.
But do we want to add a test for this case?
(Do we also want a test that fails when receiving a ClientRandom of all zeros?)
Should I push squashed commit? |
Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #4490)
Squashed and pushed. A test would be nice, but I've pushed this for now anyway |
Addresses #4292
It looks like https://tools.ietf.org/html/draft-ietf-tls-tls13-21#section-4.1.2 does not explicitly allow a client to send the difference client random in its second flight Client Hello.
At least picotls checks they are the same, and aborts a handshake if they are different.
Checklist