fix(auth): use secure_compare for credential comparisons#1032
Conversation
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
| computed_stored_key = :pgo_scram.h(client_key) | ||
|
|
||
| if computed_stored_key == sasl_secrets.stored_key do | ||
| if Plug.Crypto.secure_compare(computed_stored_key, sasl_secrets.stored_key) do |
There was a problem hiding this comment.
I don't think this has any risk of timing attack? The value is hashed.
There was a problem hiding this comment.
@v0idpwn Yeah, you are right. I added secure_compare to all the places where you have some client-controlled input. Does it hurt much to be extra defensive? WDYT?
There was a problem hiding this comment.
I'll remove the three sites tbh. Plain text comparisons is the only real risk 🤔
There was a problem hiding this comment.
Thinking out loud, they can reproduce the hash value locally too. So they essentially know what bytes are being compared at this line. So yeah, guessing the first byte of the stored key is possible. Although, guessing the next bytes would start to get impossible as many inputs can produce the same first few bytes of a hash.🤔🤔
There was a problem hiding this comment.
fwiw, I just verified Postgres source as reference, and its SCRAM implementation uses memcmp for these hash comparisons, so I think we are good to go as it is
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
|
Thank you! 💚 |
2 participants
What kind of change does this PR introduce?
Nitpicky change to use
secure_comparewhen comparing client-fed credentials. We are already using this in https://github.com/supabase/supavisor/blob/main/lib/supavisor/client_handler/auth_methods/password.ex#L86, so it closes the inconsistency as well.