Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#16845 closed Cleanup/optimization (fixed)

Admin should hide password hash field by default

Reported by: PaulM Owned by: PaulM
Component: contrib.auth Version: 1.3
Severity: Release blocker Keywords:
Cc: dstufft Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Django's admin allows administrators to view all fields on the User model, including the password hash. While this does not directly reveal the password, it is sensitive information and most administrators do not need to directly view or set it.

Allowing admins to view this information means that an attacker who compromises an admin account (via cookie theft or other means) has direct access to the password hashes for all users, facilitating offline cracking attacks. If we hide this information by default in the admin, it is much harder for an attacker to gather this information, and it means that the damage is limited to just the compromised django site, rather than every other site where users re-used those passwords.

We already hide sensitive information in tracebacks, and so we should hide this information as well.

Attachments (1)

mask-password-field.diff (2.7 KB) - added by dstufft 4 years ago.
Masks the Password Field in Admin to only show first N (6 by default) Digits and the hash type. Also makes the field read only.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by PaulM

The only semi-common use case I could come up with for an admin needing to see information in this field is to verify which type of password hashing scheme is in use for the user. We could solve that by displaying the field as a non-editable sanitized field with the hashing info visible.

Admins who are copy-pasting user password strings can probably find some other way to deal with this that doesn't involve horrible practices like that.

comment:3 follow-up: Changed 4 years ago by julien

For the API, maybe something like ModelAdmin.sensitive_fields?

comment:4 in reply to: ↑ 3 Changed 4 years ago by carljm

Replying to julien:

For the API, maybe something like ModelAdmin.sensitive_fields?

I don't think this requires a public API; I'm not sure what generic handling we could provide for "sensitive fields" that would be useful. If user code has fields admins shouldn't see, they can already easily omit those fields from the ModelAdmin entirely using fields or fieldsets, or provide specialized handling via custom code. If we want to do something special here like display the hash algorithm only, that would appropriately be solved with custom code in the ModelAdmin for User.

comment:5 Changed 4 years ago by julien

Yes good point, this is an auth-specific thing.

Version 0, edited 4 years ago by julien (next)

comment:6 Changed 4 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:7 Changed 4 years ago by PaulM

  • Severity changed from Normal to Release blocker

Marking as a release blocker and assigning to myself.

comment:8 Changed 4 years ago by PaulM

  • Owner changed from nobody to PaulM

Changed 4 years ago by dstufft

Masks the Password Field in Admin to only show first N (6 by default) Digits and the hash type. Also makes the field read only.

comment:9 Changed 4 years ago by dstufft

  • Cc dstufft added
  • Has patch set

Added an implementation of this. It uses a custom Field/Widget type that displays the hash type and the first N characters of the hash for verification purposes. I'm not sure if I did everything correctly as far as Django's contributing guidelines (first patch!), hopefully this is useful :).

comment:10 Changed 4 years ago by dstufft

Oh and I forgot! An example of the new value: http://d.stufft.io/1G3222142A2g002r2o3B

comment:11 Changed 4 years ago by adrian

  • Resolution set to fixed
  • Status changed from new to closed

In [17185]:

Fixed #16845 -- Admin 'Change user' page no longer shows the password hash. Thanks, dstufft

comment:12 Changed 4 years ago by adrian

Thanks for the patch, dstufft. I made these changes before committing:

  • Made a mask_password function so as not to duplicate the logic.
  • Renamed ReadOnlyHashedPassword* to ReadOnlyPasswordHash* because I thought it was more readable/direct.
  • Removed the help_text from the User.password field now that we're not using that.
  • Moved the "if not value" check to be the first statement within ReadOnlyPasswordHashWidget.render() so as to fail faster, in that case.
  • Rewrote the help_text for UserChangeForm.password so that it's friendlier for people who are not sysadmin types.

comment:13 Changed 4 years ago by dstufft

Fwiw this is going to need to be updated once PaulM finishes the Password Hashing. I actually had talked to him on IRC (Suppose I should have mentioned it in here) and discussed and committed to updating the patch one the Password Hashing got merged into trunk.

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