`bcrypt_sha256` hash then verify fails

[miketheman]

I was testing this today as a potential alternative for passlib with bcrypt 5, and found a blocking issue.

bcrypt_sha256.verify() returns False for a hash that bcrypt_sha256.hash() just produced from the same password in the same process. Happens with bcrypt==4.3.0 and bcrypt==5.0.0. Doesn't happen on passlib==1.7.4, so this looks like a libpass-side regression rather than a bcrypt-compat thing.

Reproducer

# pip install libpass==1.9.3 bcrypt==4.3.0
from passlib.hash import bcrypt_sha256

h = bcrypt_sha256.using(rounds=12).hash("password")
print("hash:  ", h)
print("verify:", bcrypt_sha256.verify("password", h))

Observed

hash:   $bcrypt-sha256$v=2,t=2b,r=12$TkheP1Z9v6iFcmKaS8WyLu$t81vvvUJi9VS7mino2oycdr2dTDMzKe
verify: False

Expected

verify: True. passlib==1.7.4 on the same Python and same bcrypt returns True.

Compatibility matrix

passlib package	bcrypt	hash → verify
passlib==1.7.4	4.3.0	True
libpass==1.9.3	4.3.0	False (this bug)
libpass==1.9.3	5.0.0	False (this bug)
libpass<=1.9.2	5.0.0	backend init throws (see note)

On libpass<=1.9.2 + bcrypt==5.0.0 no verify ever runs: the wrap-bug probe in _finalize_backend_mixin calls bcrypt.hashpw with a 255-byte secret, and bcrypt 5.0 rejects it with ValueError: password cannot be longer than 72 bytes....

v=1 is broken too (bcrypt_sha256.using(version=1, rounds=12)), so it's not the HMAC change in v=2.

Environment

• Python 3.13
• macOS (haven't tried other platforms)

[Daverball]

I've confirmed the same behavior, but it seems like it only fails on the very first call to verify, subsequent calls work correctly for whatever reason. So it seems like it's some initialization bug that gets fixed by the first call, but not soon enough in order for the first call to return the correct result.

I tried to trace back the problem to any changes in the bcrypt backend, but I haven't seen any particularly suspicious changes there. However I do think #21 is slightly broken, in that it now no longer will detect the wraparound bug in bcrypt >= 5.0.0, because it doesn't try to re-verify the bugged hash after catching the exception, so it can never return True. But I'm positive it's unrelated to this bug, since patching that method doesn't fix this bug.

So it's probably something more subtle. I'm not sure where this fork started from, if it was from a clean 1.7.4 state, or the WIP 1.8.0 state, which might be the cause for this regression.

[Daverball]

Alright this is very weird, with the example hash provided by @miketheman I get even stranger behavior, the first call to verify succeeds with 'password' as the secret, but then all subsequent calls fail. So what exactly happens seems to depend on the specific hash, some hashes will only succeed checking on the first call, others will only succeed on subsequent calls. It seems unrelated to the version or bcrypt parameters though, since I was using the same ones.

Here's an example for a hash that fails on the first call, but succeeds on subsequent calls:

bcrypt_sha256.verify(
    'test',
    '$bcrypt-sha256$v=2,t=2b,r=12$rlZd3bZdHx7xtnTFun9Yq.$Rpwf4TGjgvcr6FsrxswW6e.hT3LHn2W'
) 

[Daverball]

I tried seeing if the bug was already there from the commit where the fork was started:

uvx --from git+https://github.com/notypecheck/passlib@0b8d944 --with 'bcrypt<5' python

This seems to at least provide a hint, since the first call to bcrypt_sha256.verify causes a TypeError, due to the magical way passlib replaces the _NoBackend class with the first available backend mixin on the first call of any method that requires a backend to work. So I assume the incorrect result on the first call is related to that mechanism and how its internals were changed between 1.7.4 and the WIP 1.8.0 work.

It also looks like maybe the hash @miketheman provided is faulty because it was generated as the first call, it fails to verify for me on passlib 1.7.4 with bcrypt 4.3.0, so the only remaining mystery would be what happens on this first call and why it generates faulty hashes. I suppose it's possible that the first call results in one of the mixins being skipped or the method of the wrong super class gets called, since the TypeError I get on 0b8d944 happens inside super(), which is very suspicious.

Since there's only one bcrypt backend now, it might be worth simplifying this module and directly inheriting from _BcryptBackend instead of _NoBackend. Although it's possible the same bug is also exhibited in other handlers that use this _NoBackend thing to select a backend, so it might be worth rebasing all the changes on top of the 1.7.4 tag, so we work off a known good state, instead of some possibly very broken WIP state, that's difficult to debug for anyone other than the original maintainer.

[Daverball]

Alright, I think I might have found the bug, the commit that replaced explicit super() calls with the magical implicit ones, was a little bit too eager and replaced it even when the class that's being passed to it is different from the class the method was defined in, so reverting the following line will possibly fix things:

fa69e3b#diff-adfe697b5e233b33b4165a4c0e5631002800ce3d56928caf720d8da79d4879d2R586

We probably need to check the other super calls that were changed in that commit as well, to make sure they aren't causing issues either.

It looks like this fork decided to replace super() with self in that method to fix the TypeError caused by the class being replaced during the call of that method, which could be the source of our problem, since it probably means we call bcrypt_sha256._calc_checksum a second time within its super()._calc_checksum call, instead of calling BcryptBackend._calc_checksum. So on the first call we end up hashing the secret a second time.

[Daverball]

The easiest way to figure out which changes in fa69e3b were incorrect would probably be to run ruff --select UP008 --fix on the previous commit and look at the diff between that and fa69e3b since ruff should be smart enough to only simplify super() calls, that are safe to simplify.

[notypecheck]

super(Cls, self) should really be the same as super() afaik.
The issue is just some kind of incompatibility between passlib and bcrypt>=5.0.0, I sadly didn't have much time recently, but I'll try to have a look at the issue.

[Daverball]

It only is the same if Cls matches the class it's defined in, in this case it wasn't, super() lets you override the usual method resolution order, and this is what happens there, since _NoBackend gets replaced with a different class on the first call to _calc_checksum it needs to call into the _calc_checksum of the class it got replaced by, the way to achieve this is to pass the original parent class of _NoBackend to super() which is the same as calling super() in the parent class a second time, only this time it goes to the real backend, since _NoBackend has been replaced.

It's a real esoteric and confusing thing to do and really gets into the nitty gritty of how inheritance works in Python, so It's understandable that you didn't recognize what this is supposed to do. It would have made much more sense for the function that replaced the backend class to return the backend class so you can just directly call _calc_checksum there instead of relying on the magical abilities of super() almost nobody knows about or understands.

[Daverball]

Either way I'm happy to do the work of writing a patch for this, but the outlined strategy of running ruff --select UP008 --fix on the commit before fa69e3b and diffing the output against fa69e3b should give you all of the super() calls you need to revert in order to restore the old correct behavior of 1.7.4

[Daverball]

And just to put your mind at ease, the issue has nothing to do with bcrypt >=5.0.0, since it also happens with bcrypt 4.3.0 and fa69e3b is the first commit that's broken, the commit before works correctly with 4.3.0, I made sure to verify as much, it's just that your fix switching from super() to self in order to get rid of the TypeError was incorrect, since that starts method resolution back from the top, instead of continuing with the real backend.

Your fix happened to work correctly for the bcrypt class by accident, since that's the class that was previously passed to super and it doesn't have its own _calc_checksum method, but it's incorrect for bcrypt_sha256, since it does have one and calling it twice causes the secret to be hashed a second time by sha256 HMAC before being passed to the bcrypt module for the final hashing step, but only on this first call where _NoBackend gets replaced with _BcryptBackend in bcrypt.__bases__, since that's the only time _NoBackend._calc_checksum will be called.

The tests don't really cover this mechanism very well, since they tend to just manually call set_backend, so the backend never has to be inserted mid-call in the tests as far as I can tell. It might be worth adding some test cases to HandlerCase for the default backends to make sure this first call doesn't produce hashes that will not verify on subsequent calls and vice versa, so we don't end up regressing this again in the future.

I'll see when I find some time to work on this. Probably not until next week.