#16845 closed Cleanup/optimization (fixed)
Admin should hide password hash field by default
Reported by: | Paul McMillan | Owned by: | Paul McMillan |
---|---|---|---|
Component: | contrib.auth | Version: | 1.3 |
Severity: | Release blocker | Keywords: | |
Cc: | Donald Stufft | 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)
Change History (14)
comment:1 by , 13 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 13 years ago
follow-up: 4 comment:3 by , 13 years ago
For the API, maybe something like ModelAdmin.sensitive_fields
?
comment:4 by , 13 years ago
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 by , 13 years ago
Yes good point, this is an auth-specific thing.
comment:7 by , 13 years ago
Severity: | Normal → Release blocker |
---|
Marking as a release blocker and assigning to myself.
comment:8 by , 13 years ago
Owner: | changed from | to
---|
by , 13 years ago
Attachment: | mask-password-field.diff added |
---|
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 by , 13 years ago
Cc: | 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 by , 13 years ago
Oh and I forgot! An example of the new value: http://d.stufft.io/1G3222142A2g002r2o3B
comment:12 by , 13 years ago
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*
toReadOnlyPasswordHash*
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
forUserChangeForm.password
so that it's friendlier for people who are not sysadmin types.
comment:13 by , 13 years ago
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.
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.