[Django] #31375: make_password shouldn't accept values other than bytes or string as an argument
Django
argument
----------------------------------------+------------------------
Reporter: iamdavidcz | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
----------------------------------------+------------------------
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:**
- users unaware of this strange behaviour can accidentally create weak
passwords (potential security issue)
- other libraries throw 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
--
Ticket URL: <https://code.djangoproject.com/ticket/31375>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
argument
Reporter: iamdavidcz | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by iamdavidcz:
Old description:
> 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:**
>
> - users unaware of this strange behaviour can accidentally create weak
> passwords (potential security issue)
> - other libraries throw 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
New description:
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:1>
Django
argument
Reporter: iamdavidcz | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
From a bit of testing it seems `make_password` already raises `TypeError`
for invalid types on `master`
{{{#!python
>>> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:2>
Django
argument
Reporter: iamdavidcz | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by iamdavidcz):
[comment:2 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:
{{{#!python
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:
{{{#!python
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.
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:3>
Django
argument
Reporter: iamdavidcz | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
I was using `MD5PasswordHasher` which is the default for test settings;
`PBKDF2PasswordHasher.encode` seems to be accepting whatever value is
provided.
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:4>
Django
argument
Reporter: iamdavidcz | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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:
-
[https://github.com/django/django/blob/fc0fa72ff4cdbf5861a366e31cb8bbacd44da22d/django/contrib/auth/hashers.py#L303-L314
Argon2PasswordHasher]
-
[https://github.com/django/django/blob/fc0fa72ff4cdbf5861a366e31cb8bbacd44da22d/django/contrib/auth/hashers.py#L407-L417
BCryptSHA256PasswordHasher]
-
[https://github.com/django/django/blob/fc0fa72ff4cdbf5861a366e31cb8bbacd44da22d/django/contrib/auth/hashers.py#L613-L619
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`)
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:5>
Django
argument
Reporter: iamdavidcz | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by iamdavidcz):
Replying to [comment:5 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:6>
Django
argument
Reporter: iamdavidcz | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:7>
Django
argument
Reporter: iamdavidcz | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Hasan Ramezani):
Any decision about this ticket?
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:8>
Django
argument
Reporter: iamdavidcz | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.0
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Unreviewed => Accepted
Comment:
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".
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:9>
Django
argument
Reporter: iamdavidcz | Owner: Hasan Ramezani
Type: Bug | Status: assigned
Component: contrib.auth | Version: 3.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* owner: nobody => Hasan Ramezani
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:10>
Django
argument
Reporter: iamdavidcz | Owner: Hasan Ramezani
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:11>
Django
argument
Reporter: iamdavidcz | Owner: Hasan Ramezani
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:12>
Django
argument
Reporter: iamdavidcz | Owner: Hasan Ramezani
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:13>
Django
argument
Reporter: iamdavidcz | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:14>
Django
argument
Reporter: iamdavidcz | Owner: Hasan
| Ramezani
Component: contrib.auth | Version: 3.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"8aa71f4e8706b6b3e4e60aaffb29d004e1378ae3" 8aa71f4e]:
{{{
#!CommitTicketReference repository=""
revision="8aa71f4e8706b6b3e4e60aaffb29d004e1378ae3"
Fixed #31375 -- Made contrib.auth.hashers.make_password() accept only
bytes or strings.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:16>
Django
argument
Reporter: iamdavidcz | Owner: Hasan
| Ramezani
Component: contrib.auth | Version: 3.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"b3ab92cc5ad5e851692f36432465a9150e8b3313" b3ab92cc]:
{{{
#!CommitTicketReference repository=""
revision="b3ab92cc5ad5e851692f36432465a9150e8b3313"
Refs #31375 -- Added test for contrib.auth.hashers.make_password() bytes
support.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31375#comment:15>