SHA crypto providers: Compare hex values as integers #336

Closed
wants to merge 2 commits into
from

Projects

None yet

5 participants

@kulpreet

The SHA1, SHA256 and SHA512 crypto providers are using string comparison to verify if a password matches.

encrypt(*tokens) == crypted

Which works fine if authlogic is used to generate and match the crypted_password.

However, I recently faced a solution where another system had to create and verify users and passwords. All was well till we realised that that other system was generating hex string with upper case characters. Like "AB10" instead of "ab10".

I think the crypto providers that generate hex should compare them using integer comparison. All we have to do it to_i(16) on both sides of the comparison to get it going.

The pull requests does just that. No changes needed for tests, as the tests are passing strings, which would be the case from the database, and we just convert the hex strings to integers and compare them.

@kulpreet kulpreet SHA1, SHA256 and SHA512 crypto providers generate hex strings on
encrypt.

However, they compare the encrypted passwords as strings. They should
compare them using integer comparison.

All I did was to call to_i(16) on both sides of the comparison to make
the change.

No changes to tests needed, they pass in string, just as db would and
then we convert to integer to compare.
c31a031
@binarylogic
Owner

I'm not completely understanding this. Can you write a test that fails and show how this solves it? Thanks for your help.

@kulpreet kulpreet = Example test to show how string comparison on the SHA256 and 512
will fail, while integer comparison will succeed
f552daf
@kulpreet

Hi,

Just wrote a small test that shows why string comparison of hex values can cause problems while using SHA crypto providers.

Encrypted passwords produced and consumed by authlogic work fine.

However, if a third party is generating encrypted passwords in the same database table, the hex could be in upper case.

The commit here includes a test to show how integer comparison would help. kulpreet@f552daf

The key is the following.

assert_not_equal digest, digest.upcase
assert_equal digest.to_i(16), digest.upcase.to_i(16)
@binarylogic
Owner

Correct me if I'm wrong, but can't to_i(16) return duplicate integer for different strings? Which defeats the purpose of using any kind of digest....

@jvans1

to_i 16 can but to_i(36) wont. to_i will return duplicate values when the string character is out of range for the integer representation for it. 'A'.to_i(2) => 0, 'B'.to_i(2) => 0 but that can't happen for to_i 36

@schuetzm

IMO it's not a good idea to rely on the assumption that the database contains a valid hex string at all. For example, the admin could have appended an invalid hex character to (temporarily) disable login for that account (a similar technique is used in /etc/shadow on *nix). to_i(36) returns the same value for "abcd" and "abcd!".

So why not just use downcase, if case sensitivity is the actual problem?

@kulpreet

Agree with @schuetzm. downcase is a much better solution, avoids all the integer conversion uncertainties.

@binarylogic
Owner

agree downcase is a better direction

@jaredbeck
Collaborator

why not just use downcase, if case sensitivity is the actual problem?

agree downcase is a better direction

It sounds like the consensus here is to use downcase, so this PR, based on to_i(16) can be closed. Thanks for raising the issue, and it sounds like a PR using downcase instead would be welcome, thanks!

@jaredbeck jaredbeck closed this Jul 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment