Opened 4 years ago

Closed 3 years ago

#14597 closed New feature (fixed)

request.is_secure() should support headers like: X-Forwarded-Protocol and X-Forwarded-Ssl

Reported by: gnotaras Owned by: adrian
Component: HTTP handling Version: 1.2
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

request.is_secure() should support checking the existence of an HTTP header like X-Forwarded-Protocol and/or X-Forwarded-Ssl. Currently, request.is_secure() relies only upon the existence of the environment variable HTTPS, which causes problems in cases that a request is proxied to Django from another web server, eg nginx.

Related tickets in other projects:

Of course, it is always possible to export the HTTPS=on environment variable to the shell that invokes the django server process, but supporting any of the aforementioned or other relevant http headers would make things easier and more straight-forward when setting up secure websites.

Change History (18)

comment:1 Changed 4 years ago by DrMeers

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Luke Plant provided a workaround for this issue here: http://djangosnippets.org/snippets/1706/

I don't know enough about the standards to determine whether such support should be built into the core.

comment:2 Changed 4 years ago by lukeplant

  • Resolution set to wontfix
  • Status changed from new to closed

As I said in the comments in that snippet, clients are able to set those headers themselves and there is no guarantee they will be removed by the webserver if it isn't an HTTPS connection. In my testing, I was able to use Firefox to spoof an HTTPS connection very easily.

I still use the WebFactionFixes middleware because I don't think this opens up any vulnerabilities given the way in which my site uses 'request.is_secure', apart from users deliberately downgrading themselves to HTTP on parts of the site that are meant to be HTTPS, and I trust my users not to be silly enough (or knowledgeable enough) to do that. But for some sites that might not be acceptable however - if your security requirements say that only HTTPS to the site should be allowed, then this code will punch a hole in that. For that reason, I'm going to close as WONTFIX. People are free to use the workaround in that snippet if it is appropriate. This is essentially the same situation as SetRemoteAddrFromForwardedFor which was trusting potentially untrustworthy information, and was actually removed in Django 1.1 because people relied on it without checking — only it would be worse with the proposed changes, because you wouldn't be able to turn it off.

comment:3 Changed 4 years ago by gnotaras

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Closed? Please folks..., not so fast. I am quite certain we can find a much better resolution than just close this issue. So, with great respect to the djangosnippets code, I am reopening this ticket.

Please, let me describe the situation:

Django running behind reverse proxies is a common case. In the case that the reverse proxy serves requests over HTTPS, it should be possible to make this piece of information available to the Django application server. It is not possible to push an environment variable. So, we got only HTTP request headers to play with.

It turns out that there are two problems related to Django's request.is_secure() relying upon HTTP headers in order to return True. These problems are:

  • The aforementioned headers X-Forwarded-Protocol and X-Forwarded-Ssl (refered to HTTPS headers hereafter) are not part of any official standards, but are being used as a convention. Besides there are implementations that use other similar names. X-Forwarded-Proto are Front-End-Https are some of them.
  • It is possible for any client to maliciously set any of these HTTPS headers while sending the request to the reverse proxy. In my tests the headers were not filtered and were pushed as is to the application server, which is probably the right thing to do as the headers are not part of any standards and the reverse proxy wouldn't be able to know which headers to filter. In this case, the web application is the sole responsible for any security holes its use of request.is_secure() might open up. Django should stay on the safe side and not rely upon any of the HTTPS headers in order to determine the SSL status.

But the problem still remains. The reverse proxy still has to notify the app server that it serves requests to clients over the HTTPS protocol. By closing this ticket we make a decision that should be made by the system administrator. And this is an issue as well.

So, as a solution to the problems above, I think we could give the administrator the ability to decide whether the app server should trust any of the HTTPS headers by adding a new setting. For instance:

REVERSE_PROXY_HTTPS_HEADERS = {
    ('x-forwarded-protocol', 'https'),
    ('x-forwarded-ssl', 'on'),
}

The dictionary contains the header name/value pairs that Django's request.is_secure() should trust in order to return True. The REVERSE_PROXY_HTTPS_HEADERS setting should default to an empty dictionary.

So, it is up to the admin to set his reverse proxy push a specific HTTP header to the app server and also set the app server to trust that specific header. In my opinion this new setting will provide a real resolution to the existing issue.

The last call is always yours. If you think this is useful and that it resolves an existent problematic situation, then please leave this ticket open so as to receive patches and more comments.

If you still want to close it as wontfix, then I'll stfu and never comment on this ticket again.

