tests: Add Wycheproof ECDH vectors #1492

RandomLattice · 2024-02-04T02:06:08Z

Adds a test for the ECDH module using the Wycheproof vectors as outlined in #1106.

This commit adds 479 ECDH test vectors. All test vectors pass. The vectors cover:

edge cases in the shared secret
edge cases in the ephemeral public keys
edge cases in arithmetic operations

We use a python script to convert the JSON-formatted vectors into C code, in the same spirit as #1245

Adds a test for the ECDH module using the Wycheproof vectors. We use a python script to convert the JSON-formatted vectors into C code, in the same spirit as bitcoin-core#1245 Co-authored-by: Sean Andersen <[email protected]>

real-or-random

Awesome, Concept ACK

I think most contributors here are currently busy with other projects, but we'll come back to this for sure.

src/modules/ecdh/tests_impl.h

real-or-random

Concept ACK

src/modules/ecdh/tests_impl.h

real-or-random · 2024-04-19T14:13:38Z

src/wycheproof/WYCHEPROOF_COPYING

+* The file `ecdh_secp256k1_test.json` in this directory
+  comes from Google's project Wycheproof with git commit
+  `d9f6ec7d8bd8c96da05368999094e4a75ba5cb3d`, see
+  https://github.com/google/wycheproof/blob/d9f6ec7d8bd8c96da05368999094e4a75ba5cb3d/testvectors_v1/ecdh_secp256k1_test.json


