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.

Sign up for GitHub

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

Jump to bottom

Add support for secp256k1 elliptic curve #457

Merged
merged 6 commits into from
Apr 13, 2022
Merged

Add support for secp256k1 elliptic curve #457

merged 6 commits into from
Apr 13, 2022

Conversation

dkostic
Copy link
Contributor

@dkostic dkostic commented Apr 11, 2022 •
edited
Loading

Issues:

CryptoAlg-1024

Description of changes:

The change adds support for secp256k1 elliptic curve (required by ACCP).
We use the generic |EC_GFp_mont_method| for the new curve.

The largest part of the change is adding tests to cover as much as
possible the potential use of the curve. We have added EC, ECDH, and
ECDSA tests. In all three cases we generate the test vectors by newly
added Go scripts. The standard Go library doesn't have support for
secp256k1, and moreover, we can't instantiate the curve on our own
because Go's implementation assumes a curve given by y^2 = x^3 + ax + b
has a = -3, which is not the case for secp256k1. We work around this
issue by using the most widely used secp256k1 Go implementation,
Ethereum's go-ethereum/crypto/secp256k1 module. Also, EVP Wycheproof
tests specific to secp256k1 were added.

A small issue with FIPS service indicator test for ECDH is also fixed.
The test assumed in one case that the indicator should always return
APPROVED, when in reality the indicator status should depened on which
elliptic curve is used in the protocol. For example, when a non FIPS
approved curve like secp256k1 is used, we expect the indicator to
return NOT_APPROVED.

Call-outs:

Point out areas that need special attention or support during the review process. Discuss architecture or design changes.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@nebeid nebeid requested review from nebeid and torben-hansen April 11, 2022 17:47
torben-hansen
torben-hansen reviewed Apr 12, 2022
View reviewed changes
Copy link
Contributor

@torben-hansen torben-hansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

I see some missing tests in crypto/fipsmodule/ec/ec_test.cc.

Also check out crypto/evp_extra/evp_test.cc. Could potentially also add some p256k1-specific tests there?

crypto/ecdh_extra/make_secp256k1_test_vectors.go Show resolved Hide resolved
crypto/fipsmodule/ec/make_ec_scalar_base_mult_tests.go Outdated Show resolved Hide resolved
crypto/fipsmodule/ec/make_ec_scalar_base_mult_tests.go Outdated Show resolved Hide resolved
dkostic
dkostic commented Apr 12, 2022
View reviewed changes
Copy link
Contributor Author

@dkostic dkostic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes are ported from this feature branch: https://github.com/awslabs/aws-lc/tree/add_secp192r1_secp256k1. Since we don't need to support P-192 curve I ported only changes relevant to secp256k1.

nebeid
nebeid reviewed Apr 12, 2022
View reviewed changes
Copy link
Contributor

@nebeid nebeid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious: why EC_curve_name2nid and EC_curve_nid2name are no longer needed?

crypto/fipsmodule/ecdsa/ecdsa_sign_tests.txt
@@ -2273,3 +2273,678 @@ Digest = fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
K = 00938d2f6550a46fb07b058e6287f428f0ff12aa6732a666d4a6cf2dd7cd8023ca76d0ce4e16b62830d0ff9e2fab9987261f3f3ffe0749ff70950d91b897d57007b2
R = 00ec0b91fa4386a8acdc0e46dd9c1d1775abbe0da8ead424aa4ace58e284a5be00e2c1ef95b6f4d861615564e1e7305656567f95275ce63b534420eae77ec37492c2
S = 01e1099fb389db498ab4cf23b4f06a74b9326878ae3c76ea13832e50702b30fe8303093a59cc9a0995f1dfc15e6f7dabca8a2acaf03ec005447d29fb429a252064ec

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a reference here to where the tests were generated from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, also in ecdsa_verify_tests.txt

crypto/fipsmodule/ecdsa/ecdsa_test.cc
@@ -192,6 +192,7 @@ TEST(ECDSATest, BuiltinCurves) {
{ NID_secp384r1, "secp384r1" },
{ NID_secp521r1, "secp521r1" },
{ NID_secp160r1, "secp160r1" },
{ NID_secp256k1, "secp256k1" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: In the order below, secp256k1 is checked for before secp160r1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to change this before I pushed the update. If you don't mind I'd let it be like this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

crypto/fipsmodule/ecdsa/ecdsa_verify_tests.txt Show resolved Hide resolved
@dkostic
Copy link
Contributor Author

dkostic commented Apr 13, 2022

I see some missing tests in crypto/fipsmodule/ec/ec_test.cc.
Also check out crypto/evp_extra/evp_test.cc. Could potentially also add some p256k1-specific tests there?

@torben-hansen Added more tests both in ec_test.cc and evp_test.cc. Let me know if I missed anything.

Just curious: why EC_curve_name2nid and EC_curve_nid2name are no longer needed?

@nebeid They were not needed in the first place, I realized that only now.

nebeid
nebeid reviewed Apr 13, 2022
View reviewed changes
crypto/fipsmodule/ec/ec_test.cc
@@ -378,6 +378,48 @@ static const uint8_t kP521PublicKey_compressed_0x03[] = {
0x9f, 0x5f, 0xb4, 0xf8, 0xe7, 0x7b
};

static const uint8_t ksecp256k1PublicKey_uncompressed_0x02[] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How were the new tests generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used Sage to generate a random point :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity, the Sage code to generate a random point on secp256k1. The setup of the curve is taken from this very useful webpage (that covers many other curves as well): https://neuromancer.sk/std/secg/secp256k1#

p = 0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc2f
K = GF(p)
a = K(0x0000000000000000000000000000000000000000000000000000000000000000)
b = K(0x0000000000000000000000000000000000000000000000000000000000000007)
E = EllipticCurve(K, (a, b))
G = E(0x79be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798, 0x483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8)
E.set_order(0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141 * 0x1)

(x, y, z) = E.random_element()

@dkostic dkostic merged commit 98da1e3 into aws:main Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nebeid nebeid nebeid approved these changes

@torben-hansen torben-hansen torben-hansen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dkostic @torben-hansen @nebeid