Skip to content

Commit

Permalink
fix(ECDH): wrong test attribute in Wycheproof test
Browse files Browse the repository at this point in the history
For context, testing an ECDH implementation with Wycheproof vectors led
to an AttributeError on line 420 that used test.secret instead of
test.private.

Fixing this typo uncovered two additional bugs in
test_exchange_wycheproof regarding the correct interpretation of the
test result.

This showed a problem with the test that should cover that function.
The example wrapper used P-192, but Wycheproof does not provide vectors
for that curve. And a bug in run_wrapper_python meant that Wycheproof
vectors were disabled anyway.

As such, the wrapper example now uses P-256 and the bug is fixed, so the
function is properly covered and verifies that the fixes applied are
working.

Co-authored-by: Baptistin BOILOT <[email protected]>
Issue: #11
  • Loading branch information
JulioLoayzaM and bboilot-ledger committed Aug 22, 2024
1 parent 1af9286 commit 8320432
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 9 deletions.
11 changes: 7 additions & 4 deletions crypto_condor/primitives/ECDH.py
Original file line number Diff line number Diff line change
Expand Up @@ -417,23 +417,26 @@ def test_exchange_wycheproof(ecdh: ECDH, curve: Curve) -> Results | None:
for test in group.tests:
test_type = TestType(test.result)
info = TestInfo.new(test.id, test_type, test.flags, test.comment)
secret = int.from_bytes(test.secret, "big")
secret = int.from_bytes(test.private, "big")
data = EcdhWycheproofData(secret, test.public, test.shared)
try:
shared = ecdh.exchange_wycheproof(secret, test.public)
except NotImplementedError:
logger.info("Method not implemented, test skipped")
return None
except Exception as error:
info.fail(f"Exchange failed: {str(error)}", data)
if test_type == TestType.VALID:
info.fail(f"Exchange failed: {str(error)}", data)
else:
info.ok(data)
logger.debug("Exception raised by exchange_wycheproof", exc_info=True)
results.add(info)
continue
data.result = shared
res = shared == test.shared
data.result = shared
match (test_type, res):
case (TestType.VALID, True, TestType.INVALID, False):
case (TestType.VALID, True) | (TestType.INVALID, False):
info.ok(data)
case (TestType.VALID, False):
info.fail("Implementation returned wrong shared secret", data)
Expand Down Expand Up @@ -569,7 +572,7 @@ def run_wrapper_python(
logger.debug("Reloading ECDH wrapper module %s", wrapper.stem)
ecdh_wrapper = importlib.reload(ecdh_wrapper)
ecdh = ecdh_wrapper.CC_ECDH()
return test_exchange(ecdh, curve)
return test_exchange(ecdh, curve, compliance=compliance, resilience=resilience)


def run_wrapper(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
"""Wrapper example 1: PyCryptodome ECDH over P-192.
"""Wrapper example 1: PyCryptodome ECDH over P-256.
Usage:
This example implements both methods of the CC_ECDH class to test the PyCryptodome
Expand All @@ -19,9 +19,9 @@ def exchange_nist(
) -> bytes:
"""ECDH exchange with NIST vectors."""
# We can directly create the public key from the coordinates.
pub = ECC.construct(curve="P-192", point_x=pub_x, point_y=pub_y)
pub = ECC.construct(curve="P-256", point_x=pub_x, point_y=pub_y)
# Same goes for the private key and the private value.
priv = ECC.construct(curve="P-192", d=secret)
priv = ECC.construct(curve="P-256", d=secret)
# We run the key agreement with a KDF (key derivation function) that does
# nothing, as crypto-condor expects the "raw" shared secret.
shared = DH.key_agreement(static_priv=priv, static_pub=pub, kdf=lambda x: x)
Expand All @@ -33,7 +33,7 @@ def exchange_wycheproof(self, secret: int, pub_key: bytes) -> bytes:
# included in the key itself.
pub = ECC.import_key(pub_key)
# No difference for the private key.
priv = ECC.construct(curve="P-192", d=secret)
priv = ECC.construct(curve="P-256", d=secret)
# Also no change for the exchange.
shared = DH.key_agreement(static_priv=priv, static_pub=pub, kdf=lambda x: x)
return shared
2 changes: 1 addition & 1 deletion tests/cli/test_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ class TestECDH:

@pytest.mark.parametrize(
("lang", "example", "curve"),
[("Python", "1", "P-192"), ("Python", "2", "P-192")],
[("Python", "1", "P-256"), ("Python", "2", "P-192")],
)
def test_examples(self, lang: str, example: str, curve: str, tmp_path: Path):
"""Tests ECDH examples."""
Expand Down

0 comments on commit 8320432

Please sign in to comment.