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

strip NUL characters where it actually matters #3

Closed
wants to merge 1 commit into from

Conversation

marado
Copy link

@marado marado commented Oct 13, 2016

bcrypt doesn't handle NUL characters. Probably with that in mind,
mara strips any NUL char from the string that is going to be hashed.
However, the strip is taking place before sha512'ing the password,
and sha512 can produce NUL chars.

This patch moves the stripping of NUL characters to the already sha512
hashed string, thus ensuring that no NUL character reaches the bcrypt
hashing function itself.

Note: since this actually changes the hashed password, this change
might not be backwards compatible with your current database.

bcrypt doesn't handle NUL characters. Probably with that in mind,
mara strips any NUL char from the string that is going to be hashed.
However, the strip is taking place before sha512'ing the password,
and sha512 *can* produce NUL chars.

This patch moves the stripping of NUL characters to the already sha512
hashed string, thus ensuring that no NUL character reaches the bcrypt
hashing function itself.

Note: since this actually changes the hashed password, this change
might not be backwards compatible with your current database.
@coveralls
Copy link

coveralls commented Oct 13, 2016

Coverage Status

Coverage decreased (-0.03%) to 77.386% when pulling 281acac on marado:master into b67eb12 on radiac:master.

@marado marado closed this Oct 13, 2016
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

2 participants