ed25519, ristretto: more test coverage - #271
Conversation
|
Thank you! This is in my list to review when I'm back from a short vacation, later next week. |
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
Thanks! Here's a bit of feedback.
It might also be nice to update README.md with the new coverage.
| "generatorVersion": { | ||
| "type": "string", | ||
| "description": "DEPRECATED: prefer \"source\" property in test group", | ||
| "deprecated": true | ||
| }, |
There was a problem hiding this comment.
No need to add this for new schemas (same feedback for the other new schema files).
| }, | ||
| "curve": { | ||
| "enum": [ | ||
| "ristretto255" |
There was a problem hiding this comment.
might be nicer to avoid single-value enums here (and for operation).
| @@ -0,0 +1,109 @@ | |||
| { | |||
There was a problem hiding this comment.
You can drop the _v1 portion of the file name for these new schemas. The only reason some of the files have that today is that there used to be a testvectors/ schema and a testvectors_v1/ schema for some types, but we've removed all the testvectors/ stuff.
| "resultScalar": { | ||
| "type": "string", | ||
| "format": "HexBytes", | ||
| "description": "the expected inverse; omitted for invalid tests" |
There was a problem hiding this comment.
we can encode this "required except for invalid" notion into the schema itself. I think the chunked encryption schema has an example.
Same feedback for the other instances.
| "publicKeyPem": "-----BEGIN PUBLIC KEY-----\nMCowBQYDK2VwAyEAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=\n-----END PUBLIC KEY-----\n", | ||
| "tests": [ | ||
| { | ||
| "tcId": 152, |
There was a problem hiding this comment.
I might be misunderstanding but I think tcIds 152 .. 180 would be better as result: acceptable with a flag instead of result: invalid. AFAIU RFC 8032 verification satisfies these vectors and this is encoding a stricter subgroup policy.
As a concrete example, upstream libsodium 1.0.18 accepts 160..180, neither that version or current master performs a full subgroup validation.
WDYT? Am I missing something?
| "inonce": { | ||
| "type": "string", | ||
| "format": "HexBytes", | ||
| "description": "the 12-byte logical internal nonce" |
There was a problem hiding this comment.
I'm not sure I follow why this is 12 bytes instead of 8. It looks like all of the values have a four trailing zero bytes. I thought the nonce construction was counter[4] || inonce[8].
2 participants
Adds 146 regression vectors derived from cryptographic boundary fixes in
sodium_compatv2.5.1 and v2.5.2, using vector-forge:This also adds four schemas for the new vector formats.
The main idea behind this PR was to write stable test vectors to exercise the attack surface in those releases so that other implementations that currently test with Wycheproof can ensure they are resilient to similar bugs.