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

Closed
Tracked by #52182
FiloSottile opened this issue Oct 1, 2019 · 15 comments
Assignees
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@FiloSottile
Copy link
Contributor

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 the NeedsFix The path to resolution is known, but the work has not been done. label Oct 1, 2019
@FiloSottile FiloSottile added this to the Go1.14 milestone Oct 1, 2019
@ericlagergren
Copy link
Contributor

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

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 modified the milestones: Go1.14, Backlog 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 the NeedsFix The path to resolution is known, but the work has not been done. label Feb 5, 2020
@FiloSottile FiloSottile modified the milestones: Backlog, Proposal Feb 5, 2020
@FiloSottile FiloSottile added the Proposal-Crypto Proposal related to crypto packages or other security issues label Feb 5, 2020
@FiloSottile
Copy link
Contributor Author

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 (old) Feb 12, 2020
@rsc
Copy link
Contributor

rsc commented Feb 12, 2020

Adding to proposal minutes, more discussion welcome.

@rsc
Copy link
Contributor

rsc commented Feb 26, 2020

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

@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

Ping @agl?

@agl
Copy link
Contributor

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 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 (old) Mar 11, 2020
@rsc
Copy link
Contributor

rsc commented Mar 25, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Mar 25, 2020
@FiloSottile FiloSottile modified the milestones: Proposal, Backlog 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 added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 26, 2020
@FiloSottile FiloSottile self-assigned this Mar 31, 2020
@gopherbot
Copy link

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 modified the milestones: Backlog, Go1.18 Sep 23, 2021
@ianlancetaylor
Copy link
Contributor

@FiloSottile Is this likely to happen for 1.18? Thanks.

@ianlancetaylor
Copy link
Contributor

This does not seem to have happened for 1.18. Moving milestone to 1.19.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.18, Go1.19 Jan 28, 2022
@FiloSottile
Copy link
Contributor Author

Most of the work is done, only thing left is the deprecation.

@gopherbot
Copy link

Change https://go.dev/cl/398914 mentions this issue: crypto/ecdh: new package

@gopherbot gopherbot modified the milestones: Go1.19, Go1.20 Aug 2, 2022
@gopherbot
Copy link

Change https://go.dev/cl/459977 mentions this issue: crypto/elliptic: re-apply some deprecation markers

gopherbot pushed a commit that referenced this issue Mar 16, 2023
Per the updated go.dev/wiki/Deprecated, those APIs replaced by
crypto/ecdh (added in Go 1.20) can now be marked as deprecated
in Go 1.21.

Updates #52221
Updates #34648

Change-Id: Id0e11d7faa3a58a1716ce1ec6e2fff97bab96259
Reviewed-on: https://go-review.googlesource.com/c/go/+/459977
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Jan 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Development

No branches or pull requests

6 participants