-
Notifications
You must be signed in to change notification settings - Fork 27
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
new hash_to_field definition #212
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I quite like the new modularity, especially as it lets us more easily test things. (We can have different test vectors for hash_to_field and expand_message_md et al.)
@chris-wood, @armfazh should we wait to update code until after we land #196? (I know that I need to review that code! Sorry!!! I will do it next...) |
What about merging 196 (modulo Riad's review). |
this name is confusing!
This commit incorporates the XOF and MD designs from cfrg#202.
this edit changes the format of the Suites section to make the definition of each suite more self-contained. Most importantly, each suite definition explicitly states the encoding type (hash_ or encode_to_curve).
04ba703
to
e2f803d
Compare
This is to guarantee that different values of len_in_octets result in completely different outputs. Otherwise, changing len_in_octets by a small amount may not change ell, and the outputs would be related in that case.
I added |
k is used in expand_message_md and in hash_to_field, so it needs to be specified in suites in order to ensure compatibility.
f55aff5
to
4830ca7
Compare
You have said that power analysis attacks are not in scope for this document (#202 (comment)); you should note something about that in the security considerations. |
add a paragraph in Security Considerations discussing the way that H shoud be domain separated inside/outside hash_to_field. Also, added a requirement in hashtofield-expand-other to make sure that other variants follow this guideline, too. Also, slight reorg / cleanup / deduplication in Security Considerations.
I've got updated code ready to go. Should I just add those commits to this PR, or should we handle them separately? @armfazh @chris-wood |
Separately is fine by me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest changes LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, I just left few comments.
What is the consensus about using octet rather than byte?
I prefer byte. @kwantam? |
I think "octet" is a little awkward and annoying, but is more precise than "byte" (in the sense that octet is exactly 8 bits, but byte isn't necessarily). In practice this probably mattered a lot more when RFC793 was written than it does now, so I'm fine switching everything to bytes and just noting somewhere that we assume bytes are 8 bits. |
df1db4a
to
4021f2f
Compare
Ach, there's one more issue I need to think about before we merge @chris-wood @armfazh We want to use a prefix-free encoding of DST in both of the expand_message variants. This is easy: just prepend DST with its length. But I want to make sure the text reflects this throughout. -- Done now. By the way, I didn't give a good justification for this above, but the reason is that otherwise we don't meet our own requirement that all distinct (msg, DST, length_in_bytes) triplets give distinct outputs. Specifically, without a prefix-free DST encoding, it's possible to "trade" bytes between msg and DST to get two triplets that give the same output. |
this is necessary to ensure that distinct (msg, DST, len) inputs give distinct outputs. Otherwise, it's possible to 'move' bytes from msg to DST and thereby create ambiguous inputs.
This PR redefines hash_to_field in the way discussed in #202.
Now ready for review, I believe.
Todo:
update implementationswe can do this in a separate PR (or I'm happy to do it here... what do we think?)@armfazh @chris-wood @JustinDrake I'd appreciate comments on the WIP version. I'm sure I've left typos, things aren't super clear yet, etc.!