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: Downgrade Protection mechanism #3022
Conversation
ssl/statem/statem_lib.c
Outdated
0x44, 0x4f, 0x57, 0x4e, 0x47, 0x52, 0x44, 0x00 | ||
}; | ||
static const unsigned char tls12downgrade[] = { | ||
0x44, 0x4f, 0x57, 0x4e, 0x47, 0x52, 0x44, 0x01 |
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.
These are in both statem_lib and s3_lib; should they be consolidated and prototyped in ssl_locl.h?
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.
Could do. I actually had that at one point in development. The problem I had was you have to declare them with an explicit size if you put the declarations in a header - otherwise sizeof doesn't work. I thought the implicit size was better, and no big deal to have them declared twice. I don't have a strong opinion either way though, so I'll go with whatever the consensus is.
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.
declare once and count the bytes carefully :)
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.
Ok. Done.
Make it disabled by default. When TLSv1.3 is out of draft we can remove this option and have it enabled all the time.
20b6ad1
to
4be403c
Compare
New commit pushed to address the feedback. Also rebased and fixed a travis issue. |
Pushed. Thanks. |
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3022)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3022)
Make it disabled by default. When TLSv1.3 is out of draft we can remove this option and have it enabled all the time. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3022)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3022)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3022)
Checklist
Description of change
This PR implements the TLSv1.3 downgrade protection mechanism.
For now it is switched off by default (enabled via "enable-tls13downgrade"). This is because the current draft says pre-RFC implementations SHOULD NOT implement it (if a pre-RFC implementation connects to an RFC implementation it will mistakenly detect a downgrade and abort the connection).
As soon as the draft becomes an RFC we can enable it by default and remove the "enable-tls13downgrade" option completely.