-
-
Notifications
You must be signed in to change notification settings - Fork 86
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
Possible timing attack in magic link token lookup? #31
Comments
I was thinking of possible suggestions for quick fixes to this like doing lookups based on cryptographic digest hash (would have to add this column): def find_session
Session.valid.find_by!(
authenticatable_type: authenticatable_classname,
token_hash: ::Digest::SHA256.hexdigest(params.fetch(:token, "")),
)
end Or alternatively using the existing BCrypt dependency for that column, or just using However looking at how the gem works, I'm wondering if you could solve the whole thing by throwing away the |
Hi @abevoelker! If you look a line above where I think the current model is sufficient. But if you feel there’s an improvement in what you’re suggesting, PRs are welcome 😊 |
Hi @mikker, I did see that but it registered to my brain as effectively just a sleep since the output is discarded, which I've seen it said doesn't prevent timing attacks, just maybe slow them down a little: https://security.stackexchange.com/questions/96489/can-i-prevent-timing-attacks-with-random-delays Anyway this level of paranoia isn't really that important for my use of the gem, especially given the 1hr token timeout, so I'm gonna close this. Have a great weekend! 🎉 |
I'm no crypto expert so pardon me if I'm wrong but it looks like the magic link lookup process is vulnerable to timing attacks due to the token lookup being a naive database lookup of the input string:
passwordless/app/controllers/passwordless/sessions_controller.rb
Line 101 in b0fc343
(See some considerations based on a previous vulnerability in Devise token auth here: https://stackoverflow.com/a/18695244/215168)
The text was updated successfully, but these errors were encountered: