Technical Board Decision Needed: Admin append_slash behaviour.

[Carlton Gibson]

Hi all, 

I need to ask for a Technical Board decision on the resolution of Ticket #31747. 

Ticket #31747: Avoid potential admin model enumeration

https://code.djangoproject.com/ticket/31747

PR #13134: Fixed #31747 -- Fixed model enumeration via admin URLs. 

https://github.com/django/django/pull/13134

## Summary

The initial issue is that there is a difference in HTTP response code for admin

URLs depending on whether a URL routes to an existing model or not:

   ​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

In principle an attacker could use this to leak information about the app's

models, which could be part of a further attack. 

This was originally reported to the Django Security Team, who declined to take

it as a security issue, but recommended adding a final catch-all view to the

admin, which would redirect all unauthenticated requests to login, thus masking

the private model info. 

Jon Dufresne picked this issue up, and submitted a PR, but in so-doing noticed

that a very similar (the same?) issue occurred with APPEND_SLASH behaviour in

the admin too. (Try URLs without the slash, looking for a redirect to the 

correct URL **before** being redirected to login.)

The initial thought was that we might need to remove APPEND_SLASH URL

normalisation for the admin. We were able to avoid that by restricting the

append_slash-style URL normalisation for the admin to authenticated staff

users. This addresses the main force of the original report (which is

unauthenticated users remotely probing the site) but Jon noted that it still

allows a staff-user to potentially discover the existence of model for which 

they don't have permissions in this way. 

In order to avoid this last threat Jon felt that we still needed to disable

append slash URL normalisation for the admin. 

In contrast, I felt the threat of a staff user discovering models in this way

would apply to only a tiny fraction of all installations, and that the utility

of URL normalisation vastly outweighs that on balance. 

We have a trade-off between balancing privacy and usability here. 

After much discussion and thought, Jon and I came to agree (I think 🙂) that an

`AdminSite.append_slash` toggle was needed to control the behaviour here. If

privacy is paramount, you can set that to `False` to disable the URL

normalisation in the admin. If you don't need to keep models secret from staff

users you can have it `True` and still get the benefits of append slash

behaviour. 

We felt this was something that everyone could live with. Wherever you are on

the spectrum, for the simplest case, where you're just using the default admin

site, you toggle the behaviour with a single line in your URL conf: 

    from django.contrib import admin

    admin.site.append_slash = ...set True/False as you want here...

(On the way, we also found a way of opting-out for a single ModelAdmin, but

let's not talk about that here.)

All good, except...

## Decision needed

Jon and I still don't agree on the correct default value for

`AdminSite.append_slash`. 

Our disagreement is a difference in weighting, which is hard to argue about,

and we don't want to fall out over it either. So we'd like to ask the Technical

Board to decide. 

### Option 1 - default `True`

As the PR stands: 

https://github.com/django/django/pull/13134

* `AdminSite.append_slash` defaults to `True`. 

* This is consistent with the past behaviour for authenticated staff users. 

### Option 2 - default `False`

* Make `AdminSite.append_slash` defaults to `False`. 

* This would be a breaking change, but...

* We could document it in the release notes, and...

* The "fix" is the simple adjustment in the URL conf for most folks. 

### Option 3 - default `False` with deprecation

* We could do 2, but somehow with a deprecation period. 

I don't think we should do this. It's difficult and a hard break at the end of 

the deprecation, but I mention it for completeness (and because the PR

originally included a deprecation of the APPEND_SLASH behaviour for the admin.)

My opinion is that we should opt for Option 1, as consistent with the past

behaviour, and most appropriate for most users. I can live with Option 2 if

that's the decision, but I think we'd artificially elevating an issue that

simply doesn't apply to most installs. 

I think Jon's opinion is the counterpart of that. He prefers Option 2, as the

more secure default. He could live with Option 1 if that's the decision, but

thinks we shouldn't be asking opt-in to privacy here, whatever the usability

consequences. 

As such, can we ask the TB to choose an option? Thanks. 

Jon: hopefully I'e presented your view fairly; if you feel not, please do 

follow-up 🙂

Kind Regards,

Carlton

Hi all, 

I need to ask for a Technical Board decision on the resolution of Ticket #31747. 

Ticket #31747: Avoid potential admin model enumeration

https://code.djangoproject.com/ticket/31747

PR #13134: Fixed #31747 -- Fixed model enumeration via admin URLs. 

https://github.com/django/django/pull/13134

## Summary

The initial issue is that there is a difference in HTTP response code for admin

URLs depending on whether a URL routes to an existing model or not:

   ​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

In principle an attacker could use this to leak information about the app's

models, which could be part of a further attack. 

This was originally reported to the Django Security Team, who declined to take

it as a security issue, but recommended adding a final catch-all view to the

admin, which would redirect all unauthenticated requests to login, thus masking

the private model info. 

Jon Dufresne picked this issue up, and submitted a PR, but in so-doing noticed

that a very similar (the same?) issue occurred with APPEND_SLASH behaviour in

the admin too. (Try URLs without the slash, looking for a redirect to the 

correct URL **before** being redirected to login.)

The initial thought was that we might need to remove APPEND_SLASH URL

normalisation for the admin. We were able to avoid that by restricting the

append_slash-style URL normalisation for the admin to authenticated staff

users. This addresses the main force of the original report (which is

unauthenticated users remotely probing the site) but Jon noted that it still

allows a staff-user to potentially discover the existence of model for which 

they don't have permissions in this way. 

In order to avoid this last threat Jon felt that we still needed to disable

append slash URL normalisation for the admin. 

In contrast, I felt the threat of a staff user discovering models in this way

would apply to only a tiny fraction of all installations, and that the utility

of URL normalisation vastly outweighs that on balance. 

We have a trade-off between balancing privacy and usability here. 

After much discussion and thought, Jon and I came to agree (I think 🙂) that an

`AdminSite.append_slash` toggle was needed to control the behaviour here. If

privacy is paramount, you can set that to `False` to disable the URL

normalisation in the admin. If you don't need to keep models secret from staff

users you can have it `True` and still get the benefits of append slash

behaviour. 

We felt this was something that everyone could live with. Wherever you are on

the spectrum, for the simplest case, where you're just using the default admin

site, you toggle the behaviour with a single line in your URL conf: 

    from django.contrib import admin

    admin.site.append_slash = ...set True/False as you want here...

(On the way, we also found a way of opting-out for a single ModelAdmin, but

let's not talk about that here.)

All good, except...

## Decision needed

Jon and I still don't agree on the correct default value for

`AdminSite.append_slash`. 

Our disagreement is a difference in weighting, which is hard to argue about,

and we don't want to fall out over it either. So we'd like to ask the Technical

Board to decide. 

### Option 1 - default `True`

As the PR stands: 

https://github.com/django/django/pull/13134

* `AdminSite.append_slash` defaults to `True`. 

* This is consistent with the past behaviour for authenticated staff users. 

### Option 2 - default `False`

* Make `AdminSite.append_slash` defaults to `False`. 

* This would be a breaking change, but...

* We could document it in the release notes, and...

* The "fix" is the simple adjustment in the URL conf for most folks. 

### Option 3 - default `False` with deprecation

* We could do 2, but somehow with a deprecation period. 

I don't think we should do this. It's difficult and a hard break at the end of 

the deprecation, but I mention it for completeness (and because the PR

originally included a deprecation of the APPEND_SLASH behaviour for the admin.)

My opinion is that we should opt for Option 1, as consistent with the past

behaviour, and most appropriate for most users. I can live with Option 2 if

that's the decision, but I think we'd artificially elevating an issue that

simply doesn't apply to most installs. 

I think Jon's opinion is the counterpart of that. He prefers Option 2, as the

more secure default. He could live with Option 1 if that's the decision, but

thinks we shouldn't be asking opt-in to privacy here, whatever the usability

consequences. 

As such, can we ask the TB to choose an option? Thanks. 

Jon: hopefully I'e presented your view fairly; if you feel not, please do 

follow-up 🙂

Kind Regards,

Carlton

[אורי‎]

Hi,

For security reasons, I would recommend protecting any url which starts with /admin/ (or the website's admin url prefix) with an HTTPS password from the web server directly (such as Nginx or Apache), so that even the login to the admin will be protected. You may consider adding this suggestion to Django's documentation. For example, if you enter https://en.speedy.net/admin/accounts/user/ you will be asked for an HTTPS authentication password. Only if you know the password, then you will be redirected to the login page, and only after you login you will be able to view the admin views. This increases security, as you will have to login twice. In my opinion you can consider adding this suggestion to Django's documentation.

Thanks,

Uri Rodberg

Speedy Net

אורי

u...@speedy.net

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/384107bd-511a-410e-8acb-e4a1f76b303cn%40googlegroups.com.

[Carlton Gibson]

Hi Uri. 

Can I please ask that this discussion not get side-tracked. 

I'm asking for a Technical Board decision on the specific question, under the rules of DEP 10. 

The PR in question has been in progress for six months, and we want to resolve it in time for the Django 3.2 feature freeze next week. 
Thanks. 

[Tim Graham]

As a non-technical board member, I'd prefer option 1. I also think that for most use cases, staff users are at least somewhat trusted and even if they are not, model enumeration isn't likely to be a security problem.

[James Bennett]

I'm going to be the contrarian here and at least ask whether the right answer is "don't do any of these options".

To see why, consider a hypothetical world where we do one of the above options, and a site sets whatever config is necessary to disable APPEND_SLASH behavior in the admin.

The very next thing we're going to get is a report to the security address saying there's a detectable timing difference between what we do to hide a real app/model from a user with admin-access-but-not-to-that-model/app, and what we do when that app/model genuinely doesn't exist, and in order to properly hide apps/models could we please address that?

(and if we do end up taking one of the above options, please consider this my report of this vulnerability, as I can pretty much guarantee that 404'ing a nonexistent app/model will be faster than finding it, resolving a URL into it, and doing the associated permission check)

And this will never end. For a user who already has been granted admin access, there likely *always* will be some detectable difference between the admin's behavior with respect to a model/app that isn't present versus one that is present but for which the user is not authorized. In an app with the complexity of the admin, there are just too many possible behavioral-difference vectors that people will be able to come up with, and we'll never be able to close them all reliably.

So what I'd really like us to do here is pause and consider what kinds of guarantees we can really commit to. Some are obvious and good and we already do or try to do them -- you shouldn't be able to access/modify/delete things you haven't been granted access to, you shouldn't be able to escalate your own privileges, you shouldn't be able to XSS the admin, etc.

But it's not at all clear to me that "the existence or nonexistence of apps/models to which they don't have access is undetectable to an admin user" is one of the guarantees the admin should make. I understand the rationale, but I'm not sure the threat it wants to be securing against is one we reasonably *can* secure against.

[Adam Johnson]

I agree with James, this is going down something of a rabbit hole of micro security holes within the trusted area of the admin.

There is also lower hanging fruit in the realm of admin security. For instance geodjango's admin extensions rely on external scripts and are not compatible with a strict CSP (the rest of the admin has long been compatible with a strict CSP).

Also, I believe there's a way that users can disable append_slash for their admins without a setting in Django, given the new "optional opt-out from append-slash" behaviour. It should be possible with a middleware like this, placed above CommonMiddleware:

class AdminNoAppendSlashMiddleware:
    def __init__(self, get_response):
        self.get_response = get_response
   
    def __call__(self, request):
        return self.get_response(request)
   
    def process_view(self, request, view_func, view_args, view_kwargs):
        if request.path.startswith("/my-admin/"):
            view_func.should_append_slash = False

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAL13Cg_1QRhht3eSD29AJ25Cbkp%2BfbCxyOi7GdhYpfzcEkmDbQ%40mail.gmail.com.

--

Adam

[Tom Forbes]

I agree with this especially around the point about timing attacks. 

I don’t believe potentially being able to infer the names of models in a very, very noisy way (thousands of requests) gives anyone leverage in a system or even any particularly sensitive information at all. Maybe in a really extreme scenario involving other issues within an app like some form of blind class-based direct object access vulnerability, but that’s really stretching the imagination.

If I had to pick I would say 1, but it still leaves open timing attacks as I’d expect the permission check path would take an order of magnitude longer than the fast “404 but not really” path. So it begs the question “do we really want to?”

Tom

On 6 Jan 2021, at 13:20, James Bennett <ubern...@gmail.com> wrote:



--

[Carlton Gibson]

OK, thanks all. So...

Two new options then:

1. Add the catch-all view to admin to stop the unauthenticated probing, as per the Security Teams initial idea, but not the AdminSite.append_slash option.

2. Don't even add the catch-all, and close the ticket as wontfix.

A site concerned here still has the middleware option with 2, but I imagine Jon arguing whether this is sufficiently secure by default.

Additional points:

* Middleware option did come up in the original discussion and on the PR.

* It SEEMS to me that the catch-all view does serve it's purpose as as the AdminSite.admin_view decorator redirects all non-staff requests equally to login (whether they exist or not, because the catch-all view exists.) This is prior to any per-view timing variation. (I think 🙂)

* So I'd say Option 1 here — I'll adjust the PR on that basis, but if you conclude that actually Option 2, do say.

Thanks again. This has been a difficult issue to think about and deal with, and I do appreciate the input and guidance.

Kind Regards,

Carlton

[Tim Graham]

I wouldn't object to a wontfix. It seems like we've already spent a lot of effort here for little benefit, if any.

[Florian Apolloner]

On Thursday, January 7, 2021 at 2:16:57 PM UTC+1 carlton...@gmail.com wrote:

1. Add the catch-all view to admin to stop the unauthenticated probing, as per the Security Teams initial idea, but not the AdminSite.append_slash option.

2. Don't even add the catch-all, and close the ticket as wontfix.

I think the catch-all view is certainly a worthwhile addition, it is a low hanging fruit that makes fast probing if auth.user is installed impossible.

* It SEEMS to me that the catch-all view does serve it's purpose as as the AdminSite.admin_view decorator redirects all non-staff requests equally to login (whether they exist or not, because the catch-all view exists.) This is prior to any per-view timing variation. (I think 🙂)

Technically you could already mount a timing attack because url resolving is not constant time, the first matching view wins :þ

Cheers,

Florian

[Markus Holtermann]

Thanks you for bringing this up, Carlton. And thanks Jon for tackling the issues.

I concur with what has been said so far. Especially what James said, that there are so many places where one possibly/maybe/theoretically could come up with timing attacks. Mitigating the difference in response code behavior (302 vs 404) seems like a sensible idea.

But adding the append slash behavior to the Admin seems unnecessary. Especially given the example Adam brought up. Maybe you want to post that approach on the corresponding ticket, Adam, and close it as wontfix?

Cheers,

Markus

> --
> You received this message because you are subscribed to the Google
> Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to django-develop...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/django-developers/03910826-32d4-44c9-a3d5-a35f984c05e7n%40googlegroups.com <https://groups.google.com/d/msgid/django-developers/03910826-32d4-44c9-a3d5-a35f984c05e7n%40googlegroups.com?utm_medium=email&utm_source=footer>.

[Adam Johnson]

I don't think we should mark the ticket as closed since we want to merge part of the open PR, the catch-all view.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/a19773d6-4482-45b6-aaf0-08f08626b398%40www.fastmail.com.

--

Adam