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

Flakey password verification test #9491

Closed
russjones opened this issue Dec 19, 2021 · 1 comment · Fixed by #13229
Closed

Flakey password verification test #9491

russjones opened this issue Dec 19, 2021 · 1 comment · Fixed by #13229

Comments

@russjones
Copy link
Contributor

----------------------------------------------------------------------
FAIL: password_test.go:98: PasswordSuite.TestTiming

password_test.go:163:
    c.Assert(diffFraction < 0.1, Equals, true, comment)
... obtained bool = false
... expected bool = true
... elapsed difference (11.278536047602582%) greater than 10%
@zmb3
Copy link
Collaborator

zmb3 commented Jan 12, 2022

As I noted in a related PR:

The idea here is that if a user doesn't exist, then by default the call would complete faster since it doesn't have to do any comparison. We mitigate this by intentionally slowing down the "user doesn't exist" case by doing a comparison against a dummy hash instead. This way we aren't leaking any information to an attacker about what users may or may not exist in the backend.

The irony here is that the user doesn't exist case is actually slower than the case where the user does exist, and by slowing it down further we're only increasing the gap. We don't notice this because the test only looks at the absolute value of the delta.

suspect this is because the amount of time it takes to compare the password hash is dwarfed by the extra handling for the case when the user doesn't exist:
- we're doing formatted logging that we wouldn't otherwise do
- we're checking trace.IsNotFound three times
- the trace package generates backtraces for the errors which is a costly operation compared to checking a bcrypt hash

I suspect we'll have to stop using trace in order to really fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants