Opened 13 months ago

Closed 13 months ago

Last modified 13 months ago

#23295 closed Bug (fixed)

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

Reported by: mark0978 Owned by: collinanderson
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

Description

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.

Change History (7)

comment:1 Changed 13 months ago by collinanderson

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

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

comment:2 Changed 13 months ago by carljm

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 follow-up: Changed 13 months ago by collinanderson

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

comment:4 in reply to: ↑ 3 Changed 13 months ago by carljm

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.

comment:5 Changed 13 months ago by collinanderson

  • Has patch set
  • Owner changed from nobody to collinanderson
  • Status changed from new to assigned

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

comment:6 Changed 13 months ago by Collin Anderson <cmawebsite@…>

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

In 230393e5e81ff524a3ab8c476f75011d3ac53115:

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

comment:7 Changed 13 months 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