Opened 7 years ago

Closed 7 years ago

#28693 closed Bug (fixed)

DisallowedHost can cause uncaught exception and HTTP 500

Reported by: Greg Price Owned by: Tim Graham <timograham@…>
Component: HTTP handling Version: 1.11
Severity: Normal Keywords: DisallowedHost
Cc: Tomer Chachamu Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If the client sends an HTTP "Host:" header which isn't allowed, we try to return a 400 response while logging a DisallowedHost exception. For a brief time after SuspiciousOperation was first introduced, this caused a 500, but that was fixed in #19866 back in 2013.

But now I've been getting exceptions in production recently that are caused by DisallowedHost. It turns out that there is a case in the exception-handling path on the way to returning a 400 response that itself tries to call request.get_host(), and so raises a new DisallowedHost exception. Here's the test I added to my application's test suite, to test my workaround:

class ErrorPageTest(TestCase):
    def test_bogus_http_host(self):
        # This tests that we've successfully worked around a certain bug in
        # Django's exception handling.  The enforce_csrf_checks=True,
        # secure=True, and HTTP_REFERER with an `https:` scheme are all
        # there to get us down just the right path for Django to blow up
        # when presented with an HTTP_HOST that's not a valid DNS name.
        client = django.test.Client(enforce_csrf_checks=True)
        result = client.post('/json/users',
                             secure=True,
                             HTTP_REFERER='https://somewhere',
                             HTTP_HOST='$nonsense')
        self.assertEqual(result.status_code, 400)

The workaround is to override handler400 with a simple implementation that just skips the @requires_csrf_token from the default implementation. Like this:

def handler400(request, exception):
    return HttpResponseBadRequest(
        '<h1>Bad Request (400)</h1>', content_type='text/html')

(That version also skips the template lookup, since we haven't provided a 400 template.)

To fix it in Django, it'd be enough to just remove the @requires_csrf_token decorator from django.views.defaults.bad_request. I'm not sure what that might break, though -- I think the case where it could matter is if an app's 400.html tries to use csrf_token, but I'm not sure what the failure would look like. I think it could be pretty reasonable to say that a 400 error page can't use csrf_token, so long as the error that results if someone tries to do that is an appropriate one.

An alternate fix would be to try to make requires_csrf_token more robust. If it's going to be used on a 4xx error page (or at least on 400), it needs to really truly never raise an exception, no matter how nonsensical the request the client sent is, because that's where we go when we know the client has gotten something wrong.

See https://github.com/zulip/zulip/pull/6934 for an example traceback, and my actual workaround and test code.

Change History (3)

comment:1 by Tomer Chachamu, 7 years ago

Owner: changed from nobody to Tomer Chachamu
Status: newassigned
Triage Stage: UnreviewedAccepted

There would need to be a deprecation path for custom 400 views that are using csrf tokens, so I think catching DisallowedHost in the traceback is the right answer here.

comment:2 by Tomer Chachamu, 7 years ago

Cc: Tomer Chachamu added
Component: UncategorizedHTTP handling
Has patch: set
Owner: Tomer Chachamu removed
Status: assignednew
Last edited 7 years ago by Tim Graham (previous) (diff)

comment:3 by Tim Graham <timograham@…>, 7 years ago

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In 7ec0fdf6:

Fixed #28693 -- Fixed crash in CsrfViewMiddleware when an HTTPS request has an invalid host.

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