Wycheproof: Fix incorrect assertion for invalid encaps/decaps tests#1636
Merged
mkannwischer merged 1 commit intomainfrom Apr 18, 2026
Merged
Wycheproof: Fix incorrect assertion for invalid encaps/decaps tests#1636mkannwischer merged 1 commit intomainfrom
mkannwischer merged 1 commit intomainfrom
Conversation
f2e623f to
a87dddf
Compare
Contributor
CBMC Results (ML-KEM-512)Full Results (191 proofs)
|
Contributor
CBMC Results (ML-KEM-768)Full Results (191 proofs)
|
Contributor
CBMC Results (ML-KEM-1024)Full Results (191 proofs)
|
a87dddf to
9a37576
Compare
hanno-becker
reviewed
Apr 4, 2026
hanno-becker
reviewed
Apr 4, 2026
hanno-becker
reviewed
Apr 4, 2026
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!
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>
9a37576 to
b98e73a
Compare
hanno-becker
approved these changes
Apr 18, 2026
Contributor
hanno-becker
left a comment
There was a problem hiding this comment.
@mkannwischer LGTM. Please check my edit and feel free to merge then.
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: