ALLOWED_HOSTS setting is done in the wrong place, should be in a custom middleware

[Mark Jones]

Right now ALLOWED_HOST checking is done in Request.get_host() which is called by CommonMiddleware. And then called again in fix_location_header(request, response) which means it is impossible to redirect an attacker to fbi.gov and at the same time record their attack because on a redirect get_host is called twice!

Raising a suspicious operation just does not belong in Request.get_host(), it really isn't part of the function that gives you the host back, it is something you do after you get the host information.

This is what I've had to write to work around this problem:

class CommonMiddleware(DjangoCommonMiddleware):
    """ In an effort to stop the deluge of ALLOWED_HOSTS emails sent at our software
    by very stupid pentesters, I have decided to record what they are doing, and then
    send them to the fbi.gov site (out of spite).  It shall be interesting to see
    if they actually figure it out or not. (But right now I can't do that because of
    limitations within Django"""

    def process_request(self, request):

        def get_client_ip(request):
            x_forwarded_for = request.META.get('HTTP_X_FORWARDED_FOR')
            if x_forwarded_for:
                ip = x_forwarded_for.split(',')[0]
            else:
                ip = request.META.get('REMOTE_ADDR')
            return ip

        try:
            return super(CommonMiddleware, self).process_request(request)
        except SuspiciousOperation as xcpt:
            if 'ALLOWED_HOSTS' in str(xcpt):
                # We try three options, in order of decreasing preference.
                if settings.USE_X_FORWARDED_HOST and (
                    'HTTP_X_FORWARDED_HOST' in request.META):
                    host = request.META['HTTP_X_FORWARDED_HOST']
                elif 'HTTP_HOST' in request.META:
                    host = request.META['HTTP_HOST']
                else:
                    # Reconstruct the host using the algorithm from PEP 333.
                    host = request.META['SERVER_NAME']
                    server_port = str(request.META['SERVER_PORT'])
                    if server_port != ('443' if request.is_secure() else '80'):
                        host = '%s:%s' % (host, server_port)

                obj = AllowedHostViolation.objects.create(
                    host_attacked=host,
                    url_attacked=request.get_full_path(),
                    attacker=get_client_ip(request)
                )
                response = HttpResponse("You were hoping to have breached security!"
                                        " Not today though!"
                                        " Now smile for the camera, because you've been busted!\n",
                                        content_type='application/text', status=418)
                response.allowed_host_violation = True
                return response
            raise

    def process_response(self, request, response):
        """ Have to also do this to keep it from throwing another Suspicious Operation """
        if response.allowed_host_violation:
            return response

        return super(CommonMiddleware, self).process_response(request, response)

This bug is also present in 1.6 but in that case you get a differentiated Exception thrown. You still can't return a HttpResponseRedirect to send them all to fbi.gov though because the logic is in the wrong place.

[Collin Anderson]

Interesting. Could hooking into Django's logging solve your problem? ​https://docs.djangoproject.com/en/1.6/topics/logging/#django-security

[Carl Meyer]

The reason for performing the check in get_host is that there was no other way to catch all the instances of people using the request host in their own code. Leaving get_host alone and doing the check in CommonMiddleware would have fixed only a small portion of vulnerabilities related to spoofed hosts, leaving all others wide open. Returning a possibly-spoofed value from get_host would be a security vulnerability.

The behavior you want is a bit specialized, so I don't think it's problematic that it requires a bit of custom code to implement. I don't entirely understand the code you posted. For instance, why do you need the process_response check? AFAICT CommonMiddleware.process_response does not call get_host (unless you have SEND_BROKEN_LINK_EMAILS=True (now deprecated) and the response is a 404).

You have a valid point about the fix_location_header response-fix function that is run unconditionally, and always checks request.get_host() for redirect responses. I'm not sure why fix_location_header needs to check get_host in advance, rather than just calling build_absolute_uri. If it did the latter, then if you already had a full URL in your Location header, build_absolute_uri would bail early and never call get_host at all, which would take care of your case.

[Collin Anderson]

Well, it doesn't break any tests to remove the " and request.get_host()" check.
​https://github.com/django/django/pull/3072

[Carl Meyer]

Replying to collinanderson:

Well, it doesn't break any tests to remove the " and request.get_host()" check.
​https://github.com/django/django/pull/3072

Nice, seems fine to me. I think if we do that we should also add a test verifying that get_host is never called by fix_location_header if the location header is already a full URL; otherwise the motivating use case here could regress without it being caught.

[Collin Anderson]

I've attached a (super simple) test. Buildbot pending ​http://djangoci.com/job/django-pull-requests/735/

[Collin Anderson <cmawebsite@…>]

In 230393e5e81ff524a3ab8c476f75011d3ac53115:

Fixed #23295 -- Removed unnecessary fix_location_header request.get_host() check.

[Carl Meyer <carl@…>]

In 9fef66ef7c3dbb156d0b235261ec499f81494eae:

Merge pull request #3072 from collinanderson/23295

Fixed #23295 -- Removed unnecessary fix_location_header request.get_host() check