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: ECDSA parameters should be validated. #22210

AnomalRoil opened this issue Oct 11, 2017 · 2 comments


Copy link

commented Oct 11, 2017

What version of Go are you using (go version)?

go1.9.1 linux/amd64

What operating system and processor architecture are you using (go env)?

GOHOSTARCH="amd64" (intel)

What did you do?

Playground link:

I've been playing around with Go's ECDSA package and have noticed a few problems, none of practical security relevance, since those problems have premises unlikely to happen in real use cases. (I actually discussed those on Go's security mailing list a long time ago.)

The problems are all about parameters validation in ECDSA.

What did you expect to see?

The Go's implementation should conform to the DSS standard, notably by performing verification that the point used are actually on the curve at hand.
But if the checks I discuss here are considered not worthy, a workaround like what has been done for DSA might be good to completely avoid any risk of infinite loops.

I would expect a method to check the validity of the provided parameters, as per FIPS 186-4 sections 3.1 and 3.3.

What did you see instead?

  • No check is performed on the curve's points used to see whether they are on the curve or not, so it is possible to use a point not even on the curve. (Whether this is dangerous for ECDSA has not been studied.)
    Note that checks that points are on the curve are performed in the TLS package (where they are important, because of the invalid curve attack) thanks to the elliptic.Unmarshal method.

  • I also see that no check is performed on the private parameter "d" to verify if it's within well defined bounds. So it is possible to sign using a 0 value, which is not among the correct boundaries which are [1, n-1] (even if the value 1 does not make much sense from a security point of view).
    There is an example on the playground linked where this causes an infinite loop when signing an null hash with a null private integer.
    The value 0 is simply invalid as a private integer and should be rejected.

  • There are no "validation" methods for keys, this could be a good way to avoid performing such checks at each signature/verification: by providing a method users are supposed to use to validate the keys.

Such checks are important in my opinion and are part of the so-called "defense in depth" we want to generally ensure in a crypto library.

What are your opinions on these topics?


This comment has been minimized.

Copy link

commented Oct 11, 2017

@odeke-em odeke-em added the Security label Oct 12, 2017
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 29, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 23, 2018
@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019

This comment has been minimized.

Copy link

commented Sep 30, 2019

We should refuse to Sign and Verify with invalid keys, as long as the checks are not too expensive compared to the rest of the operation. I'd take a PR with some benchstat for this.

@FiloSottile FiloSottile removed their assignment Sep 30, 2019
@FiloSottile FiloSottile modified the milestones: Go1.14, Unplanned Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
5 participants
You can’t perform that action at this time.