comment:4 Changed 4 years ago by lukeplant

  • Resolution set to wontfix
  • Status changed from reopened to closed

My logic regarding this is as follows:

  1. This new setting is essentially equivalent to providing a ReverseProxyHttpsHeadersMiddleware that people can add to MIDDLEWARE_CLASSES - except that the middleware would probably be a better option, since it would require less code in core, and is more easily swapped out to cope with other situations and custom logic. Also there is, for good reason, strong reluctance to add new settings where they are not absolutely necessary.
  1. In the past, we had the SetRemoteAddrFromForwardedFor middleware which also existed to cope with the 'very common' reverse proxy situation.
  1. That middleware suffered from the problem that it was not generally reliable, and, despite documentation to the contrary, many people assumed that it was safe to trust the values it inserted, when in fact it was only safe in some circumstances.
  1. This problem was significant enough that we ended up removing the middleware, and requiring that people who used it should copy-paste the old implementation to their project, ensuring that it met their requirements.
  1. With the proposed REVERSE_PROXY_HTTP_HEADERS, or with the equivalent ReverseProxyHttpsHeadersMiddleware, a very similar problem exists. In both your tests and mine, in none of the configurations so far discussed is it safe to trust those headers, since the client can always add them, contrasting to the situation with X-Forwarded-For which is usually more reliable if you are actually in a reverse proxy situation (AFAIK). Certainly it is no better than that situation.
  1. As the SetRemoteAddrFromForwardedFor fiasco showed, you cannot in general support proxies - there are too many possible configurations. Any implementation we baked into core would immediately be wrong for many situations.
  1. This is a security related question - it is very possible that the security of some sites is going to be seriously affected if request.is_secure() returns True when it should return False.
  1. Requiring people to write and include their own ReverseProxyHttpsHeadersMiddleware is essentially a very similar burden to requiring them to write and include their own SetRemoteAddrFromForwardedFor, and we were quite happy to do the latter, and even introduce a backwards incompatibility to do so. In fact they can use django-heroism or something similar to make it even easier.

With these in mind, I find it very difficult to come to any other conclusion. Surely if we added this we would just be repeating a past mistake. What am I missing?

Also, by closing this ticket we are not "making a decision that should be made by a system administrator", as they can add their own middleware to do this. We are simply refusing to include a generally insecure mechanism in Django itself.

So I'm going to re-close, since you didn't show how this situation is any different from the SetRemoteAddrFromForwardedFor which I mentioned in my last comment. Feel free to bring this up on django-developers if you want a second opinion.

comment:5 Changed 4 years ago by gnotaras

@Luke: First of all thanks for your detailed reply.

I agree that a middleware is often the preferred way, but as I explained above, the reverse-proxy/app-server admin has to somehow specify which headers the app_server should trust regarding the protocol that is used by the reverse-proxy to communicate with the clients. I assume that it is of those cases that a new setting is absolutely necessary, so as not to hard-code any headers that django would trust by default.

I think that the resolution I suggested in the previous message is fundamentally different from the SetRemoteAddrFromForwardedFor case, in which Django relied upon a specific HTTP header which could be easily set by a malicious client.

What I suggested was to use the REVERSE_PROXY_HTTPS_HEADERS dictionary (which defaults to {} as mentioned earlier) as a facility where the administrator of the project could manually specify which HTTP headers the app_server should trust in order to assume that it works under HTTPS. No header would be hard-coded. The admin could set the proxy server to sent a header like: X-My-SSL-Header: ssl_is_on_123AAA to the app_server.

Then in the app_server's settings.py:

# Trusted HTTP headers for secure operation
REVERSE_PROXY_HTTPS_HEADERS = {
    ('HTTP_X_MY_SSL_HEADER', 'ssl_is_on_123AAA'),
}
  1. The malicious client should have to guess the header name and value that the app_server trusts. (This was not the case in the SetRemoteAddrFromForwardedFor situation)
  2. No standards are broken, since such headers are not covered by any standards. (It would be impossible to resolve the SetRemoteAddrFromForwardedFor situation without breaking any standards)

But, anyway. I'll probably write and publish an application that provides this kind of resolution to the aforementioned problem.
Thanks for your replies and your time.

Over and out.
--
George Notaras

comment:6 Changed 4 years ago by lukeplant

Your latest proposal addresses some of the concerns about security, but as soon as you include it in Django, you will find hosts like WebFaction (where the customer is not in charge of those HTTPS headers) saying things like "put these settings in your settings.py to make it work" - and the developers will never read all the big fat disclaimers about those settings. I'm not saying that WebFaction are stupid or anything like that, it is just human nature to find the quickest solution to a problem.

I still think that the solution is to put the burden of this logic and its security implications onto the developers — just like we solved SetRemoteAddrFromForwardedFor by removing it, not by making customizable.

comment:7 Changed 4 years ago by gnotaras

Okay, I understand your concerns and you are right about what will probably happen with web hosting providers. Nevertheless, there is still an easy workaround, which appears to be the only true effortless way to proceed right now. To set the HTTPS variable to "on" to the app_server's environment. For example:

HTTPS=on python manage.py runserver 127.0.0.1:8000

This also makes Django's request.is_secure() to always return True, which makes any security checks based on this function to give a false sense of security, if we may call it that way.

What I am saying is that there is always going to be the easy workaround. But is it a big deal?

To go even further I think we are spending way too much time on the "security implications" of the suggested workaround. This is just about determining whether the SSL protocol is used or not. This is not about authenticating the client based on a client certificate, in which case I assume that custom headers and even SSL communication are a must for trusted reverse_proxy<->app_server communication. There is no web application (at least known to me) that will reveal any sensitive information based only on the fact that a proxy server indicates that it communicates with the client over SSL. Even if a client sends a header like X-Forwarded-Protocol, it would have to previously trick the reverse-proxy, and, even if the client succeeded (possible if the proxy permits both SSL and non-SSL access to a specific resource), it would still have to provide some valid authentication information to the app_server before it could access any sensitive information. And even if it still succeeded it would be the client's own sensitive information that would travel in the clear. What is the big deal with that? Unless we are talking about really buggy web applications. But why should we care about such applications?

comment:8 follow-up: Changed 4 years ago by lukeplant

This is the big deal: Django is a general purpose web framework, and request.is_secure() should be correct. End of story.

But, for the sake of argument, here is an attack scenario: user makes request to http://example.com . The web app, running Django behind a reverse proxy which is controlled by a third party (e.g. WebFaction), doesn't allow HTTP connections, and redirects everything to HTTPS using a custom piece of middleware that checks request.is_secure(). However, an active network attacker notices the HTTP connection and adds the X-Forwarded-Protocol header. The web app is fooled, and sends back a response over HTTP instead of doing a redirect, which the network attacker can read, possibly gaining sensitive information.

comment:9 follow-up: Changed 4 years ago by gnotaras

I have explained numerous times that I didn't suggest hard-coding any non-standard http headers like X-Forwarded-Protocol into django, but let the web app admin specify them himself in the settings.py and the reverse-proxy configuration. Web hosting providers, if they care about their client's security should give them the chance to specify the HTTPS headers the reverse-proxy would send to the client's web app.

In the end I come to the conclusion that the developers of other applications that check such headers like cherypy, gunicorn. drupal, etc must be stupid.

Again, I remind that we are talking about the simple use case, where SSL is just used to prevent the user's data from travelling in the clear. SSL, in this case, is used by the user for his own protection. It is the user who should care about connecting to the https resource in the first place.

BTW, supposing that you are 100% right, what method would you consider secure enough for the web_app to determine the protocol that is being used in the communication between the reverse-proxy and the client?

Anyway, it may be fun commenting on this ticket, but this conversation is getting nowhere.

I wouldn't say "I hope", because I am 100% certain that there will be other reports like this one, because this is a real issue when reverse-proxies are used.

I am out of this conversation. Thanks for your time.

comment:10 in reply to: ↑ 8 Changed 4 years ago by gnotaras

Replying to lukeplant:

This is the big deal: Django is a general purpose web framework, and request.is_secure() should be correct. End of story.

However, the real deal is that when a reverse proxy, which communicates with the clients over SSL, is used, request.is_secure() is wrong.

But, for the sake of argument, here is an attack scenario: user makes request to http://example.com . The web app, running Django behind a reverse proxy which is controlled by a third party (e.g. WebFaction), doesn't allow HTTP connections, and redirects everything to HTTPS using a custom piece of middleware that checks request.is_secure(). However, an active network attacker notices the HTTP connection and adds the X-Forwarded-Protocol header. The web app is fooled, and sends back a response over HTTP instead of doing a redirect, which the network attacker can read, possibly gaining sensitive information.

BTW, I checked your snippet at http://djangosnippets.org/snippets/1706/ and all other relevant snippets. They are all vulnerable to the hypothetical attack scenario you've described above as they use hard-coded headers. I think that my suggestion is better and I suggest you put the Ego aside and re-open the ticket for further discussion. At least in this case, none of your arguments is valid enough so as to close the ticket.

