Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#23295 closed Bug (fixed)

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

Reported by: Mark Jones Owned by: Collin Anderson
Component: Uncategorized Version: 1.5
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


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 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 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]
                ip = request.META.get('REMOTE_ADDR')
            return ip

            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']
                    # 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(
                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

    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 though because the logic is in the wrong place.

Change History (7)

comment:1 Changed 2 years ago by Collin Anderson

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Interesting. Could hooking into Django's logging solve your problem?

comment:2 Changed 2 years ago by 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.

comment:3 Changed 2 years ago by Collin Anderson

Well, it doesn't break any tests to remove the " and request.get_host()" check.

comment:4 in reply to:  3 Changed 2 years ago by Carl Meyer

Replying to collinanderson:

Well, it doesn't break any tests to remove the " and request.get_host()" check.

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.

comment:5 Changed 2 years ago by Collin Anderson

Has patch: set
Owner: changed from nobody to Collin Anderson
Status: newassigned

I've attached a (super simple) test. Buildbot pending

comment:6 Changed 2 years ago by Collin Anderson <cmawebsite@…>

Resolution: fixed
Status: assignedclosed

In 230393e5e81ff524a3ab8c476f75011d3ac53115:

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

comment:7 Changed 2 years ago by Carl Meyer <carl@…>

In 9fef66ef7c3dbb156d0b235261ec499f81494eae:

Merge pull request #3072 from collinanderson/23295

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

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