Possible timing attack in magic link token lookup?

[abevoelker]

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: https://github.com/mikker/passwordless/blob/b0fc34356b6e62c549addc19e2f02faf548c160c/app/controllers/passwordless/sessions_controller.rb#L101

(See some considerations based on a previous vulnerability in Devise token auth here: https://stackoverflow.com/a/18695244/215168)

[abevoelker]

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 ActiveSupport::SecurityUtils.secure_compare to do constant-time compares without any new column or hashing. However either of these latter two I believe you couldn't use find_by! and would require an additional param to the magic link to identify the specific authenticatable resource you'd be looping over each token for.

However looking at how the gem works, I'm wondering if you could solve the whole thing by throwing away the passwordless_sessions table entirely and just using ActiveSupport::MessageEncryptor to generate encrypted tokens that contain the data you normally store in that table. What you'd give up is the ability to revoke/expire individual magic links by manipulating the database (instead you could only invalidate all existing magic links at once by rolling the secret key). Not sure how much utility there really is there anyway since the tokens appear to only be valid for one hour by default? But if you wanted to keep the table you could just use MessageEncryptor to encrypt the id of the row, and safely do Session.valid.find after decryption without worrying about timing issues.

[mikker]

Hi @abevoelker! If you look a line above where find_session is called you’ll see I already do a Bcrypt hash to avoid something like what you describe.

I think the current model is sufficient. But if you feel there’s an improvement in what you’re suggesting, PRs are welcome 😊

[abevoelker]

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 https://stackoverflow.com/questions/28395665/could-a-random-sleep-prevent-timing-attacks

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! :tada: