[Django] #23295: ALLOWED_HOSTS setting is done in the wrong place, should be in a custom middleware
Django
middleware
-------------------------------+--------------------
Reporter: mark0978 | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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:
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23295>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
middleware
Reporter: mark0978 | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.5
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Interesting. Could hooking into Django's logging solve your problem?
https://docs.djangoproject.com/en/1.6/topics/logging/#django-security
--
Ticket URL: <https://code.djangoproject.com/ticket/23295#comment:1>
Django
middleware
Reporter: mark0978 | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.5
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23295#comment:2>
Django
middleware
Reporter: mark0978 | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.5
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by collinanderson):
Well, it doesn't break any tests to remove the " and request.get_host()"
check.
https://github.com/django/django/pull/3072
--
Ticket URL: <https://code.djangoproject.com/ticket/23295#comment:3>
Django
middleware
Reporter: mark0978 | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.5
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
Replying to [comment:3 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23295#comment:4>
Django
middleware
Reporter: mark0978 | Owner: collinanderson
Type: Bug | Status: assigned
Component: Uncategorized | Version: 1.5
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* status: new => assigned
* owner: nobody => collinanderson
* has_patch: 0 => 1
Comment:
I've attached a (super simple) test. Buildbot pending
http://djangoci.com/job/django-pull-requests/735/
--
Ticket URL: <https://code.djangoproject.com/ticket/23295#comment:5>
Django
middleware
Reporter: mark0978 | Owner: collinanderson
Component: Uncategorized | Version: 1.5
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Collin Anderson <cmawebsite@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"230393e5e81ff524a3ab8c476f75011d3ac53115"]:
{{{
#!CommitTicketReference repository=""
revision="230393e5e81ff524a3ab8c476f75011d3ac53115"
Fixed #23295 -- Removed unnecessary fix_location_header request.get_host()
check.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23295#comment:6>
Django
middleware
Reporter: mark0978 | Owner: collinanderson
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carl Meyer <carl@…>):
In [changeset:"9fef66ef7c3dbb156d0b235261ec499f81494eae"]:
{{{
#!CommitTicketReference repository=""
revision="9fef66ef7c3dbb156d0b235261ec499f81494eae"
Merge pull request #3072 from collinanderson/23295
Fixed #23295 -- Removed unnecessary fix_location_header request.get_host()
check
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23295#comment:7>