Password truncation must trigger an exception

Issue #59 resolved
Robert Buchholz
created an issue

As a security API, passlib should fail noisily rather than possibly truncate passwords silently.

As such, the following should not even be possible with passlib:

In [11]: h = bcrypt.encrypt('a'*72 + 'a')

In [12]: bcrypt.verify('a'*72 + 'a', h)
Out[12]: True

In [13]: bcrypt.verify('a'*72 + 'b', h)
Out[13]: True

I understand it is by design (of the bcrypt algorithm) and that the bcrypt_sha256 implementation does not suffer from this problem. However, bcrypt.encrypt() of a string longer than 72 characters should fail as should bcrypt.verify() (independently).

Comments (9)

  1. Eli Collins repo owner

    This was coded as silent for two reasons. One was so that passlib could validate against existing test vectors for the various truncating hashes. More importantly, it was so that passlib wouldn't unexpectedly throw an error when users tried to use an over-large password to .verify() an existing hash, such as one generated by a library which adheres to the spec.

    Based on that motivation, I don't think it's a good idea for .verify() calls to have this limitation -- it will make it impossible to verify against existing hashes of over-large passwords; and while there may be relatively few 73+ character bcrypt hashes, there are a lot of legacy des_crypt & lmhash hashes where users will "know" their password is larger than 8 / 14 characters. Changing .verify() would require application developers to add in a confusing "Please try again using only the first <X> characters of your password" message before their users could successfully log in. It seems to add complexity at multiple levels, but with no real increase in security.

    That said, this does seem like a good default behavior going forward for .encrypt(). There's already a PasswordSizeError that may be raised when an abusively large input is provided, so application developers (should) be expecting .encrypt() to potentially raise such an error. I'll mark this for the 1.7 release.


    Design Notes:

    • This affects five hashes: bcrypt, des_crypt, lmhash, cisco_pix, and cisco_asa; and the django_* variants thereof.

    • Should add a PasswordTruncationError that subclasses PasswordSizeError, so that applications can easily distinguish it in order to present a different message to user.

    • Potentially add an allow_truncation flag so callers can turn this off, for crazy edge cases I can't think of right now.

    • Make sure PasswordHash interface docs are clear that these errors may be raised.

  2. MartinH

    Thinking about this: I would greatly appreciate at least a warning to the log if verify() truncates a password, as it potentially means I'm doing something very wrong as a programmer. (For example prepending some ginormous prefix to the password in the belief that this makes it more secure).

    Sure, log warnings are not perfect, and easily missed - but worlds better than no way to tell this happened at all.

  3. Eli Collins repo owner

    Support added as of rev 90c008b97c02:

    bcrypt, des_crypt, and a handful of others now a support a "truncation_error" flag, which enabled throwing an error instead of silent truncation. It's also supported by the CryptContext object so it can be applied as a general policy to all hashes. For example:

    hasher = bcrypt.using(truncate_error=True)

    hasher.hash("x"*73)

    ... will now raise a passlib.exc.PasswordTruncateError

    After consideration, I decided against added a "warning" mode -- if an attacker had access to application logs, they could correlate timecodes of the warnings with user logins. While not a significant information leak, I prefer to err on the side of caution :)

    This is currently NOT the default behavior, as that would break too many deployments out there; but have tried to make the option reasonably visible in the documentation of all the hashes, so it can be enabled if desired.

    This will be part of the 1.7 release, which will be out in the next month.

  4. Robert Buchholz reporter

    Eli, thank you for implementing an error-checking option. Concerning your point of not raising/warning by default: I would argue that any system relying on the truncation behaviour is already broken (its security may be compromised), but I appreciate your consideration for API stability.

    One option to introduce the warning without compromising specific users is to deprecate the old behaviour of instantiating a hasher an implicit truncate_error setting. So for example: 1.7 introduces the option with a default of "truncate_error=False" (possible values: False / True). Developers have a chance to explicitly set or unset the option. 1.8 deprecates truncate_error=False, raising a warning when the class is instantiated with False set explicitly or implicitly * 1.9 ignores the option, behaving as if it were true

    The option could then be deprecated and removed similarly.

  5. Log in to comment