Opened 7 months ago

Last modified 7 weeks ago

#28645 new Bug

AuthenticationForm's inactive user error isn't raised when using ModelBackend

Reported by: Guilherme Junqueira Owned by: shangdahao
Component: contrib.auth Version: 1.11
Severity: Normal Keywords: 2.1
Cc: Christoph Schwarzenberg Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Looking in file django.contrib.auth.forms

The class AuthenticationForm and clean method.

The inactive user never is raised, this happens because after Django 1.10 all users that is not active cannot authenticate, so self.user_chache is always be None for inactive users, even if has a correct user and pass.

So the code needed to be changed to raise the correct error for a user that is not active.

My stackoverflow thread about this:
https://stackoverflow.com/questions/46459258/how-to-inform-a-user-that-he-is-not-active-in-django-login-view/46459998#46459998

Attachments (1)

28645-jc.diff (5.5 KB) - added by Tim Graham 3 months ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 months ago by Tim Graham

Summary: The inactive user error never is raised in login formAuthenticationForm's inactive user error isn't raised when using ModelBackend
Triage Stage: UnreviewedAccepted

comment:2 Changed 7 months ago by shangdahao

Owner: changed from nobody to shangdahao
Status: newassigned

comment:3 Changed 6 months ago by shangdahao

Has patch: set

comment:4 Changed 6 months ago by Tim Graham

Patch needs improvement: set

comment:5 Changed 6 months ago by shangdahao

Patch needs improvement: unset

comment:6 Changed 6 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 359370a:

Fixed #28645 -- Reallowed AuthenticationForm to raise the inactive user error when using ModelBackend.

Regression in e0a3d937309a82b8beea8f41b17d8b6298da2a86.

Thanks Guilherme Junqueira for the report and Tim Graham for the review.

comment:7 Changed 6 months ago by Tim Graham <timograham@…>

In 36dd0126:

[2.0.x] Fixed #28645 -- Reallowed AuthenticationForm to raise the inactive user error when using ModelBackend.

Regression in e0a3d937309a82b8beea8f41b17d8b6298da2a86.

Thanks Guilherme Junqueira for the report and Tim Graham for the review.

Backport of 359370a8b8ca0efe99b1d4630b291ec060b69225 from master

comment:8 Changed 6 months ago by Tim Graham <timograham@…>

In 308f644:

[1.11.x] Fixed #28645 -- Reallowed AuthenticationForm to raise the inactive user error when using ModelBackend.

Regression in e0a3d937309a82b8beea8f41b17d8b6298da2a86.

Thanks Guilherme Junqueira for the report and Tim Graham for the review.

Backport of 359370a8b8ca0efe99b1d4630b291ec060b69225 from master

comment:9 Changed 3 months ago by Tim Graham <timograham@…>

In af33fb25:

Fixed CVE-2018-6188 -- Fixed information leakage in AuthenticationForm.

Reverted 359370a8b8ca0efe99b1d4630b291ec060b69225 (refs #28645).

This is a security fix.

comment:10 Changed 3 months ago by Tim Graham <timograham@…>

In c37bb286:

[2.0.x] Fixed CVE-2018-6188 -- Fixed information leakage in AuthenticationForm.

Reverted 359370a8b8ca0efe99b1d4630b291ec060b69225 (refs #28645).

This is a security fix.

comment:11 Changed 3 months ago by Tim Graham <timograham@…>

In 57b95fed:

[1.11.x] Fixed CVE-2018-6188 -- Fixed information leakage in AuthenticationForm.

Reverted 359370a8b8ca0efe99b1d4630b291ec060b69225 (refs #28645).

This is a security fix.

comment:12 Changed 3 months ago by Tim Graham

Has patch: unset
Keywords: 2.1 added
Resolution: fixed
Status: closednew

Reopening since the fix had to be reverted. We'll try to develop a solution for Django 2.1. Probably the solution will be too invasive to backport to the stable branches.

In a mail to the security mailing list, Jack Cushman suggested:

It's desirable for auth backends to enforce rules like “no inactive users” when supplied with otherwise-correct credentials – that’s more of a backend concern than a display concern, and forms shouldn’t be required to enforce it. But it is desirable for auth forms to show custom error messages when an auth backend rejects a user, if and only if the user supplied correct credentials. The ideal way to solve both problems would be for auth backends to return a tuple of (user or None, custom_error_code or None), but that would break backwards compatibility.

So, can we let auth backends return custom error codes with backwards compatibility?

Attached is an untested patch that hopefully does that, by adding an authenticate_with_error_code method that backends can optionally implement and forms can optionally consume. I think this is a good angle on the problem – it cleans up the can of worms with displaying custom errors, and also totally avoids dealing with attacker-submitted data after credentials fail to validate, which is key to avoiding any subtle security issues.

I'll attach the patch, but I haven't evaluated it in much detail.

Changed 3 months ago by Tim Graham

Attachment: 28645-jc.diff added

comment:13 Changed 7 weeks ago by Christoph Schwarzenberg

Cc: Christoph Schwarzenberg added

comment:14 Changed 7 weeks ago by Christoph Schwarzenberg

Maybe it is enough to check the supplied password.

I've modified the code from shangdahao accordingly:
https://stackoverflow.com/a/49138231/9453030

comment:15 Changed 7 weeks ago by Tim Graham

That approach may leak whether or not a username exists because of the time it takes to hash a password. For user names that exist, password hashing will run twice compared to once for user names that don't exist. See #20760 for a past example.

Note: See TracTickets for help on using tickets.
Back to Top