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

Correct descriptions of M1 & M2 per RFC 5054 #163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cipherboy
Copy link

@cipherboy cipherboy commented Feb 7, 2024

As far as I can tell from the code, what is defined as:

key = compute_premaster_secret(...)

does not include the given hash invocation step. While RFC 5054 is unclearly worded (see comment below), SRP-6 is clear that M1 should not include K = H(S) and thus this description of the protocol is incorrect. As far as I can tell, nobody else uses this computation of M1 (with K = H(S) as a parameter) and thus it should be dropped from the tabular and comment descriptions.

See also: bcgit/bc-csharp#506 (comment)


Edit to add: regardless of which spec is deemed correct or supported or implemented (SPR-6 or RFC 2945) -- it is clear that M1 = H(A, B, H(S)) is not aligning with either of those two, and also (I believe) wasn't implemented in the code.

As far as I can tell from the code, what is defined as:

 > key = compute_premaster_secret(...)

does not include the given hash invocation step. While RFC 5054 is
unclearly worded (see comment below), SRP-6 is clear that M1 should
not include K = H(S) and thus this description of the protocol is
incorrect. As far as I can tell, nobody else uses this computation
of M1 (with K = H(S) as a parameter) and thus it should be dropped from
the tabular and comment descriptions.

See also: bcgit/bc-csharp#506 (comment)

Signed-off-by: Alexander Scheel <alexander.scheel@keyfactor.com>
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.

1 participant