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

crypto/ecdsa: Source comment contradicts code #16819

Closed
pyramids opened this issue Aug 21, 2016 · 2 comments
Closed

crypto/ecdsa: Source comment contradicts code #16819

pyramids opened this issue Aug 21, 2016 · 2 comments

Comments

@pyramids
Copy link

pyramids commented Aug 21, 2016

  1. What did you expect to see?
    A match between comments and code in https://github.com/golang/go/blob/master/src/crypto/ecdsa/ecdsa.go#L152.

  2. What did you see instead?
    https://github.com/golang/go/blob/master/src/crypto/ecdsa/ecdsa.go#L152: "// Get max(log2(q) / 2, 256) bits of entropy from rand."

    https://github.com/golang/go/blob/master/src/crypto/ecdsa/ecdsa.go#L154 proceeds to calculate an entropylen of approximately the number of bytes in min(log2(q) / 2, 256), which differs from the comment describing the intention by selecting the minimum, not the maximum.

Which one is the correct amount of entropy? The code seems suspicious by limiting the entropy and hence security level to 256 bits maximum even if a curve with larger potential is used, but it could be motivated by the hash used, SHA512 chopped to 256 bits, which does already impose a limit to the maximum security level.

Is the comment correct? Since I don't understand the division by two (q is the size of the underlying field in bits and hence the amount of entropy I would expect to want to use), I cannot answer this question and hence cannot propose a fix such as simply changing the code to match the comment or vice versa.

It is possible that the motivating idea is that the security level is half the size of the curve's underlying field in bits, and that no more entropy than that makes sense. If correct, then the code makes sense and the comment should be changed to reflect what the code does.

pyramids added a commit to pyramids/go that referenced this issue Aug 22, 2016
This is a minimal fix of consistency only, by changing the comment to reflect what the code already does.  I did not check the choice of using only half the curve's field size (in bits) of entropy.
@quentinmit
Copy link
Contributor

/cc @bradfitz @agl

@quentinmit quentinmit added this to the Go1.8Maybe milestone Aug 22, 2016
@agl agl self-assigned this Aug 23, 2016
@gopherbot
Copy link

CL https://golang.org/cl/30153 mentions this issue.

@golang golang locked and limited conversation to collaborators Oct 2, 2017
@rsc rsc unassigned agl Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants