PBKDF2PasswordHasher enforces UTF-8 decodable requirement on passwords provided as bytes

[Johannes Leuschner]

Before 78fac1b0473ed8960ecd2a30aca4fa8420d150b8, PBKDF2PasswordHasher (which is the default hasher) used to accept a password of type bytes in make_password and check_password. After that commit, force_str is called on the password, raising a decoding error if the bytes are not valid UTF-8. The ​pbkdf2 implementation then actually converts back to bytes.

Minimal example:

from django.contrib.auth.hashers import make_password
make_password(b"\xc0", hasher="pbkdf2_sha256")  # fails with DjangoUnicodeDecodeError

and also

from django.contrib.auth.hashers import make_password, check_password
encoded = make_password(b"", hasher="pbkdf2_sha256")
check_password(b"\xc0", encoded)  # fails with DjangoUnicodeDecodeError

A use-case for passing a password of type bytes is generated passwords/keys, which can be exposed to the user e.g. via base64 encoding. This maximizes password strength compared to only allowing valid UTF-8 characters, and generating random passwords with a restricted character set is not as straight-forward. Existing applications using passwords of type bytes now fail both at making and checking passwords.

Note that in the same commit force_str is also introduced to MD5PasswordHasher, but there it makes sense because .encode() has been called anyways, i.e. bytes was not supported before.

[Mike Edmunds]

The ​make_password() docs are a little confusing (or at least open to interpretation):

… It takes one mandatory argument: the password in plain-text (string or bytes). …

It looks like "plain-text … bytes" is really trying to say "utf-8 encoded text." If so, then b"\xc0" is not a valid "plain-text bytes" password and the error is correct. And this should be treated as a docs clarification bug (for both make_password() and check_password()).

