[Probably BUG] set_password and check_password accept values other than string as parameters

[Dawid Czeluśniak]

Hi all,

I've noticed that both set_password and check_password methods accept values other than str as parameters. For example I'm able to set password to boolean values:

In [1]: u.set_password(True)

In [2]: u.save()

In [3]: u.refresh_from_db()

In [4]: u.check_password(True)

Out[4]: True

In [5]: u.check_password('True')

Out[5]: True

What is even weirder, I'm able to set password as Exception class:

In [1]: u.set_password(Exception)

In [2]: u.save()

In [3]: u.refresh_from_db()

In [4]: u.check_password(repr(Exception))

Out[4]: True

and the User instance itself:

In [1]: u.set_password(u)

In [2]: u.save()

In [3]: u.refresh_from_db()

In [4]: u.check_password(u)

Out[4]: True

In [5]: u.check_password(str(u))

Out[5]: True

IMHO this is not correct behaviour especially because Django documentation implies that these methods accept strings.

set_password(raw_password)
Sets the user’s password to the given raw string, taking care of the password hashing. Doesn’t save the User object.

check_password(raw_password)
Returns True if the given raw string is the correct password for the user. (This takes care of the password hashing in making the comparison.)

Please let me know if this is reproducible on your side.

Dawid

[Adam Johnson]

Hi Dawid,

Welcome to the django-developers mailing list!

This is pretty normal Pythonic behaviour. Inside these methods, Django casts the given object to a string with str() (specifically, in force_bytes). Most objects can be cast to a string, although I agree many of them won't necessarily make sense as passwords.

This is because Python normally leans on "duck typing". Runtime type checking is normally used sparingly since it removes flexibility. Static type checkers are gaining some popularity but it remains to be seen how far they will affect Django and its ecosystem. Even then, I'm not sure we'd be able to enforce strings-only as the type signature here, as it wouldn't be backward compatible.

Thanks,

Adam

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/0429a2cd-a16c-429f-98b5-938629073ca5%40googlegroups.com.

--

Adam

[Ethem Güner]

I reproduced this case. Used a random model from my project.

Dawid Czeluśniak <czelusn...@gmail.com>, 12 Mar 2020 Per, 02:06 tarihinde şunu yazdı:

--

[1337 Shadow Hacker]

I agree with Adam, but in this case it seems to pose a security risk in case of user mistake, as such, raising a ValueError would have protect against the mistake of passing empty passwords, unless you consider empty passwords a feature of course in which case please dismiss my email.

[Dawid Czeluśniak]

I think that the root question here is: should we allow users to create passwords from anything that is not str? Now seems like make_password function allows to do that (Django 3.0.4):

In [1]: make_password(True)

Out[1]: 'pbkdf2_sha256$180000$WXVqmAhNTScA$bAiYHSr2fs3LbccZ+mDOAqE0vhYCPUOTVtot+TDTgSU='

In [2]: make_password(False)

Out[2]: 'pbkdf2_sha256$180000$19XGmulpDIUE$XbaYmfcbwPvlekI5RltSbRRJnfqLS7mfigb88VveOBY='

In [3]: make_password(list)

Out[3]: 'pbkdf2_sha256$180000$RkRlYdoMjKhR$QpSMO7wPNo3TVCGZk0BR1zolUI69OE2PFB7N3DYfBE0='

In [4]: make_password(frozenset)

Out[4]: 'pbkdf2_sha256$180000$qY0D7n7Q36Tb$1BDA0JcC0uz9RTIepDvcviU5O23WL/Cs/O9NX25fy18='

In [5]: make_password([1, 2, 3, {"hello": "world"}])

Out[5]: 'pbkdf2_sha256$180000$B4rNXyIZDrzM$pbdM797yYZzWu04WUrcZXBNNUwojSXZREkrbprxeP0A='

Many projects are actually checking the if the password is a str throwing the TypeError if it's not. I don't quite understand why Django should be an exception in this case...

https://fossies.org/linux/openslides/openslides/users/views.py#l_189
https://github.com/golismero/openvas_lib/blob/master/openvas_lib/common.py#L232
https://github.com/firebase/firebase-admin-python/blob/master/firebase_admin/_auth_utils.py#L73

[Adam Johnson]

User provided passwords are validated already: https://docs.djangoproject.com/en/3.0/topics/auth/passwords/#module-django.contrib.auth.password_validation

When using set_password directly, you as the programmer are responsible for ensuring the value you use for password is valid. Normally this means calling the functions detailed in "Integrating validation" beforehand.

On Thu, 12 Mar 2020 at 15:55, '1337 Shadow Hacker' via Django developers (Contributions to Django itself) <django-d...@googlegroups.com> wrote:

