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/elliptic: automatically upgrade CurveParams for known curves and deprecate custom ones #34648

Open
FiloSottile opened this issue Oct 1, 2019 · 10 comments

Comments

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Oct 1, 2019

The generic implementation of elliptic.Curve provided by CurveParams is slow and insecure. No one would want to use it for a standard curve for which a constant-time, optimized implementation is available, like P256().

However, it's an easy mistake to replace a P256() value with P256().CurveParams() as they have exactly the same type. I say we just take away this footgun and redirect methods of CurveParams to the optimized curve implementation whenever the parameters match a known curve. That won't be extremely fast, but still faster than actually using the generic implementation.

Moreover, I think CurveParams in general was a mistake, and no one should be using it for custom curves either. For example, it's not constant time, and will never be, and we are not going to spend resources making it faster. Given it should be used for neither custom nor standard curves, I say we deprecate the CurveParams methods outright.

@FiloSottile FiloSottile added this to the Go1.14 milestone Oct 1, 2019
@ericlagergren
Copy link
Contributor

@ericlagergren ericlagergren commented Oct 2, 2019

To clarify: this is just deprecating the use of elliptic.CurveParam's methods, right?

The actual parameters (BitSize and Name, in particular) are useful.

@FiloSottile
Copy link
Contributor Author

@FiloSottile FiloSottile commented Oct 2, 2019

To clarify: this is just deprecating the use of elliptic.CurveParam's methods, right?

The actual parameters (BitSize and Name, in particular) are useful.

Yeah, definitely. It's about the implementation. (Although I wish we also didn't have the big.Int fields.)

@rsc rsc removed this from the Go1.14 milestone Oct 9, 2019
@rsc rsc added this to the Backlog milestone Oct 9, 2019
@FiloSottile FiloSottile changed the title crypto/elliptic: automatically upgrade CurveParams for known curves and deprecate custom ones proposal: crypto/elliptic: automatically upgrade CurveParams for known curves and deprecate custom ones Feb 5, 2020
@FiloSottile FiloSottile removed this from the Backlog milestone Feb 5, 2020
@FiloSottile FiloSottile added this to the Proposal milestone Feb 5, 2020
@FiloSottile
Copy link
Contributor Author

@FiloSottile FiloSottile commented Feb 5, 2020

Flagging as a proposal to discuss deprecating the generic, not constant time methods Add, Double, IsOnCurve, ScalarBaseMult and ScalarMult of CurveParams. Everyone should be instead using the methods on a named curve like P256().

@rsc rsc added this to Active in Proposals Feb 12, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Feb 12, 2020

Adding to proposal minutes, more discussion welcome.

@rsc
Copy link
Contributor

@rsc rsc commented Feb 26, 2020

@agl added this API, we should probably ask him his thoughts before deprecating it.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 4, 2020

Ping @agl?

@agl
Copy link
Contributor

@agl agl commented Mar 5, 2020

Burn it with fire (if you wish). I.e. I agree with Filippo that it would be nice and that the decision should come down to whether we can do it within the constraints of the Go standard library.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 11, 2020

Alrighty then! Based on the discussion above, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals Mar 11, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Mar 25, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Mar 25, 2020
@FiloSottile FiloSottile removed this from the Proposal milestone Mar 26, 2020
@FiloSottile FiloSottile added this to the Backlog milestone Mar 26, 2020
@FiloSottile FiloSottile changed the title proposal: crypto/elliptic: automatically upgrade CurveParams for known curves and deprecate custom ones crypto/elliptic: automatically upgrade CurveParams for known curves and deprecate custom ones Mar 26, 2020
@FiloSottile FiloSottile self-assigned this Mar 31, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented May 14, 2020

Change https://golang.org/cl/233939 mentions this issue: crypto/elliptic: upgrade from generic curve impl to specific if available

gopherbot pushed a commit that referenced this issue May 10, 2021
…able

This change alters the CurveParam methods to upgrade from the generic
curve implementation to the specific P224 or P256 implementations when
called on the embedded CurveParams. This removes the trap of using
elliptic.P224().Params() instead of elliptic.P224(), for example, which
results in using the generic implementation instead of the optimized
constant time one. For P224 this is done for all of the CurveParams
methods, except Params, as the optimized implementation covers all
these methods. For P256 this is only done for ScalarMult and
ScalarBaseMult, as despite having implementations of addition and
doubling they aren't exposed and instead the generic implementation is
used. For P256 an additional check that there actually is a specific
implementation is added, as unlike the P224 implementation the P256 one
is only available on certain platforms.

This change takes the simple, fast approach to checking this, it simply
compares pointers. This removes the most obvious class of mistakes
people make, but still allows edge cases where the embedded CurveParams
pointer has been dereferenced (as seen in the unit tests) or when someone
has manually constructed their own CurveParams that matches one of the
standard curves. A more complex approach could be taken to also address
these cases, but it would require directly comparing all of the
CurveParam fields which would, in the worst case, require comparing
against two standard CurveParam sets in the ScalarMult and
ScalarBaseMult paths, which are likely to be the hottest already.

Updates #34648

Change-Id: I82d752f979260394632905c15ffe4f65f4ffa376
Reviewed-on: https://go-review.googlesource.com/c/go/+/233939
Trust: Roland Shoemaker <roland@golang.org>
Trust: Katie Hockman <katie@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
@FiloSottile FiloSottile removed this from the Backlog milestone Sep 23, 2021
@FiloSottile FiloSottile added this to the Go1.18 milestone Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants