Update for BCrypt wrap bug detected by mo7ty · Pull Request #24 · notypecheck/passlib

mo7ty · 2025-10-12T15:54:52Z

Improved version of PR Update for BCrypt wrap bug detected #19, based on bcrypt improvements, test fixup #23, including usage of TruncateMixin along with passlib.exc.PasswordTruncateError.

Refer to BCrypt wrap bug detected #18

* Use declared `IDENT_*`'s in corresponding checks

* Handle possible `ValueError` during `_finalize_backend_mixin` if expected * Add and use `passlib.handlers.bcrypt._BcryptCommon.`[+is_password_too_long()+] helper method * Add and use `passlib.handlers.bcrypt._BcryptCommon.`[+hash_password()+] helper method * Override `passlib.utils.handlers.TruncateMixin.using()` for `_BcryptCommon`: set `truncate_verify_reject` if `_fails_on_long_secrets and truncate_error` * Refer to [BCrypt wrap bug detected notypecheck#18](notypecheck#18)

* Update `_bcrypt_test.fuzz_verifier_bcrypt()/check_bcrypt()` to use `passlib.handlers.bcrypt._BcryptCommon.hash_password()`

* Consider `cand_hasher.truncate_verify_reject` to verify if should truncate long secret before comparing

… `TruncateMixin._check_truncate_policy()`

chapmajs · 2025-10-13T13:46:27Z

I think this PR still misses the mark w.r.t. TruncateMixin allowing user-configurable behavior. I don't think TruncateMixin features should probably be used to work around backend changes, that seems like it has the potential to allow users of the library to make configuration changes that cause unintuitive errors.

Additionally, wrapping _calculate_checksum() and performing different behavior there based on TruncateMixin settings may lead to a situation in which hashes generated with this handler can't be verified by it. If automated behavior there really is desired, the backend ought to implement hash() and verify() itself -- probably just perform the checks and then super() it.

notypecheck · 2025-10-13T14:58:47Z

@chapmajs I'm not quite sure myself about using TruncateMixin as well, but I think it should be ok. By default it should not raise an error when trying to verify existing hashes, unless you set truncate_verify_reject to True.

passlib/handlers/bcrypt.py

tests/utils.py

chapmajs · 2025-10-13T15:06:23Z

@chapmajs I'm not quite sure myself about using TruncateMixin as well, but I think it should be ok. By default it should not raise an error when trying to verify existing hashes, unless you set truncate_verify_reject to True.

My issue with this is that the state of truncate_verify_reject is being dynamically modified depending on which backend is providing bcrypt functionality. It's possible for a user to set an option and have that change not be honored.

The mixin should provide the desired behavior (that a passlib exception be raised) as long as it's activated. If the defaults are changed to activate it, that's fine, but it should be a static defaults change that still allows users to override it to achieve old behavior.

Changing the default probably shouldn't be done on a point release.

Since the bcrypt backend doesn't provide its own hash() and verify() this should be happening automatically, with no manual checks in the bcrypt backend. See #23, it works there without other changes to behavior.

…ng_secrets flag`

… >= 5.0.0" This reverts commit 5ed5618.

This reverts commit 3826f12.

* Rename and improve `passlib.handlers.bcrypt._BcryptCommon.is_secret_truncate_err` helper method * Add and use `passlib.handlers.bcrypt._BcryptBackend.`[+_check_truncate_flag()+] helper method for `TruncateMixin` flags validation * Add and use `passlib.handlers.bcrypt._BcryptBackend.`[+_handle_w_truncate()+] to handle retries for truncate errors * Implement `passlib.handlers.bcrypt._BcryptBackend.`[+verify+] * Update `passlib.handlers.bcrypt._BcryptBackend._calc_checksum` to use `_handle_w_truncate(_bcrypt.hashpw, truncate_error)` * Revert "Detect BCrypt >= 5.0.0 expected `ValueError` for long secrets" - commit 3be3773 * Refer to [BCrypt wrap bug detected notypecheck#18](notypecheck#18)

* Update `_bcrypt_test.fuzz_verifier_bcrypt()/check_bcrypt()` to use `passlib.utils.handlers.GenericHandler.verify`

mo7ty · 2025-10-14T15:14:42Z

Hi @notypecheck and @chapmajs,

Thank you for all your comments, feedback, concerns and suggestions.
Hoping that the latest update on this PR answers them all, please comment here if there’s still anything that might not be quite right.

passlib/handlers/bcrypt.py

chapmajs · 2025-10-14T15:16:49Z

There's still a fundamental misunderstanding of what TruncateMixin should and shouldn't be doing here, and direct manipulation of its state, which will override/ignore user configuration.

* Simplify and update `passlib.utils.handlers.TruncateMixin.using()` to also accept `truncate_verify_reject` * Add and use `passlib.utils.handlers.TruncateMixin.`[+_check_truncate_flag()+] and [+_check_verify_truncate_policy()+] helper methods * Update `tests.utils.HandlerCase.test_truncate_error_setting()` to cover `truncate_verify_reject` setting * Update `passlib.handlers.bcrypt._BcryptBackend` accordingly

mo7ty · 2025-10-15T08:03:21Z

Local tests run results using BCrypt versions 4.3.0 and 5.0.0:

collected 3573 items
1603 skipped
1970 passed

chapmajs

This existing comment (which was present prior to any of the PR work) explains the crux of the issue I'm seeing:

        Setting ``truncate_error=True`` will cause :meth:`~passlib.ifc.PasswordHash.hash`
        to raise a :exc:`~passlib.exc.PasswordTruncateError` instead.

Handlers that use the TruncateMixin should not be raising in their own code for truncation, except maybe if they need to completely reimplement hash() and verify() -- it's behavior and functionality defined and implemented in the mixin. See PR #23 -- you will notice that there are no changes to internal state or raising within wrapped methods, etc. It "just works" because it

uses the default hash() and verify()
includes the TruncateMixin

Note that the tests correctly manipulate variables to control TruncateMixin behavior as-is. You can look at some of the other backends that already include TruncateMixin for alternate examples.

Do also note that calling super().hash() and/or super().verify() from within a custom hash() and/or verify() method will also correctly integrate TruncateMixin functionality.

mo7ty · 2025-10-18T14:04:19Z

Closing PR as per #25 (comment).

mo7ty added 5 commits October 11, 2025 11:34

Tweak passlib.handlers.bcrypt._BcryptCommon._finalize_backend_mixin

8c9a7df

* Use declared `IDENT_*`'s in corresponding checks

Tweak comment on tests.utils.HandlerCase.test_secret_w_truncate_size()

da751b9

Update test_handlers_bcrypt/test_77_fuzz_input() for BCrypt >= 5.0.0

5ed5618

* Update `_bcrypt_test.fuzz_verifier_bcrypt()/check_bcrypt()` to use `passlib.handlers.bcrypt._BcryptCommon.hash_password()`

Update tests.utils.HandlerCase.test_secret_w_truncate_size()

3826f12

* Consider `cand_hasher.truncate_verify_reject` to verify if should truncate long secret before comparing

mo7ty mentioned this pull request Oct 12, 2025

Update for BCrypt wrap bug detected #19

Closed

mo7ty marked this pull request as ready for review October 12, 2025 15:56

Update passlib.handlers.bcrypt._BcryptCommon.hash_password() to use…

9de2a89

… `TruncateMixin._check_truncate_policy()`

notypecheck requested changes Oct 13, 2025

View reviewed changes

passlib/handlers/bcrypt.py Outdated Show resolved Hide resolved

passlib/handlers/bcrypt.py Outdated Show resolved Hide resolved

chapmajs reviewed Oct 13, 2025

View reviewed changes

tests/utils.py Show resolved Hide resolved

mo7ty added 5 commits October 14, 2025 09:23

Detect bcrypt >= 5.0.0 using packaging.version to set `_fails_on_lo…

0754ab2

…ng_secrets flag`

Revert "Update test_handlers_bcrypt/test_77_fuzz_input() for BCrypt…

4cf5d8a

… >= 5.0.0" This reverts commit 5ed5618.

Revert "Update tests.utils.HandlerCase.test_secret_w_truncate_size()"

e9a6789

This reverts commit 3826f12.

Update test_handlers_bcrypt/test_77_fuzz_input() for BCrypt >= 5.0.0

dcd1018

* Update `_bcrypt_test.fuzz_verifier_bcrypt()/check_bcrypt()` to use `passlib.utils.handlers.GenericHandler.verify`

mo7ty requested a review from notypecheck October 14, 2025 15:14

chapmajs reviewed Oct 14, 2025

View reviewed changes

passlib/handlers/bcrypt.py Outdated Show resolved Hide resolved

mo7ty requested a review from chapmajs October 15, 2025 08:05

chapmajs reviewed Oct 15, 2025

View reviewed changes

mo7ty mentioned this pull request Oct 18, 2025

test: fix bcrypt tests #25

Merged

mo7ty closed this Oct 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update for BCrypt wrap bug detected#24

Update for BCrypt wrap bug detected#24
mo7ty wants to merge 12 commits intonotypecheck:mainfrom
mo7ty:bcrypt_wrap_bug_detected

mo7ty commented Oct 12, 2025 •

edited

Loading

Uh oh!

chapmajs commented Oct 13, 2025 •

edited

Loading

Uh oh!

notypecheck commented Oct 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chapmajs commented Oct 13, 2025 •

edited

Loading

Uh oh!

mo7ty commented Oct 14, 2025

Uh oh!

Uh oh!

chapmajs commented Oct 14, 2025

Uh oh!

mo7ty commented Oct 15, 2025

Uh oh!

chapmajs left a comment •

edited

Loading

Uh oh!

mo7ty commented Oct 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mo7ty commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chapmajs commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notypecheck commented Oct 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chapmajs commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mo7ty commented Oct 14, 2025

Uh oh!

Uh oh!

chapmajs commented Oct 14, 2025

Uh oh!

mo7ty commented Oct 15, 2025

Uh oh!

chapmajs left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mo7ty commented Oct 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mo7ty commented Oct 12, 2025 •

edited

Loading

chapmajs commented Oct 13, 2025 •

edited

Loading

chapmajs commented Oct 13, 2025 •

edited

Loading

chapmajs left a comment •

edited

Loading