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

tests: Add Wycheproof ECDH vectors #1492

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

tests: Add Wycheproof ECDH vectors #1492

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f26c323
tests: Add Wycheproof ECDH vectors
RandomLattice Feb 3, 2024
3cba981
PR feedback: add more wycheproof ECDH test cases
RandomLattice Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,9 +225,17 @@ maintainer-clean-local: clean-precomp
### (see the comments in the previous section for detailed rationale)
TESTVECTORS = src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.h

if ENABLE_MODULE_ECDH
TESTVECTORS += src/wycheproof/ecdh_secp256k1_test.h
endif

src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.h:
mkdir -p $(@D)
python3 $(top_srcdir)/tools/tests_wycheproof_generate.py $(top_srcdir)/src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.json > $@
python3 $(top_srcdir)/tools/tests_wycheproof_generate_ecdsa.py $(top_srcdir)/src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.json > $@

src/wycheproof/ecdh_secp256k1_test.h:
mkdir -p $(@D)
python3 $(top_srcdir)/tools/tests_wycheproof_generate_ecdh.py $(top_srcdir)/src/wycheproof/ecdh_secp256k1_test.json > $@

testvectors: $(TESTVECTORS)

Expand All @@ -250,7 +258,9 @@ EXTRA_DIST += sage/secp256k1_params.sage
EXTRA_DIST += sage/weierstrass_prover.sage
EXTRA_DIST += src/wycheproof/WYCHEPROOF_COPYING
EXTRA_DIST += src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.json
EXTRA_DIST += tools/tests_wycheproof_generate.py
EXTRA_DIST += src/wycheproof/ecdh_secp256k1_test.json
EXTRA_DIST += tools/tests_wycheproof_generate_ecdsa.py
EXTRA_DIST += tools/tests_wycheproof_generate_ecdh.py

if ENABLE_MODULE_ECDH
include src/modules/ecdh/Makefile.am.include
Expand Down
1 change: 1 addition & 0 deletions src/modules/ecdh/Makefile.am.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ include_HEADERS += include/secp256k1_ecdh.h
noinst_HEADERS += src/modules/ecdh/main_impl.h
noinst_HEADERS += src/modules/ecdh/tests_impl.h
noinst_HEADERS += src/modules/ecdh/bench_impl.h
noinst_HEADERS += src/wycheproof/ecdh_secp256k1_test.h
50 changes: 50 additions & 0 deletions src/modules/ecdh/tests_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

@real-or-random real-or-random Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
(void)x;

You use x below.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, addressed

(void)y;
(void)data;
memcpy(output, x, 32);
return 1;
}

static int ecdh_hash_function_test_fail(unsigned char *output, const unsigned char *x, const unsigned char *y, void *data) {
(void)output;
(void)x;
Expand Down Expand Up @@ -142,11 +150,53 @@ static void test_result_basepoint(void) {
}
}

static void test_ecdh_wycheproof(void) {
#include "../../wycheproof/ecdh_secp256k1_test.h"
int t;
for (t = 0; t < SECP256K1_ECDH_WYCHEPROOF_NUMBER_TESTVECTORS; t++) {
int parsed_ok;
secp256k1_pubkey point;
const unsigned char *pk;
const unsigned char *sk;
const unsigned char *expected_shared_secret;
unsigned char output_ecdh[65] = { 0 };

int expected_result;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 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".

int actual;

memset(&point, 0, sizeof(point));
pk = &wycheproof_ecdh_public_keys[testvectors[t].pk_offset];
parsed_ok = secp256k1_ec_pubkey_parse(CTX, &point, pk, testvectors[t].pk_len);

expected_result = testvectors[t].expected_result;

/* fail if public key is valid, but doesn't parse */
CHECK(parsed_ok || expected_result == 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

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`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, addressed


if (!parsed_ok && expected_result == 0) {
continue;
}

sk = &wycheproof_ecdh_private_keys[testvectors[t].sk_offset];
CHECK(testvectors[t].sk_len == 32);

actual = secp256k1_ecdh(CTX, output_ecdh, &point, sk, ecdh_hash_function_test_xpassthru, NULL);
expected_shared_secret = &wycheproof_ecdh_shared_secrets[testvectors[t].shared_offset];

CHECK(actual == expected_result);
if (expected_result == 0) {
CHECK(testvectors[t].shared_len == 0);
}
CHECK(secp256k1_memcmp_var(output_ecdh, expected_shared_secret, testvectors[t].shared_len) == 0);
}
}

static void run_ecdh_tests(void) {
test_ecdh_api();
test_ecdh_generator_basepoint();
test_bad_scalar();
test_result_basepoint();
test_ecdh_wycheproof();
}

#endif /* SECP256K1_MODULE_ECDH_TESTS_H */
11 changes: 10 additions & 1 deletion src/wycheproof/WYCHEPROOF_COPYING
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,18 @@
`b063b4aedae951c69df014cd25fa6d69ae9e8cb9`, see
https://github.com/google/wycheproof/blob/b063b4aedae951c69df014cd25fa6d69ae9e8cb9/testvectors_v1/ecdsa_secp256k1_sha256_bitcoin_test.json

* 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


* The file `ecdsa_secp256k1_sha256_bitcoin_test.h` is generated from
`ecdsa_secp256k1_sha256_bitcoin_test.json` using the script
`tests_wycheproof_generate.py`.
`tests_wycheproof_generate_ecdsa.py`.

* The file `ecdh_secp256k1_test.h` is generated from
`ecdh_secp256k1_test.json` using the script
`tests_wycheproof_generate_ecdh.py`.

-------------------------------------------------------------------------------

Expand Down
Loading