Wycheproof ownership was recently moved to C2SP (https://github.com/C2SP/wycheproof community maintenance), so this should be updated to the new URL.) See @FiloSottile's talk https://archive.org/details/oscw-2024-fillippo-valsorda-cryptographic-test-vectors for background.)

You could update the other URLs in a separate commit, and update the ECDSA vectors, see C2SP/wycheproof#91 (if you're willing to care of this in this PR).

Sure thing, we will open a concurrent PR to update this. I think it will be cleaner.

If you're still interested, then I think a proper version of the abandoned (?) #1638 could be included in this PR. It should be trivial.

src/modules/ecdh/tests_impl.h

real-or-random · 2024-04-19T14:47:43Z

tools/tests_wycheproof_generate_ecdh.py

+
+def should_skip(test_vector_flags):
+    # skip these vectors because they are for ASN.1 encoding issues and other curves
+    flags_to_skip = {"InvalidAsn", "InvalidCurveAttack", "InvalidEncoding", "WrongCurve", "UnnamedCurve"}


I'm not sure about all of these.

"InvalidAsn': ✔️

"InvalidCurveAttack". the json says "The point of the public key is not on the curve." -- shouldn't we have these? (How is this different from "InvalidPublic"? I assume the keys in case of "InvalidCurveAttack" are on some other curve.)

What is "InvalidEncoding"?

"WrongCurve": I really don't understand the JSON here. For example, test case 492 says: "public key has invalid point of order 2 on secp256r1. The point of the public key is a valid on secp256k1. ", but then says "invalid"?! Do you know what they have in mind?

"UnnamendCurve": Have you tried these? If we reject correctly, let's just include them? Again, I can't follow the JSON entirely. :/ For example, test case 511 has "public key of order 3" with "WeakPublicKey", "InvalidPublic", "UnnamedCurve". How can it be invalid and at the same time have an order? How can the order be 3 on our curve? (Shouldn't it have InvalidCurveAttack then? ...)

"InvalidCurveAttack". the json says "The point of the public key is not on the curve." -- shouldn't we have these? (How is this different from "InvalidPublic"? I assume the keys in case of "InvalidCurveAttack" are on some other curve.

You’re right – we should have these. We changed it so that we’re no longer skipping InvalidCurveAttack test vectors.

What is "InvalidEncoding"?

We are now including this test case.

"WrongCurve": I really don't understand the JSON here. For example, test case 492 says: "public key has invalid point of order 2 on secp256r1. The point of the public key is a valid on secp256k1. ", but then says "invalid"?! Do you know what they have in mind?

All these 20 cases of WrongCurve have a public key whose ASN.1 representation carries an OID for a different curve (not secp256k1). None of the libsecp256k1 code parses this ASN.1 structure so we are not including these in the test cases.

In that specific case (tcId 492) they encode public key bytes that in secp256r1 coincide with a point of order 2 but in secp256k1 is a valid point (is in the (prime) group). We are just skipping this since this confusion is at an abstraction level higher than libsecp256k1. Again, none of the libsecp256k1 code parses this ASN.1 where the confusion (may) happen.

"UnnamendCurve": Have you tried these? If we reject correctly, let's just include them?

These are now included.

test case 511 has "public key of order 3" with "WeakPublicKey", "InvalidPublic", "UnnamedCurve". How can it be invalid and at the same time have an order?

I guess their definition of invalid here is “does not lie in the proper subgroup”. This is consistent with the SEC (see §3.2.2.1 step 4 of https://www.secg.org/sec1-v2.pdf - ensures that the order or the point is large).

We are now skipping tcID 496, 497, 502, 503, 504, 505, 507. All these public keys have a custom ASN.1 encoding that explicitly encodes some curve parameters (including the order). Again, libsecp256k1 never parses these so we don’t care about these. In the tests we skip them.

For example, tcId 496 has the following public key:

openssl asn1parse -in 496.bin -i -inform DR -dump 0:d=0 hl=4 l= 307 cons: SEQUENCE 4:d=1 hl=3 l= 236 cons: SEQUENCE 7:d=2 hl=2 l= 7 prim: OBJECT :id-ecPublicKey 16:d=2 hl=3 l= 224 cons: SEQUENCE 19:d=3 hl=2 l= 1 prim: INTEGER :01 22:d=3 hl=2 l= 44 cons: SEQUENCE 24:d=4 hl=2 l= 7 prim: OBJECT :prime-field 33:d=4 hl=2 l= 33 prim: INTEGER :FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEFFFFFC2F 68:d=3 hl=2 l= 68 cons: SEQUENCE 70:d=4 hl=2 l= 32 prim: OCTET STRING 0000 - 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 ................ 0010 - 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 ................ 104:d=4 hl=2 l= 32 prim: OCTET STRING 0000 - 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 ................ 0010 - 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 07 ................ 138:d=3 hl=2 l= 65 prim: OCTET STRING 0000 - 04 79 be 66 7e f9 dc bb-ac 55 a0 62 95 ce 87 0b .y.f~....U.b.... 0010 - 07 02 9b fc db 2d ce 28-d9 59 f2 81 5b 16 f8 17 .....-.(.Y..[... 0020 - 98 48 3a da 77 26 a3 c4-65 5d a4 fb fc 0e 11 08 .H:.w&..e]...... 0030 - a8 fd 17 b4 48 a6 85 54-19 9c 47 d0 8f fb 10 d4 ....H..T..G..... 0040 - b8 . 205:d=3 hl=2 l= 33 prim: INTEGER :-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 240:d=3 hl=2 l= 1 prim: INTEGER :01 243:d=1 hl=2 l= 66 prim: BIT STRING 0000 - 00 04 49 c2 48 ed c6 59-e1 84 82 b7 10 57 48 a4 ..I.H..Y.....WH. 0010 - b9 5d 3a 46 95 2a 5b a7-2d a0 d7 02 dc 97 a6 4e .]:F.*[.-......N 0020 - 99 79 9d 8c ff 7a 5c 4b-92 5e 43 60 ec e2 5c cf .y...z\K.^C`..\. 0030 - 30 7d 7a 9a 70 63 28 6b-bd 16 ef 64 c6 5f 54 67 0}z.pc(k...d._Tg 0040 - 57 e2

For reference this is the ASN.1 encoding:

xxd 496.bin 00000000: 3082 0133 3081 ec06 072a 8648 ce3d 0201 0..30....*.H.=.. 00000010: 3081 e002 0101 302c 0607 2a86 48ce 3d01 0.....0,..*.H.=. 00000020: 0102 2100 ffff ffff ffff ffff ffff ffff ..!............. 00000030: ffff ffff ffff ffff ffff ffff ffff fffe ................ 00000040: ffff fc2f 3044 0420 0000 0000 0000 0000 .../0D. ........ 00000050: 0000 0000 0000 0000 0000 0000 0000 0000 ................ 00000060: 0000 0000 0000 0000 0420 0000 0000 0000 ......... ...... 00000070: 0000 0000 0000 0000 0000 0000 0000 0000 ................ 00000080: 0000 0000 0000 0000 0007 0441 0479 be66 ...........A.y.f 00000090: 7ef9 dcbb ac55 a062 95ce 870b 0702 9bfc ~....U.b........ 000000a0: db2d ce28 d959 f281 5b16 f817 9848 3ada .-.(.Y..[....H:. 000000b0: 7726 a3c4 655d a4fb fc0e 1108 a8fd 17b4 w&..e].......... 000000c0: 48a6 8554 199c 47d0 8ffb 10d4 b802 21ff H..T..G.......!. 000000d0: 0000 0000 0000 0000 0000 0000 0000 0001 ................ 000000e0: 4551 2319 50b7 5fc4 402d a173 2fc9 bebf EQ#[email protected]/... 000000f0: 0201 0103 4200 0449 c248 edc6 59e1 8482 ....B..I.H..Y... 00000100: b710 5748 a4b9 5d3a 4695 2a5b a72d a0d7 ..WH..]:F.*[.-.. 00000110: 02dc 97a6 4e99 799d 8cff 7a5c 4b92 5e43 ....N.y...z\K.^C 00000120: 60ec e25c cf30 7d7a 9a70 6328 6bbd 16ef `..\.0}z.pc(k... 00000130: 64c6 5f54 6757 e2 d._TgW.

Here the order of the curve is encoded as explicit parameter (which is the “wrong” order): -115792089237316195423570985008687907852837564279074904382605163141518161494337 or -FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141.

All other skipped cases are analogous.

Nice, thanks a lot for going through all of this!

RandomLattice · 2024-04-30T16:14:59Z

Thanks @real-or-random for the review. We're going to have a look and come back here soon to take care of this PR :-)

Co-authored-by: Sean Andersen <[email protected]>

RandomLattice · 2024-10-17T12:25:20Z

@real-or-random it took a bit longer than expected but here we are. We addressed the main points of the review in 3cba981, PTAL whenever you've a chance. Thanks!

RandomLattice · 2025-01-08T08:37:39Z

friendly ping to review this whenever you've a chance @real-or-random. Thanks!

RandomLattice · 2025-03-21T07:33:14Z

@real-or-random this is ready for review, wondering if we could have some eyes. Thank you so much in advance!

real-or-random

Nice! All I have is some nits, and I believe this is ready. It should perhaps get the eyes of another reviewer, but I don't think another in-depth review is necessary. In the end, this "just" adds tests.

Sorry, that this too so long.

real-or-random · 2025-03-24T07:36:43Z

src/modules/ecdh/tests_impl.h

+        expected_result = testvectors[t].expected_result;
+
+        CHECK(parsed_ok == expected_result);
+
+        if (!parsed_ok && expected_result == 0) {
+            continue;
+        }


nit:

Suggested change

expected_result = testvectors[t].expected_result;

CHECK(parsed_ok == expected_result);

if (!parsed_ok && expected_result == 0) {

continue;

}

expected_result = testvectors[t].expected_result;

CHECK(parsed_ok == expected_result);

if (!parsed_ok) {

continue;

}

Now this can be simplified, plus I think there's more vertical whitespace than common. :)

real-or-random · 2025-03-24T08:16:13Z

tools/tests_wycheproof_generate_ecdh.py

+for i in range(num_groups):
+    group = doc['testGroups'][i]


Suggested change

for i in range(num_groups):

group = doc['testGroups'][i]

for group in doc['testGroups']

And then we can remove num_groups = len(doc['testGroups'])

real-or-random · 2025-03-24T08:16:41Z

tools/tests_wycheproof_generate_ecdh.py

+    for j in range(num_tests):
+        test_vector = group['tests'][j]


Suggested change

for j in range(num_tests):

test_vector = group['tests'][j]

for test_vector in group['tests']:

And then we can remove num_tests = len(group['tests'])

real-or-random · 2025-03-24T08:18:39Z

tools/tests_wycheproof_generate_ecdh.py

+
+num_vectors = 0
+offset_sk_running, offset_pk_running, offset_shared = 0, 0, 0
+out = ""


out is a bit ambigous. test_vectors_out?

real-or-random · 2025-03-24T08:19:21Z

tools/tests_wycheproof_generate_ecdh.py

+
+def parse_public_key(pk):
+    der_pub_key = parse_der_pk(unhexlify(pk))   # Convert back to str and strip off the `0x`
+    return hexlify(der_pub_key).decode('utf-8')[2:]


Suggested change

return hexlify(der_pub_key).decode('utf-8')[2:]

return hexlify(der_pub_key).decode()[2:]

utf-8 is the default

real-or-random · 2025-03-24T08:27:27Z

tools/tests_wycheproof_generate_ecdh.py

+def normalize_private_key(sk):
+    # Ensure the private key is at most 64 characters long, retaining the last 64 if longer.
+    normalized = sk[-64:].zfill(64)
+    assert len(normalized) == 64, "private key must be exactly 64 characters long."


Suggested change

assert len(normalized) == 64, "private key must be exactly 64 characters long."

if len(normalized) != 64:

raise ValueError("private key must be exactly 64 characters long.")

(Don't rely on assertions being enabled)

real-or-random · 2025-03-24T08:29:22Z

tools/tests_wycheproof_generate_ecdh.py

+    return hexlify(der_pub_key).decode('utf-8')[2:]
+
+def normalize_private_key(sk):
+    # Ensure the private key is at most 64 characters long, retaining the last 64 if longer.


How could it ever be longer?

real-or-random · 2025-03-24T08:30:47Z

tools/tests_wycheproof_generate_ecdh.py

+
+def should_skip(test_vector_flags):
+    # skip these vectors because they are for ASN.1 encoding issues and other curves
+    flags_to_skip = {"InvalidAsn", "InvalidCurveAttack", "InvalidEncoding", "WrongCurve", "UnnamedCurve"}


Nice, thanks a lot for going through all of this!

real-or-random · 2025-03-24T08:32:22Z

tools/tests_wycheproof_generate_ecdh.py

+def should_skip_flags(test_vector_flags):
+    # skip these vectors because they are for ASN.1 encoding issues and other curves
+    flags_to_skip = {"InvalidAsn", "WrongCurve"}
+    return any(flag in test_vector_flags for flag in flags_to_skip)
+
+def should_skip_tcid(test_vector_tcid):
+    '''
+    We skip some test case IDs that have a public key whose custom ASN.1 representation explicitly
+    encodes some curve parameters that are invalid. libsecp256k1 never parses this part so we do
+    not care testing those. See https://github.com/bitcoin-core/secp256k1/pull/1492#discussion_r1572491546
+    '''
+    tcids_to_skip = [496, 497, 502, 503, 504, 505, 507]
+    return test_vector_tcid in tcids_to_skip


consistency: docstring vs comment. (I'd just stick to the comment).

real-or-random · 2025-03-24T08:33:43Z

tools/tests_wycheproof_generate_ecdh.py

+
+def should_skip_flags(test_vector_flags):
+    # skip these vectors because they are for ASN.1 encoding issues and other curves
+    flags_to_skip = {"InvalidAsn", "WrongCurve"}


A short description list of all the (unclear) categories would be nice. This has confused us, and I assume it will confuse others in the future. (Or just add another reference to #1492 (comment) )

tests: Add Wycheproof ECDH vectors

f26c323

Adds a test for the ECDH module using the Wycheproof vectors. We use a python script to convert the JSON-formatted vectors into C code, in the same spirit as bitcoin-core#1245 Co-authored-by: Sean Andersen <[email protected]>

real-or-random reviewed Feb 22, 2024

View reviewed changes

src/modules/ecdh/tests_impl.h Outdated Show resolved Hide resolved

real-or-random added the assurance label Feb 22, 2024

real-or-random reviewed Apr 19, 2024

View reviewed changes

RandomLattice force-pushed the wycheproof-ecdh branch from d039d8c to 4ccf267 Compare October 17, 2024 10:03

PR feedback: add more wycheproof ECDH test cases

3cba981

Co-authored-by: Sean Andersen <[email protected]>

RandomLattice force-pushed the wycheproof-ecdh branch from 4ccf267 to 3cba981 Compare October 17, 2024 10:06

RandomLattice requested a review from real-or-random January 8, 2025 08:36

real-or-random reviewed Mar 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: Add Wycheproof ECDH vectors #1492

tests: Add Wycheproof ECDH vectors #1492

RandomLattice commented Feb 4, 2024

real-or-random left a comment

real-or-random left a comment

real-or-random Apr 19, 2024

RandomLattice Oct 17, 2024

real-or-random Mar 24, 2025

real-or-random Apr 19, 2024

RandomLattice Oct 17, 2024

real-or-random Mar 24, 2025

RandomLattice commented Apr 30, 2024

RandomLattice commented Oct 17, 2024 •

edited

Loading

RandomLattice commented Jan 8, 2025

RandomLattice commented Mar 21, 2025

real-or-random left a comment

real-or-random Mar 24, 2025

real-or-random Mar 24, 2025

real-or-random Mar 24, 2025

real-or-random Mar 24, 2025

real-or-random Mar 24, 2025

real-or-random Mar 24, 2025

real-or-random Mar 24, 2025

real-or-random Mar 24, 2025

real-or-random Mar 24, 2025

real-or-random Mar 24, 2025

	for i in range(num_groups):
	group = doc['testGroups'][i]
	for group in doc['testGroups']

	for j in range(num_tests):
	test_vector = group['tests'][j]
	for test_vector in group['tests']:

	return hexlify(der_pub_key).decode('utf-8')[2:]
	return hexlify(der_pub_key).decode()[2:]

	assert len(normalized) == 64, "private key must be exactly 64 characters long."
	if len(normalized) != 64:
	raise ValueError("private key must be exactly 64 characters long.")

tests: Add Wycheproof ECDH vectors #1492

Are you sure you want to change the base?

tests: Add Wycheproof ECDH vectors #1492

Conversation

RandomLattice commented Feb 4, 2024

real-or-random left a comment

Choose a reason for hiding this comment

real-or-random left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomLattice commented Apr 30, 2024

RandomLattice commented Oct 17, 2024 • edited Loading

RandomLattice commented Jan 8, 2025

RandomLattice commented Mar 21, 2025

real-or-random left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomLattice commented Oct 17, 2024 •

edited

Loading