Avoid potential admin model enumeration

[Daniel Maxson]

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.

[Carlton Gibson]

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?

[Jon Dufresne]

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

[Carlton Gibson]

PR is adjusted. Putting back in the queue.

[Carlton Gibson]

I'm going to mark as Patch needs improvement for the moment. There's ​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.

[Jon Dufresne]

Addressed the above comments.

[Carlton Gibson]

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.

[Carlton Gibson]

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.

[GitHub <noreply@…>]

In ba31b010:

Fixed #31747 -- Fixed model enumeration via admin URLs.

Co-authored-by: Carlton Gibson <carlton.gibson@…>

[Mariusz Felisiak <felisiak.mariusz@…>]

In 4338b652:

Refs #31747 -- Added more tests for preserving query strings when redirect with APPEND_SLASH in admin.