secrets.compare_digest raises TypeError if one of it's arguments is None

[dwt]

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.

❯ python3.11
Python 3.11.0 (main, Oct 25 2022, 13:57:33) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import secrets
>>> secrets.compare_digest('', '')
True
>>> secrets.compare_digest('', None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand types(s) or combination of types: 'str' and 'NoneType'
>>> secrets.compare_digest(None, '')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand types(s) or combination of types: 'NoneType' and 'str'
>>> secrets.compare_digest(None, None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand types(s) or combination of types: 'NoneType' and 'NoneType'

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 handling None would have been really helpful.

Your environment

• CPython versions tested on: 3.6, 3.9, 3.10, 3.11
• Operating system and architecture: Linux x86, Mac ARM

Linked PRs

• gh-99512
• gh-99790
• gh-99791

[rhettinger]

This looks like normal Python behavior. The function is appropriately type checking its inputs and raising a TypeError if the wrong argument types are passed in. This is considered well-behaved and is pretty much the opposite of "it explodes".

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.

[sobolevn]

+1, this is documented:

Return True if strings a and b are equal

https://docs.python.org/3/library/secrets.html#secrets.compare_digest

And:

Return a == b. This function uses an approach designed to prevent timing analysis by avoiding content-based short circuiting behaviour, making it appropriate for cryptography. a and b must both be of the same type: either str (ASCII only, as e.g. returned by HMAC.hexdigest()), or a bytes-like object.

https://docs.python.org/3/library/hmac.html#hmac.compare_digest

Passing non-string inputs might result in TypeError.

But, there's an inconsistency in docs between secrets.compare_digest and hmac.compare_digest about bytes input 🤔

[dwt]

I'm fine with documentation, I would like to have a method that implements a==b in a constant time and allows None as an input, or different types the same that == does. Where would be the right place to ask for this?

[sobolevn]

@dwt

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?

[dwt]

@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)

[rhettinger]

The compare_digest just delegates to _hashopenssl.compare_digest. That is the only way for us to get reliable timing assurances. I recommend against mucking with this code path. To me, the use case seems dubious and it isn't clear how this would even be possible.

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 handling None would have been really helpful.

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 sleep(random()).

[dwt]

As far as I understand it, the pull request linked above will add some info about that handling None is required. That is at least something that would have helped me and good progress.

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.