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
secrets.compare_digest raises TypeError if one of it's arguments is None #99502
Comments
This looks like normal Python behavior. The function is appropriately type checking its inputs and raising a The "timing attack" concern seems dubious because these is no password to attack. Also it likely isn't possible to change the code to give error cases an identical timing with a non-error case. |
+1, this is documented:
https://docs.python.org/3/library/secrets.html#secrets.compare_digest And:
https://docs.python.org/3/library/hmac.html#hmac.compare_digest Passing non-string inputs might result in But, there's an inconsistency in docs between |
I'm fine with documentation, I would like to have a method that implements |
Would something like: def compare_digest_or_none(a: str | bytes | None, b: str | bytes | None) -> bool:
if a is None and b is None:
return True
if a is None or b is None:
return False
return secrets.compare_digest(a, b) work for you? |
@sobolevn Well, that's easy to do, but it is not constant time (at least for what I understand that term to mean). I would imagine the implementation would be something like this (pseudocode obviously): def compare_digest_or_none(a: str | bytes | None, b: str | bytes | None) -> bool:
if a is None and b is None:
sleep_to_prevent_timing_attack()
return True
if a is None or b is None:
sleep_to_prevent_timing_attack()
return False
return secrets.compare_digest(a, b) |
The
This isn't a Python specific problem. And mostly we're not in business of making security recommendations that aren't firmly established. Personally, if I were concerned about the surrounding code leaking timing information, I would obscure the result by adding |
As far as I understand it, the pull request linked above will add some info about that handling I would like something more, but I understand that there is no way to implement this reliably, without also providing the function the length of the strings that need to be compared. I understand that you don't want to give security advice - still I resent that security critical APIs like this are so easy to use the wrong way. For normal APIs that is annoying, for Security Critical APIs that means every user of the API will implement a different slightly wrong workaround that can then be exploited. |
…est` (GH-99512) Now it is in sync with https://docs.python.org/3/library/hmac.html#hmac.compare_digest It is the same function, just re-exported. So, I guess they should mention the same input types.
…re_digest` (pythonGH-99512) Now it is in sync with https://docs.python.org/3/library/hmac.htmlGH-hmac.compare_digest It is the same function, just re-exported. So, I guess they should mention the same input types. (cherry picked from commit 47d673d) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
…re_digest` (pythonGH-99512) Now it is in sync with https://docs.python.org/3/library/hmac.htmlGH-hmac.compare_digest It is the same function, just re-exported. So, I guess they should mention the same input types. (cherry picked from commit 47d673d) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
…are_digest` (GH-99512) (#99790) gh-99502: mention bytes-like objects as input in `secrets.compare_digest` (GH-99512) Now it is in sync with https://docs.python.org/3/library/hmac.htmlGH-hmac.compare_digest It is the same function, just re-exported. So, I guess they should mention the same input types. (cherry picked from commit 47d673d) Co-authored-by: Nikita Sobolev <mail@sobolevn.me> Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
…are_digest` (GH-99512) (#99791) gh-99502: mention bytes-like objects as input in `secrets.compare_digest` (GH-99512) Now it is in sync with https://docs.python.org/3/library/hmac.htmlGH-hmac.compare_digest It is the same function, just re-exported. So, I guess they should mention the same input types. (cherry picked from commit 47d673d) Co-authored-by: Nikita Sobolev <mail@sobolevn.me> Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
dwt commentedNov 15, 2022
•
edited by bedevere-bot
Bug report
When using
secrets.compare_digest()
with one or both of it's arguments being None the function explodes. This is not explicitly documented even though the documentation mentions that it compares strings.I would argue that a function that advertises to securely compare the two arguments in constant time, should not explode and thus reveal that one of the arguments (in this case someone had set the password hash in the Database to
NULL
) has a different length. If this is not wanted, I think the documentation should include some guidance how to handle this case not generate a timing attack that reveals something about the password in the Database? Not sure that is reasonable, but at least som hints about this not handlingNone
would have been really helpful.Your environment
Linked PRs
secrets.compare_digest
#99512secrets.compare_digest
(GH-99512) #99790secrets.compare_digest
(GH-99512) #99791The text was updated successfully, but these errors were encountered: