search

jazzband / django-rest-knox

Authentication Module for django rest auth
MIT License
1.17k stars 213 forks source link

Can we filter AuthToken on token_key and digest in authenticate_credentials? #326

Closed oaosman84 closed 10 months ago

oaosman84 commented 10 months ago

In authenticate_credentials, there is a query to pull all AuthTokens filtered by token_key, then compare digests in Python. I believe this was necessary when salt was part of the model, because we needed the salt to generate the digest... but right now, seeing as salt is not being used, couldn't we just pre-calculate the digest and then do a .get(token_key, digest)? Any reason we need to pull all AuthTokens matching the token_key into Python and check them one by one?

angusholder commented 10 months ago

I suspect it's purposely done this way to avoid a timing attack - attackers could guess a valid token digest by observing how long database lookups for the token take, whereas this code only checks the digest using compare_digest, to ensure the comparison is constant-time.

johnraz commented 10 months ago

What @angusholder said and also I replied to you here https://github.com/jazzband/django-rest-knox/issues/245#issuecomment-1880812373 - no need for a duplicate ;-) closing for the same reason as on the other ticket.

oaosman84 commented 10 months ago

I suspect it's purposely done this way to avoid a timing attack - attackers could guess a valid token digest by observing how long database lookups for the token take, whereas this code only checks the digest using compare_digest, to ensure the comparison is constant-time.

Granted, I'm not a security expert, but I'm not sure timing attacks apply here. Timing attacks usually work via byte-by-byte manipulation, but in this case we would:

  1. Always calculate the digest
  2. Always do the database lookup by digest

1 will always take constant time. 2 may vary, but it is based on the hashed value, so the attacker really has no way to iterate byte-by-byte to get closer to the actual digest... furthermore, even if they do have the digest, they wouldn't be able to utilize it because they can only send a token, and the token must be hashed (that's the reason for storing the digest rather than the token in the database to begin with).

In fact, if we are worried about timing attacks on database queries, querying on the token_key itself might be leaking some info as that one is able to be manipulated byte-by-byte, and the attacker could presumably iterate their way into at least the token_key prefix.

Happy to be enlightened if I'm missing something.

johnraz commented 10 months ago

@oaosman84 thanks a lot for elaborating more clearly.

My concern is still with the fact that we would constantly pre-generate potentially invalid digest for no good reasons (1).

oaosman84 commented 10 months ago

@oaosman84 thanks a lot for elaborating more clearly.

My concern is still with the fact that we would constantly pre-generate potentially invalid digest for no good reasons (1).

OK, that's fair. I'll think about this a bit more, maybe there's a way to try to get the best of both worlds..

Thanks for clarifying.