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 <6730974+andozw@users.noreply.github.com>

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.

real-or-random · 2024-02-22T18:31:13Z

src/modules/ecdh/tests_impl.h

@@ -7,6 +7,14 @@
 #ifndef SECP256K1_MODULE_ECDH_TESTS_H
 #define SECP256K1_MODULE_ECDH_TESTS_H

+static int ecdh_hash_function_test_xpassthru(unsigned char *output, const unsigned char *x, const unsigned char *y, void *data) {
+    (void)x;


Suggested change

(void)x;

You use x below.

(just a random comment, I haven't really reviewed the code so far)

Thanks, addressed

real-or-random

Concept ACK

real-or-random · 2024-04-19T14:07:16Z

src/modules/ecdh/tests_impl.h

+        expected_result = testvectors[t].expected_result;
+
+        /* fail if public key is valid, but doesn't parse */
+        CHECK(parsed_ok || expected_result == 0);


I think this should be tighter:

Suggested change

CHECK(parsed_ok || expected_result == 0);

CHECK(parsed_ok == expected_result);

The missing case is parsed_ok == 1 and expected_result == 0. If I'm not mistaken, then the entire test doesn't fail currently:

the CHECK here passes

we don't continue

but then the CHECK(secp256k1_memcmp_var)...passes vacuously becausetestvectors[t].shared_len == 0`

Thanks, addressed

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.

real-or-random · 2024-04-19T14:18:31Z

src/modules/ecdh/tests_impl.h

+        const unsigned char *expected_shared_secret;
+        unsigned char output_ecdh[65] = { 0 };
+
+        int expected_result;


Can we rename this to expected_parses or similar? result is a bit imprecise.

I agree result is a bit imprecise, but it's what wycheproof uses in their test vectors (and it is used for different purposes all together, so it seems a bit hard to fix this for them...). Let me know if this is not acceptable, happy to change it to something else. We use it for "expected parses" but also "expected ECDH function outcome".

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#.P._.@-.s/... 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.

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 <6730974+andozw@users.noreply.github.com>

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!

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 <6730974+andozw@users.noreply.github.com>

real-or-random reviewed Feb 22, 2024

View reviewed changes

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 <6730974+andozw@users.noreply.github.com>

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

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 Feb 22, 2024 •

edited

Loading

RandomLattice Oct 17, 2024

real-or-random left a comment

real-or-random Apr 19, 2024

RandomLattice Oct 17, 2024

real-or-random Apr 19, 2024

RandomLattice Oct 17, 2024

real-or-random Apr 19, 2024

RandomLattice Oct 17, 2024

real-or-random Apr 19, 2024

RandomLattice Oct 17, 2024

RandomLattice commented Apr 30, 2024

RandomLattice commented Oct 17, 2024 •

edited

Loading

	CHECK(parsed_ok \|\| expected_result == 0);
	CHECK(parsed_ok == expected_result);

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 Feb 22, 2024 • edited Loading

Choose a reason for hiding this 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

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

real-or-random Feb 22, 2024 •

edited

Loading

RandomLattice commented Oct 17, 2024 •

edited

Loading