make_password shouldn't accept values other than bytes or string as an argument

[iamdavidcz]

Currently make_password function accepts almost every Python object as an argument. This is a strange behaviour and it results directly from force_bytes casting objects to str. We should throw the TypeError when passing anything but bytes or str to make_password.

Reasons:

• programmers unaware of this strange behaviour can accidentally create weak passwords (potential security issue)
• other libraries raise the TypeError in the same cases (eg. Werkzeug, passlib)
• it's inconsistent with the documentation that says:

  It takes one mandatory argument: the password in plain-text.
  

• it's inconsistent with validate_password behaviour (passing anything but bytes or str to validate_password raises the TypeError with default settings.AUTH_PASSWORD_VALIDATORS).

Discussion:

​https://groups.google.com/forum/#!topic/django-developers/1Ap0zDjFa4E

[Simon Charette]

From a bit of testing it seems make_password already raises TypeError for invalid types on master

>>> make_password(1)
TypeError: can only concatenate str (not "int") to str
>>> class Object:
        def __str__(self):
            return 'foo'
>>> make_password(Object())
TypeError: can only concatenate str (not "Object") to str
>>> make_password(True)
TypeError: can only concatenate str (not "bool") to str
...

It is hasher specific though so I guess we could turn not instance(password, str) into a TypeError from make_password itself instead of relying on implementation details of hashers.

[iamdavidcz]

Simon Charette, thank you for your testing. Could you tell me what is your PASSWORD_HASHERS setting? I've tried to reproduce your example on master using PBKDF2PasswordHasher and this hasher does not raise the TypeError on my side:

In [1]: class Object:
    ...:     def __str__(self):
    ...:         return 'foo'

In [2]: from django.contrib.auth.hashers import get_hasher

In [3]: hasher = get_hasher('default')

In [4]: hasher
Out[4]: <django.contrib.auth.hashers.PBKDF2PasswordHasher at 0x10cef7850>

In [5]: salt = hasher.salt()

In [6]: salt
Out[6]: 'l9QFlyCku6VE'

In [7]: hasher.encode(Object(), salt)
Out[7]: 'pbkdf2_sha256$216000$l9QFlyCku6VE$qqMksofk6MSGevhG/I4xJ7AIRf+Hhq/7myi3pd6vSBU='

In [8]: hasher.encode('foo', salt)
Out[8]: 'pbkdf2_sha256$216000$l9QFlyCku6VE$qqMksofk6MSGevhG/I4xJ7AIRf+Hhq/7myi3pd6vSBU='

Now I can see two options in order to make this type guard hasher agnostic:

1. Add if statement to make_password as you suggested. Something like:

   if not isinstance(password, (bytes, str)):
       raise TypeError('password must be bytes or string (got %s).' % type(password).__name__)
   

2. Change force_bytes utility to to_bytes and add default keyword argument force=True. Then, hasher's encode method would use to_bytes function with force=False argument. In all other force_bytes occurrences in the codebase, changing from force_bytes to to_bytes should be sufficient.

def to_bytes(s, encoding='utf-8', strings_only=False, errors='strict', force=True):
    if isinstance(s, bytes):
        if encoding == 'utf-8':
            return s
        else:
            return s.decode('utf-8', errors).encode(encoding, errors)
    if strings_only and is_protected_type(s):
        return s
    if isinstance(s, memoryview):
        return bytes(s)
    if force:
        return str(s).encode(encoding, errors)
    raise TypeError('cannot convert %s object to bytes' % type(s).__name__)

However, this solution would require much more effort and maintaining backward compatibility. The first option seems to be easier.

[Simon Charette]

I was using MD5PasswordHasher which is the default for test settings; PBKDF2PasswordHasher.encode seems to be accepting whatever value is provided.

[hashlash]

I think force_bytes is only used by PBKDF2PasswordHasher and PBKDF2SHA1PasswordHasher through django.utils.crypto.pbkdf2. Some Django's included hashers depend on hashlib library while others depend on third party libraries using _load_library() method for password encoding. Examples:

• ​Argon2PasswordHasher

• ​BCryptSHA256PasswordHasher

• ​CryptPasswordHasher

As Simon Charette said, it is hasher specific. So if we want to make it language agnostic, I think the first option will do (adding if not isinstance(password, (bytes, str)) in make_password)

[iamdavidcz]

Replying to hashlash:

As Simon Charette said, it is hasher specific. So if we want to make it language agnostic, I think the first option will do (adding if not isinstance(password, (bytes, str)) in make_password)

Fully agree. Let's choose the first option then.

[Hasan Ramezani]

Simon , if you agree with the proposed solution :

if not isinstance(password, (bytes, str)):
    raise TypeError('password must be bytes or string (got %s).' % type(password).__name__)

I can prepare a patch for it.

[Hasan Ramezani]

Any decision about this ticket?

[Carlton Gibson]

OK, I think this is pretty marginal, but let's have it.

A patch needs to include a release note, saying approximately, "make_password() now requires its argument to be a string or bytes. Other types should be explicitly cast to one of these, if required".

[Hasan Ramezani]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In b3ab92cc:

Refs #31375 -- Added test for contrib.auth.hashers.make_password() bytes support.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 8aa71f4e:

Fixed #31375 -- Made contrib.auth.hashers.make_password() accept only bytes or strings.