Opened 6 weeks ago

Last modified 3 weeks ago

#31747 assigned New feature

Avoid potential admin model enumeration

Reported by: Daniel Maxson Owned by: Jon Dufresne
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (5)

comment:1 Changed 6 weeks ago by Carlton Gibson

Summary: Model enumerationAvoid potential admin model enumeration
Triage Stage: UnreviewedAccepted
Type: BugNew feature

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?

comment:2 Changed 6 weeks ago by Jon Dufresne

Has patch: set

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

comment:3 Changed 5 weeks ago by Carlton Gibson

Patch needs improvement: set
Version: 3.0master

comment:4 Changed 5 weeks ago by Carlton Gibson

Patch needs improvement: unset

PR is adjusted. Putting back in the queue.

comment:5 Changed 3 weeks ago by Nick Pope

Owner: changed from nobody to Jon Dufresne
Status: newassigned
Note: See TracTickets for help on using tickets.
Back to Top