-
Notifications
You must be signed in to change notification settings - Fork 87
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
Downgrade protection #308
Downgrade protection #308
Conversation
cf20d00
to
d6f37ec
Compare
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 like the new module comparing chosen vs supported very much.
As for testing, the debug Maybe is good enough. It would be even more impressive to see an actual MITM implemented with something like hookRecvHandshake(13), changing not only the server version but also trying to manipulate the server random.
let debug' = (serverDebug sparam) { debugVersion = Just TLS12 } | ||
sparam' = sparam { serverDebug = debug' } | ||
runTLSInitFailure (cparam,sparam') | ||
|
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.
Since this is tested 100 times with QuickCheck, can we add other downgrade scenarios as well as version combinations with no downgrade?
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 added the test case downgrading from TSL 1.3 to TLS 1.1.
If we can use arbitraryPairParams
instead of arbitraryPairParams13
and guess the negotiated version, we can cover downgrade senario (TLS 1.2 -> TLS 1.1). In this case, mixing no downgrade make sense to me.
Unfortunately, I have no idea on how to tell the negotiated version. So, this test covers downgrade senarios only.
To implement this, |
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 for merge.
8a892a7
to
6e8394d
Compare
Rebased, pushed -f to put the merge mark and merged. |
This implements downgrade protection defined in TLS 1.3.