Code

#20411 closed Bug (fixed)

Invalid Referer header blows up on CSRF protection middleware

Reported by: edevil Owned by: saz
Component: HTTP handling Version: 1.5
Severity: Normal Keywords: referer valueerror csrf
Cc: bmispelon@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If a client sends an invalid Referer header such as 'http://http://xxx.pt/', the CSRF middleware will blow up with an error:

ERROR 2013-05-15 17:38:56,542 django.request:212 22023 140475533584128 Internal Server Error: /
Traceback (most recent call last):
  File "/servers/python-environments/discosite/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 109, in get_response
    response = middleware_method(request, callback, callback_args, callback_kwargs)
  File "/servers/python-environments/discosite/local/lib/python2.7/site-packages/django/middleware/csrf.py", line 148, in process_view
    if not same_origin(referer, good_referer):
  File "/servers/python-environments/discosite/local/lib/python2.7/site-packages/django/utils/http.py", line 229, in same_origin
    return (p1.scheme, p1.hostname, p1.port) == (p2.scheme, p2.hostname, p2.port)
  File "/usr/lib/python2.7/urlparse.py", line 110, in port
    port = int(port, 10)
ValueError: invalid literal for int() with base 10: ''

Either we catch the Exception or we are more careful when comparing.

Attachments (2)

test_20411.diff (1.0 KB) - added by bmispelon 11 months ago.
Minimal test case for #20411
20411_fix_exception_invalid_referer.diff (1.6 KB) - added by saz 11 months ago.
Fix for ValueError exception if referer is invalid

Download all attachments as: .zip

Change History (9)

comment:1 Changed 11 months ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Changed 11 months ago by bmispelon

Minimal test case for #20411

comment:2 Changed 11 months ago by bmispelon

  • Cc bmispelon@… added

Hi,

Thanks for your report.

I added a small test case to reproduce the issue.

The problem happens when trying to access the port part of a malformed urlparse result.

There's two places where this could be fixed in django:

1) In django.utils.http.same_origin
2) In django.middleware.csrf.CsrfViewMiddleware.process_view

I'm not sure which one is the best place to fix this. I'd be inclined to go
with 1) and add a try/except around the return statement, catch a ValueError
and return False in that case.

For the tests, it might be worth it to test a wider ranger of malformed hosts
(I'm not sure if it really applies, but you could check what the tests for ALLOWED_HOSTS test against [1]).

[1] https://github.com/django/django/blob/master/tests/requests/tests.py#L183-L190

comment:3 Changed 11 months ago by claudep

+1 for the 1) approach (and returning False in case of errors).

comment:4 Changed 11 months ago by saz

  • Owner changed from nobody to saz
  • Status changed from new to assigned

Changed 11 months ago by saz

Fix for ValueError exception if referer is invalid

comment:5 Changed 11 months ago by saz

  • Has patch set

comment:6 Changed 11 months ago by bmispelon

  • Triage Stage changed from Accepted to Ready for checkin

Patch looks good. Marking this as Ready For Checkin.

comment:7 Changed 11 months ago by Florian Apolloner <florian@…>

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

In 051cb1f4c60ac8e7087d92ef34ed41e6684d8b9b:

Fixed #20411 -- Don't let invalid referers blow up CSRF same origin checks.

Thanks to edevil for the report and saz for the patch.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.