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

Argon2id hash matching issue #1175

Closed
seremenko-wish opened this issue Mar 25, 2021 · 4 comments
Closed

Argon2id hash matching issue #1175

seremenko-wish opened this issue Mar 25, 2021 · 4 comments
Labels
upstream Issue is caused by an upstream dependency.

Comments

@seremenko-wish
Copy link
Contributor

seremenko-wish commented Mar 25, 2021

Describe the bug

hash.Argon2.Compare successfully matches password with slightly modified hashes.

Unit test that is not expected to fail (last characters in the second and third tests were changed):

func TestCompare(t *testing.T) {
	_, reg := internal.NewFastRegistryWithMocks(t)
	h := hash.NewHasherArgon2(reg)

	assert.Nil(t, h.Compare(context.Background(), []byte("test"), []byte("$argon2id$v=19$m=32,t=2,p=4$cm94YnRVOW5jZzFzcVE4bQ$MNzk5BtR2vUhrp6qQEjRNw")))
	assert.Error(t, h.Compare(context.Background(), []byte("test"), []byte("$argon2id$v=19$m=32,t=2,p=4$cm94YnRVOW5jZzFzcVE4bQ$MNzk5BtR2vUhrp6qQEjRN2")))
	assert.Error(t, h.Compare(context.Background(), []byte("test"), []byte("$argon2id$v=19$m=32,t=2,p=4$cm94YnRVOW5jZzFzcVE4bQ$MNzk5BtR2vUhrp6qQEjRN5")))
}

Results of the test:

    hasher_test.go:60: 
        	Error Trace:	hasher_test.go:60
        	Error:      	An error is expected but got nil.
        	Test:       	TestCompare
    hasher_test.go:61: 
        	Error Trace:	hasher_test.go:61
        	Error:      	An error is expected but got nil.
        	Test:       	TestCompare

Reproducing the bug
Add this test to hash/hasher_test.go and run it.

Expected behavior
Test should pass

Environment
Git commit 33c5f6bf / master branch

Additional context

I used https://argon2.online/ service to validate my findings. It shows that only the hash from the first test case matches the password. The second and third don't match.

@aeneasr
Copy link
Member

aeneasr commented Mar 25, 2021

Interesting, this needs to be reported to golang/go with a x/crypto: prefix in the subject line!

@aeneasr aeneasr added the upstream Issue is caused by an upstream dependency. label Mar 25, 2021
@seremenko-wish
Copy link
Contributor Author

seremenko-wish commented Mar 25, 2021

The problem is that base64.RawStdEncoding.DecodeString calls in Argon2.decodeHash (https://github.com/ory/kratos/blob/master/hash/hasher_argon2.go#L83) method do not use strict mode and allow slightly modified input string to be successfully decoded and later to be used in hash matching. Using non-strict mode violates RFC4648 section 3.5 (https://tools.ietf.org/html/rfc4648#section-3.5) and introduces some security concerns.

I will submit a PR with a fix later today

@seremenko-wish
Copy link
Contributor Author

fixed in #1169

@aeneasr
Copy link
Member

aeneasr commented Apr 3, 2021

Thank you @seremenko-wish :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream Issue is caused by an upstream dependency.
Projects
None yet
Development

No branches or pull requests

2 participants