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

Strip leading zeros in DHE premaster secret #201

Closed
wants to merge 0 commits into from
Closed

Strip leading zeros in DHE premaster secret #201

wants to merge 0 commits into from

Conversation

ocheron
Copy link
Contributor

@ocheron ocheron commented Mar 16, 2017

This is a remplacement for #197, in order to fix DHE without changing ECDHE.

Fixes an infrequent and random BadRecordMac failure during handshake, when DHE is selected and the premaster secret begins with a 0x00 byte. It was not visible in the test suite as both server and client code performed the same erroneous computation.

@ocheron
Copy link
Contributor Author

ocheron commented Mar 16, 2017

@kazu-yamamoto I've changed directly dhGetShared after making sure you don't
use it in TLS 1.3 code. In your patch I wonder if groupGetShared could use ScrubbedBytes.

where
-- strips leading zeros from the result of DH.getShared, as required
-- for DH(E) premaster secret in SSL/TLS before version 1.3.
stripLeadingZeros (DH.SharedKey sb) = DH.SharedKey (snd $ B.span (== 0) sb)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you should use dropWhile instead of span.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ordinarily yes, but here it's Data.ByteArray from memory and there is no dropWhile.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. OK.

@kazu-yamamoto
Copy link
Collaborator

Thank you for your kindness but you don't have to check my tls13 branch.
I will catch up the changes by myself if necessary.

@kazu-yamamoto kazu-yamamoto self-requested a review March 19, 2017 08:47
Copy link
Collaborator

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

ocheron added a commit that referenced this pull request Mar 19, 2017
Strip leading zeros in DHE premaster secret
@ocheron ocheron closed this Mar 19, 2017
@ocheron
Copy link
Contributor Author

ocheron commented Mar 19, 2017

Merged, thanks for reviewing.

I don't plan to look at TLS 1.3 very often, only on occasions. I still have many things to learn on the previous versions. Here the tlswg PR was helpful to understand why a difference between DH/ECDH.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants