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

Switch argument order for hash_equals #59

Merged
merged 1 commit into from
Dec 9, 2015
Merged

Switch argument order for hash_equals #59

merged 1 commit into from
Dec 9, 2015

Conversation

kelunik
Copy link
Contributor

@kelunik kelunik commented Dec 6, 2015

First argument is documented to be the known string, but actually it shouldn't matter, since length information is leaked anyway.

@odino
Copy link
Contributor

odino commented Dec 7, 2015

hey @kelunik thanks! What do you mean by "length information is leaked anyway"?

@kelunik
Copy link
Contributor Author

kelunik commented Dec 7, 2015

hash_equals is documented to have the first argument always the known string, but this is no longer true. The implementation is symmetric for both arguments. I guess it iterated over the user-provided string before not to leak timing information, but as timing information is impossible to leak here, it has been removed to a symmetric version.

Leaking length information means that an attacker can always guess the length of the token that you compare it to, but that doesn't really matter. If your strings are not equal in length, you'll hit the early return. But as said, length isn't really the secret here.

@kelunik
Copy link
Contributor Author

kelunik commented Dec 7, 2015

Please have a look at PR #58 first.

cirpo added a commit that referenced this pull request Dec 9, 2015
Switch argument order for hash_equals
@cirpo cirpo merged commit 9e6c061 into namshi:master Dec 9, 2015
@kelunik kelunik deleted the patch-hmac branch December 9, 2015 09:28
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

Successfully merging this pull request may close these issues.

None yet

3 participants