There will be one last comment of mine where I will point to an app that deals with this issue.

comment:11 Changed 4 years ago by russellm

@gnotaras -- When someone disagrees with you, it doesn't necessarily follow that ego is the cause, and pointing to ego as a cause doesn't serve to progress an enlightened debate.

As far as I can make out, Luke has made an entirely reasonable and levelheaded argument. You may disagree with Luke's assumptions, or believe you've found a hole in his reasoning, but I see nothing that points to Luke's ego being the cause of that disagreement. On the other hand, "I think my suggestion is better and therefore you should change your opinion" is neither a compelling logical argument, nor is it especially ego-free.

Also - as we describe in our contributions gude, Trac is not the right place to have technical design discussions. You opened a ticket, and a core dev closed it. If you disagree with Luke's reasoning, the right place to have this discussion is on django-developers, where it will get attention from the entire developer community, not just the people who are watching this specific ticket.

comment:12 Changed 4 years ago by gnotaras

@russellm: Thanks for pointing me to the contributors guide. I was not aware of that document. I thought Trac was the right place to discuss the issue and that's why I insisted on keeping the ticket open. As for the issue itself, reading this discussion again I am certain that I have nothing more to add to it. I sincerely do not see why this should be discussed in the developers mailing list. Ticket is resolved as wontfix. Let's just forget about it.
:)

comment:13 in reply to: ↑ 9 Changed 4 years ago by lukeplant

Replying to gnotaras:

BTW, supposing that you are 100% right, what method would you consider secure enough for the web_app to determine the protocol that is being used in the communication between the reverse-proxy and the client?

I realise you're not wanting to continue any discussion, but this is one question that is so far unanswered, so I'll try to answer.

If you have a situation where you have control of the proxy and can ensure an un-guessable header is added, that sounds secure enough to me. Or if the proxy strips the header from the incoming request or otherwise ensures that it is always correct, that would be secure. But that is wishful thinking for many scenarios. WebFaction does not do this, and thus the snippet I posted is quite insecure in that regard — as you pointed out. I created it and use it because I have no other choice (and I've asked WebFaction to improve the way they do this). The theoretical problem with it, the fact that typical server configurations make this theoretical problem into an actual one, and the fact that this is very server specific, are precisely the reasons I made a snippet and didn't go ahead and add it straight to Django itself.

For me, the lack of any reliable solution to this problem is definitely not a reason to support an unreliable solution. It should instead be used as pressure on the web hosts to come up with a decent solution.

comment:14 Changed 4 years ago by gnotaras

@lukeplant: Thanks for your reply and sorry if I got over-excited about keeping this ticket open. I have already written above that I understand your concerns and I also have the same concerns regarding web hosts.

comment:15 Changed 4 years ago by PaulM

  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset

I raised this issue on the django-dev mailing list. Hopefully we can re-examine this issue.

http://groups.google.com/group/django-developers/browse_thread/thread/b13d4c04df9d09e6

comment:16 Changed 3 years ago by carljm

  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to New feature

Based on the discussion on the above thread, I'm reopening and accepting this ticket.

The implementation should do nothing by default, but should allow the user (probably via setting) to configure a header+value pair that Django will interpret as "yes, this request is secure". The documentation for this feature should be very clear that the proxy server MUST unconditionally set or strip this header, otherwise you are introducing a security hole by using it. Ideally we might also give some examples of how to correctly configure common proxy servers (nginx, Apache/mod_proxy).

There is an existing implementation of this in django-secure; there are probably others as well.

I think I've seen one other implementation (don't recall where) that allowed the user to set multiple header+value pairs. I think this is a bad idea, as it encourages overly-broad use of this feature. The correct way to use it is to set it per-deployment to the specific header that you know is set+validated by the proxy server in that particular deployment, not to pre-emptively set it to a range of possible headers that some proxy servers might use.

comment:17 Changed 3 years ago by adrian

  • Owner changed from nobody to adrian
  • Status changed from reopened to new

I'm going to implement this. I'm hoping to check something into trunk today, so that it'll get in for Django 1.4.

comment:18 Changed 3 years ago by adrian

  • Resolution set to fixed
  • Status changed from new to closed

In [17209]:

Fixed #14597 -- Added a SECURE_PROXY_SSL_HEADER setting for cases when you're behind a proxy that 'swallows' the fact that a request is HTTPS

Note: See TracTickets for help on using tickets.
Back to Top