Wycheproof: Fix incorrect assertion for invalid encaps/decaps tests#1636
Open
mkannwischer wants to merge 1 commit intomainfrom
Open
Wycheproof: Fix incorrect assertion for invalid encaps/decaps tests#1636mkannwischer wants to merge 1 commit intomainfrom
mkannwischer wants to merge 1 commit intomainfrom
Conversation
f2e623f to
a87dddf
Compare
Contributor
CBMC Results (ML-KEM-512)Full Results (187 proofs)
|
Contributor
CBMC Results (ML-KEM-768)Full Results (187 proofs)
|
Contributor
CBMC Results (ML-KEM-1024)Full Results (187 proofs)
|
The encaps and combined test functions asserted that K should NOT match the expected value for invalid test cases where the binary succeeded. This is wrong: for invalid inputs, the binary should reject the input via input validation, not succeed and produce a different K. Fix this by restructuring all test functions to branch on tc["result"] first, then verify the binary's behavior: - "invalid": assert the binary rejected the input (_error or decode_error) - "valid": assert outputs match expected values - anything else: fail with unexpected result Additional changes: - Remove "acceptable" as a result type (not used in any test vectors) - In keygen_seed tests, only "valid" results exist; simplify accordingly - In semi_expanded_decaps tests, assert that invalid tests are rejected rather than silently ignoring errors on valid tests - In combined tests, distinguish keygen decode errors from runtime errors, and always verify ek when keygen succeeds Signed-off-by: Matthias J. Kannwischer <matthias@zerorisc.com>
a87dddf to
9a37576
Compare
hanno-becker
reviewed
Apr 4, 2026
| tc["result"] == "invalid" | ||
| ), f"binary error on non-invalid tcId={tc['tcId']}" | ||
| elif tc["result"] in ("valid", "acceptable"): | ||
| "_error" in out or "decode_error" in out |
Contributor
There was a problem hiding this comment.
What is the "_error" branch supposed to catch in addition to decode_error?
hanno-becker
reviewed
Apr 4, 2026
| out["K"].upper() != tc["K"].upper() | ||
| ), f"K should not match for invalid tcId={tc['tcId']}" | ||
| False | ||
| ), f"Unexpected result '{tc['result']}' for tcId={tc['tcId']}" |
Contributor
There was a problem hiding this comment.
This is slightly misleading. "Unexpected result" to me suggests that something was wrong with the client's output -- but instead, we simply don't understand the test case?
hanno-becker
reviewed
Apr 4, 2026
| out["K"].upper() != tc["K"].upper() | ||
| ), f"K should not match for invalid tcId={tc['tcId']}" | ||
| False | ||
| ), f"Unexpected result '{tc['result']}' for tcId={tc['tcId']}" |
Contributor
There was a problem hiding this comment.
Suggested change
| ), f"Unexpected result '{tc['result']}' for tcId={tc['tcId']}" | |
| ), f"Unsupported test result '{tc['result']}' for tcId={tc['tcId']}" |
hanno-becker
requested changes
Apr 4, 2026
Contributor
hanno-becker
left a comment
There was a problem hiding this comment.
One nit, otherwise it looks good. Thank you very much for catching this!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The encaps and combined test functions asserted that K should NOT match the expected value for invalid test cases where the binary succeeded. This is wrong: for invalid inputs, the binary should reject the input via input validation, not succeed and produce a different K.
Fix this by restructuring all test functions to branch on tc["result"] first, then verify the binary's behavior:
Additional changes: