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.

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

ParallelHash fails to encode n on zero length input, producing incorrect result. #906

Closed
dghgit opened this issue Jun 21, 2020 · 5 comments
Assignees

Comments

@dghgit
Copy link

dghgit commented Jun 21, 2020

Testing ParallelHash128

{"tcId":967,
"msg":"",
"len":0,
"blockSize":126,
"customization":")7HY7q\u003C \u0027V!Rpq_l4:k(Ju$Q@:Qt]d1R:jRIQ~\u0027CWSQ*ADPNMZx%e5t23v9{?FCBFN8\u0026",
"outLen":16}

Produces "7F2F" in the sample answer. I can reproduce this if I comment out the encoding of n that is listed in step 4 of the algorithm:

  1. z = z || right_encode(n) || right_encode(L).

where n is 0 as there is no data.

The correct answer appears to be: "ef5e"

@celic
Copy link
Collaborator

celic commented Jun 26, 2020

So even when n is 0, the right_encode(n) should cause some difference in z during this concatenation? I'll find time to take a look. Is this related to #912 ?

@dghgit
Copy link
Author

dghgit commented Jun 27, 2020

Yes, that's correct even an n of zero produces a change.

This issue is not really related directly to #912, although obviously the ParallelHash is vulnerable to the same issue as it's based on cSHAKE, it's just that the size of the customization string needs to be reduced by the length of the function string to trigger a situation where the encoded N & S input is block aligned. It would probably be a good to include an appropriate customization string here too though. Personally, I can't imagine presenting ParallelHash, TupleHash, or KMAC for testing without including cSHAKE, but then I've seen a lot of things now that I could not have imagined, someone might present a device, or module, which only exposes one of the subsidiary algorithms, so including an AFT vector here which would also expose the issue would seem like a good idea.

@Kritner
Copy link
Contributor

Kritner commented Sep 28, 2020

A fix for this is on demo

@dghgit
Copy link
Author

dghgit commented Sep 29, 2020

I've been able to confirm this. Thanks, marking as closed.

@dghgit dghgit closed this as completed Sep 29, 2020
@Kritner
Copy link
Contributor

Kritner commented Sep 30, 2020

this change is now on production https://github.com/usnistgov/ACVP-Server/releases/tag/v1.1.0.12

celic pushed a commit to usnistgov/ACVP-Server that referenced this issue Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants