Security: input over 72 char: throw error

[larrykluger]

See https://n0rdy.foo/posts/20250121/okta-bcrypt-lessons-for-better-apis/

Bcrypt is typically used to encrypt passwords. But it currently silently accepts input over 72 char even though characters 73 and beyond are ignored.

This enables appsec attacks in various scenarios...see the post

[alex]

Yes, this flaw is documented here: https://github.com/pyca/bcrypt?tab=readme-ov-file#maximum-password-length

Really at this point bcrypt exists for historic compatibility and new applications should use scrypt or argon2id (as documented here: https://github.com/pyca/bcrypt?tab=readme-ov-file#bcrypt)

Is there a particular change you're proposing?

[paketb0te]

not OP, but I also read this blog post, came here to take a look at the linked Issue #691 and found it was already closed back in 2023.

The article makes a convincing (to me, at least) argument that it would be preferrable to raise an error or otherwise signal to the caller that the input is too long / will not be used in its entirety, instead of silently truncating the input.

So, probably something like

def hashpw(password: bytes, salt: bytes) -> bytes:
    if len(password) > 72:
        raise ValueError("Password must be max 72 bytes")

I don't know shit about rust, but seeing that hashpw is actually implemented in rust, I guess it could look something like:

fn hashpw<'p>(
    py: pyo3::Python<'p>,
    password: &[u8],
    salt: &[u8],
) -> pyo3::PyResult<pyo3::Bound<'p, pyo3::types::PyBytes>> {
    if password.len() > 72 {
        return Err(pyo3::exceptions::PyValueError::new_err(
            "Password cannot be longer than 72 bytes"
        ));
    }
    let password = &password[..password.len()];
    ...

(please excuse any errors, this is literally the first piece of rust I have written in my life)

[alex]

For your first rust code, it's right on the money :-)

I could have sworn I had a PR somewhere that did this, and then we didn't merge it for a good reason, but I can't find the closed PR, and I can't remember the reason. @reaperhulk do you remember either?

[reaperhulk]

I don't recall a PR for this, but this is a breaking change. If we change the default behavior we do still need a way to allow the previous behavior. Not a difficult task, but necessary. We should perhaps also be louder in advising people to move to scrypt and argon2id.

[alex]

Do we need an API for that? Callers can always just `s[:72]` themselves.

…

On Fri, Feb 7, 2025 at 6:20 PM Paul Kehrer ***@***.***> wrote: I don't recall a PR for this, but this is a breaking change. If we change the default behavior we do still need a way to allow the previous behavior. Not a difficult task, but necessary. We should perhaps also be louder in advising people to move to scrypt and argon2id. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

[reaperhulk]

Yeah, good point. Silent truncation can simply be made explicit and that's a clearer thing than allow_silent_truncation=True or some other API.

[paketb0te]

I agree that this is a breaking change and as such should be very carefully considered.
Would it be sensible to not start raising Exceptions, but start by issuing a RuntimeWarning or similar (and start raising Exceptions in a later release, to allow better communicating the change to users)?

[paketb0te]

@alex @reaperhulk any ideas / plans to move this forward?
Let me know if I can assist in any way :)

[reaperhulk]

I would be fine with a PR that makes this an exception with a message explaining how to do truncation.

[alex]

resolved in #1000