I agree with Adam, but in this case it seems to pose a security risk in case of user mistake, as such, raising a ValueError would have protect against the mistake of passing empty passwords, unless you consider empty passwords a feature of course in which case please dismiss my email.

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/9Xx6DvMQMWVWMRYMhbK-8nXfyPrNU_5ljWd-YuXeXRmz3_pnYXT6axEYrDBfW4K1v5OGEshIR2SDAeZxpnDBSk6SMLe4oeiwcrDMnz7xah4%3D%40protonmail.com.

--

Adam

[Dawid Czeluśniak]

Adam,

If it's perfectly fine to pass almost any not-None object to make_password function and it returns correctly generated hash then why does the documentation say:

make_password(password, salt=None, hasher='default')
Creates a hashed password in the format used by this application. It takes one mandatory argument: the password in plain-text.

 

https://docs.djangoproject.com/en/3.0/topics/auth/passwords/#django.contrib.auth.hashers.make_password

What does "plain-text" mean there?

Thanks,
Dawid

On Thursday, 12 March 2020 18:18:59 UTC+1, Adam Johnson wrote:

User provided passwords are validated already: https://docs.djangoproject.com/en/3.0/topics/auth/passwords/#module-django.contrib.auth.password_validation

When using set_password directly, you as the programmer are responsible for ensuring the value you use for password is valid. Normally this means calling the functions detailed in "Integrating validation" beforehand.

--

Adam

[Tom Forbes]

In this context it means that you shouldn’t encrypt, hash or otherwise manipulate the password before passing it into the method. 

Django, many other packages and Python itself will accept objects that can be coerced into a string (via __str__) rather than throw an exception. We’re all consenting adults here - if you want to pass a non-string object into “make_password” then that’s up to you.

The question really is if this is a common enough mistake to warrant a guard against strange input. I’d say no, however a small change to the documentation might be in order.

Tom

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/bda75e4c-2dae-42ef-91f3-c3054031c800%40googlegroups.com.

[Dawid Czeluśniak]

Tom,

I believe that I found the root cause of this issue. Let's take a closer look at force_bytes function from Django and to_bytes function from Werkzeug (just for comparison). There is one fundamental difference between these: force_bytes from Django fallbacks to string representation of the object whereas to_bytes from Werkzeug raises TypeError if it can't cast the value to bytes. That's why when calling make_password with object that can't be "easily" cast to bytes returns a generated hash:

In [1]: from django.contrib.auth.hashers import make_password

In [2]: make_password([1, 2, 3])

Out[2]: 'pbkdf2_sha256$180000$Y9azq0uVWSoh$E7O13LefzP1I4MylXfFYKDkgCnZen6tf+FAblnBYssQ='

whereas generate_password_hash from Werkzeug raises TypeError:

In [16]: from werkzeug.security import generate_password_hash

In [17]: generate_password_hash([1, 2, 3])
TypeError: Expected bytes

IMHO throwing an exception is a more reasonable approach.

Anyway, thank you for your help :)
Dawid

Tom

To unsubscribe from this group and stop receiving emails from it, send an email to django-d...@googlegroups.com.

[Tom Forbes]

There is an argument to be made for not using force_bytes, but there is also a semantic difference between “force” and “to”. I wouldn’t think it would be possible to change force_bytes to throw an exception without serious compatibility issues - str() is how lazy objects are resolved IIRC, and there are numerous other places it is called with arguments that need to be cast to a string.

The behavior of the make_password method is quite surprising to be honest, and maybe the advantages of being able to pass any object into the method is entirely academic because nobody passes anything but strings on purpose. On the other hand this is the first post I can remember about this.

[Dawid Czeluśniak]

Tom,

The behavior of the make_password method is quite surprising to be honest

I'd go even further and say that currently the behaviour of the make_password function is wrong and unsafe. Again, let's look at hashing functions from other libraries. None of them fails silently and casts object to bytes using __str__().  Werkzeug and passlib are the most notable examples how to handle things correctly:

1. Werkzeug

In [1]: from werkzeug.security import generate_password_hash

In [2]: generate_password_hash(dict())
TypeError: Expected bytes

2. Passlib

In [1]: from passlib.hash import pbkdf2_sha256

In [2]: pbkdf2_sha256.hash(dict())
TypeError: secret must be unicode or bytes, not dict

3. Django

In [1]: from django.contrib.auth.hashers import make_password

In [2]: make_password(dict())
Out[2]: 'pbkdf2_sha256$180000$dimMkJ5wvrpn$eHh6CNAY+hTagaDmsofHMlJEbVOXEeIEfcT059Me2ho='
(seriously???)

This is especially wrong because programmers who are not aware of this strange behaviour can accidentally do things that they really don't want to do. I can imagine scenarios in which this can have some serious unintended consequences.

