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

Claimed issue with spec #183

Open
technion opened this issue Jan 5, 2017 · 5 comments
Open

Claimed issue with spec #183

technion opened this issue Jan 5, 2017 · 5 comments

Comments

@technion
Copy link
Contributor

@technion technion commented Jan 5, 2017

Hi,

A concern has been raised here regarding a spec issue:

https://www.reddit.com/r/crypto/comments/5m8f32/found_a_small_error_in_argon2_spec/

I've logged this for the person who made the post, as they are not on GitHub.

@josephlr
Copy link
Contributor

@josephlr josephlr commented Jan 6, 2017

It looks like they are correct. Compare the comment here: https://github.com/P-H-C/phc-winner-argon2/blob/master/src/core.c#L187-L189 to the spec on Page 7. There's a difference here.

My guess would be that the second part of the Mapping J1, J2 to the reference block index spec section (where we only use the 3 previous segments) got mixed up with the first part. It doesn't seem that bad because it only reduces the size of the reference area by about 7/8 on average, but the choice is still interesting. I can't speak to which would be better (spec vs. implementation).

@khovratovich
Copy link
Member

@khovratovich khovratovich commented Jan 6, 2017

Great catch! Indeed it is an implementation bug, dating back to the very first implementation at Jan 2015.

But since it is non-critical, we'll probably fix it only in the new version of Argon2, whenever it appears. The spec will be fixed soon.

@technion
Copy link
Contributor Author

@technion technion commented Jan 6, 2017

we'll probably fix it only in the new version of Argon2, whenever it appears

As long as it doesn't affect security, I'd highly suggest trying focus on stability.

@LoupVaillant
Copy link

@LoupVaillant LoupVaillant commented Jul 27, 2017

Just here to say I was the one who reported the bug on Reddit (I finally had to get that GitHub account). Thanks for forwarding it here!

@LoupVaillant
Copy link

@LoupVaillant LoupVaillant commented Oct 29, 2019

Err, I'm sorry, did you close this by mistake? I've just checked the PDF, and as far as I can tell, it still doesn't accurately describe the reference implementation. From page 7, (§3.3 Indexing):

Then we determine the set of indices R that can be referenced for given [i][j] according to the following rules:

  1. If l is the current lane, then R includes all blocks computed in this lane, that are not overwritten yet, excluding B[i][j − 1].
  2. If l is not the current lane, then R includes all blocks in the last S − 1 = 3 segments computed and finished in lane l. If B[i][j] is the first block of a segment, then the very last block from R is excluded.

If I read it correctly, item (1) is mistaken. The reference implementation does something different:

  • If I is the current lane, then R includes:
    • All segments computed in this lane, that are not overwritten yet. (This means 3 segment if we're in the second pass or further.)
    • All blocks computed from the start of the segment to B[i][j-2] included.
    • If B[i][j] is the first block of a segment, then the very last block from R is excluded.

Though that sounds convoluted. Here's a more procedural formulation:

  • During the first pass, R starts at B[i][0], and ends at B[i][i-2], included.
  • During subsequent passes, R starts at the beginning of the next segment, and ends at B[i][i-2], included. (We treat the lane as a ring buffer.)

Having a discrepancy between the specs and the reference implementation causes serious confusion when we try to implement Argon2 independently. I suffered this confusion back in 2017. I expect other people will suffer the same. I strongly believe this issue should remain open until the specs are amended (possibly with a note indicated the difference between the original intent and what we ended up having), and the PDF re-generated.

@khovratovich khovratovich reopened this Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.