[Django] #31747: Model enumeration
Django
-----------------------------------------+------------------------
Reporter: Daniel Maxson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | 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 |
-----------------------------------------+------------------------
When attacking a default Django server (one created with startproject is
sufficient), you can enumerate the names of the applications and models by
fuzzing the admin URL path. A redirect indicates that the model exists in
that application, while a 404 indicates it does not. This can be done
without any authentication. For example:
http://127.0.0.1:8000/admin/auth/user/ -> 302 to login
http://127.0.0.1:8000/admin/auth/group/ -> 302 to login
http://127.0.0.1:8000/admin/auth/foo/ -> 404
I originally raised this with the security team and after some discussion
they requested a public ticket for this.
--
Ticket URL: <https://code.djangoproject.com/ticket/31747>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------+------------------------------------
Reporter: Daniel Maxson | Owner: nobody
Component: contrib.admin | Version: 3.0
Severity: Normal | Resolution:
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):
* type: Bug => New feature
* stage: Unreviewed => Accepted
Comment:
Thanks Daniel.
Pasting here extracts from the security team discussion. This idea of
adding a `final_catch_all_pattern` seems most likely:
> Well, I vaguely recalled, and managed to find,
> https://groups.google.com/d/msgid/django-
developers/CAHdnYzu2zHVMcrjsSRpvRrdQBMntqy%2Bh0puWB2-uB8GOU6Tf7g%40mail.gmail.com
> where we discussed achieving similar goals using middlewares rather
> than view or URL manipulation. I believe a middleware which translates
> 404 exceptions in admin urls for unauthenticated users into redirects
> to the admin login would essentially solve the problem[1], without
> breaking existing URLConfs.
>
> If this solution of the issue is accepted, I'm not sure how to apply it
> to existing projects -- adding a middleware may require them to edit
> their settings, not just to upgrade (although the vulnerability may be
> light enough to handle by providing the middleware and just
> recommending its use).
>
> [1] Assuming we make no special effort to avoid it, this information
> may still be leaked using a timing attack -- that is, it should take a
> little more time to respond to a URL for a non-existing model. I'm not
> sure protecting against that would be worthwhile.
> > I believe a middleware which translates
> > 404 exceptions in admin urls for unauthenticated users into redirects
> > to the admin login would essentially solve the problem[1], without
> > breaking existing URLConfs.
>
> The main issues I see with a middleware is:
> a) how to enable it (as you also noticed)
> b) which URLs should it apply to?
>
> I think the latter is rather important since we explicitly advise to not
use /admin in the first place :) With an URLConf approach we could just
add a catch-all pattern to the end of admin.site.urls (with all issues
that follow from that). We might add an attribute ala
`final_catch_all_pattern = True` to `AdminSite` to allow disabling this
rather light issue (then it would be up to the site developer to add a
final catch all)
>
> FWIW this issue also applies to authenticated model enumeration
(basically a site which allows login but has limited staff/admin access --
something we should consider when coming up with the patch).
> You are correct, of course. The middleware-based approach requires
> extra configuration -- a setting, probably; and I agree that this is a
> little ugly (my thinking was a little influenced by the tags idea in
> the thread I referred to, which would mostly allow overcoming this, I
> think).
>
> > With an URLConf approach we
> > could just add a catch-all pattern to the end of admin.site.urls
> > (with all issues that follow from that). We might add an attribute
> > ala `final_catch_all_pattern = True` to `AdminSite` to allow
> > disabling this rather light issue (then it would be up to the site
> > developer to add a final catch all)
>
> That seems good enough. I'd consider, in this case, adding the option
> with default False in 3.0.x and making the non-backward-compatible
> change only in 3.1.
>
> > FWIW this issue also applies to authenticated model enumeration
> > (basically a site which allows login but has limited staff/admin
> > access -- something we should consider when coming up with the patch).
>
> IIUC, this code[1] does it, and it is arguably incorrect in the case of
> an authenticated user accessing a model they don't have permission for.
> What if we just changed that to raise a 404? Wouldn't that fix both
> cases? FWIW, that is what Github does when you try to access a private
> repo you don't have access to.
>
>
[1]https://github.com/django/django/blob/master/django/contrib/admin/sites.py#L222-L232
> > What if we just changed that to raise a 404? Wouldn't that fix both
> > cases? FWIW, that is what Github does when you try to access a private
> > repo you don't have access to.
> >
> >
[1]https://github.com/django/django/blob/master/django/contrib/admin/sites.py#L222-L232
>
> Raising a 404 would definitely help here, but the usability of that
> sucks imo.
>
> Do we think we can fix this in public and gather more input?
--
Ticket URL: <https://code.djangoproject.com/ticket/31747#comment:1>
Django
-------------------------------+------------------------------------
Component: contrib.admin | 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 Jon Dufresne):
* has_patch: 0 => 1
Comment:
> We might add an attribute ala final_catch_all_pattern = True to
AdminSite
I used this approach in https://github.com/django/django/pull/13134
--
Ticket URL: <https://code.djangoproject.com/ticket/31747#comment:2>
Django
-------------------------------+------------------------------------
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
* version: 3.0 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/31747#comment:3>
Django
-------------------------------+------------------------------------
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 1 => 0
Comment:
PR is adjusted. Putting back in the queue.
--
Ticket URL: <https://code.djangoproject.com/ticket/31747#comment:4>
Django
Reporter: Daniel Maxson | Owner: Jon Dufresne
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nick Pope):
* owner: nobody => Jon Dufresne
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31747#comment:5>
Django
-------------------------------+----------------------------------------
Reporter: Daniel Maxson | Owner: Jon Dufresne
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
Comment:
I'm going to mark as Patch needs improvement for the moment. There's
[https://github.com/django/django/pull/13134 some discussion on the PR] as
to the best way forwards ref a break of APPEND_SLASH behaviour for the
admin.
It looks like we can make it work for non-staff users well enough, but
there's an edge-case for staff-users with a missing permission. I would
propose to move forward with that solution as a win now.
We can then come back to the more difficult question of changing the
APPEND_SLASH behaviour for the admin, which probably needs more discussion
to get right.
--
Ticket URL: <https://code.djangoproject.com/ticket/31747#comment:6>
Django
-------------------------------+----------------------------------------
Reporter: Daniel Maxson | Owner: Jon Dufresne
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jon Dufresne):
* needs_better_patch: 1 => 0
Comment:
Addressed the above comments.
--
Ticket URL: <https://code.djangoproject.com/ticket/31747#comment:7>
Django
-------------------------------+----------------------------------------
Reporter: Daniel Maxson | Owner: Jon Dufresne
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
Comment:
There are various options on the PR to maintain the historic APPEND_SLASH
behavior for the admin catchall view to only staff users, with options for
exactly how we allow users to control that on a per ModelAdmin or per
AdminSite basis.
Spectrum of options would then be:
Redirect only for staff. (Default.)
Turn off per model admin overriding get_urls()
Optionally: Turn off per model admin with flag &/or Turn off at Admin
site with flag.
Set APPEND_SLASH=False.
All of which solve the problem of unauthenticated enumeration of existing
models via URL admin redirect behavior, but don't require removing
APPEND_SLASH itself.
--
Ticket URL: <https://code.djangoproject.com/ticket/31747#comment:8>
Django
Reporter: Daniel Maxson | Owner: Jon
| Dufresne
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
Comment:
OK, we're ready to go here, bar a TB decision on whether
`AdminSite.append_slash` should default to `True` (keeping the current
behaviour) or `False` (defaulting to `off`) for which the thread is here:
https://groups.google.com/g/django-developers/c/XW6fsFkbFrY
Going to mark RFC just to keep out of the way while we wait for a
decision.
--
Ticket URL: <https://code.djangoproject.com/ticket/31747#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Maxson | Owner: Jon
| Dufresne
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31747#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Maxson | Owner: Jon
| Dufresne
Component: contrib.admin | Version: master
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 GitHub <noreply@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ba31b0103442ac891fb3cb98f316781254e366c3" ba31b010]:
{{{
#!CommitTicketReference repository=""
revision="ba31b0103442ac891fb3cb98f316781254e366c3"
Fixed #31747 -- Fixed model enumeration via admin URLs.
Co-authored-by: Carlton Gibson <carlton...@noumenal.es>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31747#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Maxson | Owner: Jon
| Dufresne
Type: New feature | Status: closed
Severity: Normal | Resolution: fixed
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:"4338b6526d3604b371ac37134371946baa1ed0ce" 4338b652]:
{{{
#!CommitTicketReference repository=""
revision="4338b6526d3604b371ac37134371946baa1ed0ce"
Refs #31747 -- Added more tests for preserving query strings when redirect
with APPEND_SLASH in admin.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31747#comment:12>