But if "plain-text … bytes" actually means "a text password in the app's choice of encoding," then this is/was a breaking change. (And that's unclear to me: the password hashers have bounced back and forth between force_bytes() and encode() over time: see #36226 and #27795. During the force_bytes() periods, I think any encoding would have been acceptable so long as it was consistently applied.)

Oh, and if "plain-text" actually means ​plaintext (as opposed to ciphertext, so has nothing to do with human-readable "text"), then it's probably reasonable to expect arbitrary byte sequences to be supported.

[Jacob Walls]

Auth tests pass when I comment out the password = force_str(password) line (and would be hash-preserving, given what's pointed out above about the conversion back to bytes in the hasher). It looks like this was not discussed in review, so it appears unintentional. (It might have been added for parallelism with force_str(salt), but that's not a real parallel -- the salt gets string-interpolated, but the password doesn't.)

I agree we shouldn't impose a constraint on the universe of valid secure passwords at the hasher level. Forms and validators and management commands may expect strings only, but that's separate.

scrypt/argon2/bcrypt appear to accept these bytes, so we have a consistency issue (consistency was the rationale for accepting #36226).

MD5 is a different beast entirely because the password and salt get concatenated.

If we wanted some sort of canonicalization check, I don't think we'd put it in the hasher.

[Jacob Walls]

Thanks for taking ownership Vishy. I'm on vacation next week, so I'm hoping to have a PR approved by Friday of this week. Do you think you could turn this around quickly? No worries if not, I'll just assign to myself tomorrow.

[Vishy]

I got this Jacob.

Seems more like a regression, force_str() looks redundant as the pbkdf2() makes a force_bytes() anyway!

[Vishy]

I have added checks for each password hasher to maintain consistency for bytes passwords.

​https://github.com/django/django/pull/21541

[Jacob Walls]

Great, can you add a release note in 6.0.7.txt?

[Jacob Walls]

Pushed an example of what I mean here: ​PR

[Jacob Walls <jacobtylerwalls@…>]

In 356e5b0:

Fixed #37184 -- Allowed non-UTF-8 bytes passwords in the PBKDF2 and MD5 password hashers.

An unnecessary force_str() call in the PBKDF2 hasher raised
UnicodeDecodeError on perfectly valid password values.

The MD5 hasher had a similar issue, however the same commit that
introduced the bug also happened to allow bytes in general, so preserve
the support while removing the constraint on UTF-8 validity by
concatenating like:

force_bytes(salt) + force_bytes(password)

Regression in 78fac1b0473ed8960ecd2a30aca4fa8420d150b8.

Co-authored-by: Vishy <vis.pypi@…>

[Jacob Walls <jacobtylerwalls@…>]

In da8d02f:

[6.1.x] Fixed #37184 -- Allowed non-UTF-8 bytes passwords in the PBKDF2 and MD5 password hashers.

An unnecessary force_str() call in the PBKDF2 hasher raised
UnicodeDecodeError on perfectly valid password values.

The MD5 hasher had a similar issue, however the same commit that
introduced the bug also happened to allow bytes in general, so preserve
the support while removing the constraint on UTF-8 validity by
concatenating like:

force_bytes(salt) + force_bytes(password)

Regression in 78fac1b0473ed8960ecd2a30aca4fa8420d150b8.

Co-authored-by: Vishy <vis.pypi@…>

Backport of 356e5b0f5ddb473eec3ed96d8ad9a37f3f8585eb from main.

[Jacob Walls <jacobtylerwalls@…>]

In 50503b3d:

[6.0.x] Fixed #37184 -- Allowed non-UTF-8 bytes passwords in the PBKDF2 and MD5 password hashers.

An unnecessary force_str() call in the PBKDF2 hasher raised
UnicodeDecodeError on perfectly valid password values.

The MD5 hasher had a similar issue, however the same commit that
introduced the bug also happened to allow bytes in general, so preserve
the support while removing the constraint on UTF-8 validity by
concatenating like:

force_bytes(salt) + force_bytes(password)

Regression in 78fac1b0473ed8960ecd2a30aca4fa8420d150b8.

Co-authored-by: Vishy <vis.pypi@…>

Backport of 356e5b0f5ddb473eec3ed96d8ad9a37f3f8585eb from main.

[Johannes Leuschner]

Thanks a lot for the quick fix, much appreciated!

[nessita <124304+nessita@…>]

In e789914:

Clarified "plaintext" vs. "plain-text" in password hashers docs.

Replaced "plain-text" with "plaintext" where it is used to describe the
unencrypted input to a password hashing function. (In a cryptography
context this is the preferred spelling, and it is already used that way
in Django's release notes.) This reduces ambiguity about whether
make_password() expects UTF-8 encoded Unicode text ("plain-text bytes")
or the unencrypted material for the user's password ("plaintext bytes").
(See #37184.)

All other current uses of "plain text" and "plain-text" in docs and
docstrings (including the one elsewhere in passwords.txt) are describing
text that is plain (unformatted; not rich text).

[Natalia <124304+nessita@…>]

In e17c1b2:

[6.1.x] Clarified "plaintext" vs. "plain-text" in password hashers docs.

Replaced "plain-text" with "plaintext" where it is used to describe the
unencrypted input to a password hashing function. (In a cryptography
context this is the preferred spelling, and it is already used that way
in Django's release notes.) This reduces ambiguity about whether
make_password() expects UTF-8 encoded Unicode text ("plain-text bytes")
or the unencrypted material for the user's password ("plaintext bytes").
(See #37184.)

All other current uses of "plain text" and "plain-text" in docs and
docstrings (including the one elsewhere in passwords.txt) are describing
text that is plain (unformatted; not rich text).

Backport of e78991410b78391bc07a4b5010b273dfec814ff6 from main.

[Natalia <124304+nessita@…>]

In 8e8582f3:

[6.0.x] Clarified "plaintext" vs. "plain-text" in password hashers docs.

Replaced "plain-text" with "plaintext" where it is used to describe the
unencrypted input to a password hashing function. (In a cryptography
context this is the preferred spelling, and it is already used that way
in Django's release notes.) This reduces ambiguity about whether
make_password() expects UTF-8 encoded Unicode text ("plain-text bytes")
or the unencrypted material for the user's password ("plaintext bytes").
(See #37184.)

All other current uses of "plain text" and "plain-text" in docs and
docstrings (including the one elsewhere in passwords.txt) are describing
text that is plain (unformatted; not rich text).

Backport of e78991410b78391bc07a4b5010b273dfec814ff6 from main.