maybe the advantages of being able to pass any object into the method is entirely academic because nobody passes anything but strings on purpose

Exactly. I'd even say that there are no advantages of being able to pass any object into this function and it can have bad consequences.

Dawid

[Adam Johnson]

Dawid, thank you for checking these other implementations. I agree it's somewhat surprising and clearly something the developers of the other password libraries decided to guard against.

One question I have is - did you experience any real world issue with this? Reading back over the thread, you haven't mentioned this. As Tom says, this is the first mention he can remember.

We could add type guards to many of the thousands of functions in Django to prevent potential bugs. I'm sure some would. I'm sure many would just be "academic". And they come with both the implementation and maintenance costs, plus making duck-typing harder for users.

To add one here, it would go through the normal deprecation process: https://docs.djangoproject.com/en/dev/internals/release-process/#internal-release-deprecation-policy . This means PR's for 3.1, 3.2, and 4.0, changing the code, documentation, and release notes.

I'd say at this point I'm convinced this *could* be a (slightly) useful change. But you have to be aware - I don't think you'll find anyone who's willing to do this work other than yourself. There are many more interesting changes to be made to Django if you check Trac!

Thanks,

Adam

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.

To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAHzshFuQnEUrAdk53apDWw3wnPBNq%2BYQE9bxyfOpbFfyQS04dw%40mail.gmail.com.

--

Adam

[Dawid Czeluśniak]

Adam,

One question I have is - did you experience any real world issue with this?

Personally I didn't, but I can imagine scenarios where this could be an issue for other programmers. Suppose you want to create a password hash from another SHA256 hash, but you're only a human and you forget to call hexdigest() on the sha object:

In [1]: import hashlib

In [2]: password = hashlib.sha256('some_dummy_password'.encode())

In [3]: password
Out[3]: <sha256 HASH object @ 0x111d9f180>

In [4]: user.set_password(password)

In [5]: user.check_password('<sha256 HASH object @ 0x111d9f180>')
Out[5]: True

Indeed, you may think that this is a pretty uncommon scenario, but it could happen. For some reason libraries like Werkzeug and passlib raise the TypeError in this case and therefore a programmer can actually see that something is wrong. On the other hand, Django fails silently and the application will save a wrong password to the database. Yes, there are no doubts that in the end it's programmer's fault, but on the Django's main page we can read:

Django takes security seriously and helps developers avoid many common security mistakes.

I believe that adding a type guard to make_password function will help developers avoid a security mistake.

We could add type guards to many of the thousands of functions in Django to prevent potential bugs.

Of course we could, but that would be useless and waste of time. The question is if a function is important enough to add a type guard. Well, I think that function that is responsible for generating password hashes is important from the security perspective. And it seems that creators of Werkzeug and passlib libraries think so as well. But if you think the opposite, I'm fine with that. In this case, a small change in the documentation on make_password behaviour might be helpful.

Thanks,
Dawid

[Mentor Carranza Carranza]

Buen dia con todos, 

Yo estoy utilizando este metodo para grabar contraseñas, y lo hago porque al hacer un createsuperuser con python la clave que genera es con el mismo formato que cuando lo hago con este metodo, practicamente los valores que genero tienen el mismo formato que los del ejemplo de Dawid C.  Ahora lo que no se como hacer es : como compruebo si una clave almacenada en mi base de datos es la misma que estoy ingresando en la interface(pantalla),  he tratado de utilizar la instruccion:

pbkdf2_sha256.verify("claveingresada", "pbkdf2_sha256$180000$...........")

Pero me sale este mensaje:

...

ValueError: name : Not a valid  pbkdf2_sha256 hash

?  que metodo debo utilizar para validar la clave ingresada vs la almacenada en mi base de datos....he probado varios metodos...pero no funciona ninguno.....

[Florian Apolloner]

On Thursday, March 12, 2020 at 6:16:31 PM UTC+1, Dawid Czeluśniak wrote:

I think that the root question here is: should we allow users to create passwords from anything that is not str?

I would reduce it to allow strings & bytes and am willing to review a patch and push it through. It is security sensitive enough that I think we can ignore the (imo non-existant) backwards compatibility issues.

Cheers,

Florian

[charettes]

For the record a PR doing exactly that has been reviewed and merged already and should be of 3.1

https://github.com/django/django/pull/12627

Cheers,

Simon

[Mariusz Felisiak]

Hi Florian,

    We've already merged this change https://github.com/django/django/commit/8aa71f4e8706b6b3e4e60aaffb29d004e1378ae3.Please feel-free to review it and return any comments.

Best,
Mariusz

[Florian Apolloner]

oh, I somehow missed that. thanks for the heads-up.