Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed #26308 -- Prevented crash with binary URLs in is_safe_url() #6242

Merged
merged 1 commit into from
Mar 4, 2016
Merged

Fixed #26308 -- Prevented crash with binary URLs in is_safe_url() #6242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions django/utils/http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,8 @@ def is_safe_url(url, host=None):
url = url.strip()
if not url:
return False
if six.PY2:
url = force_text(url, errors='replace')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering about errors='replace' here. The function below considers control characters. Is it possible that some of the bad characters we're trying to detect here will be turned into replacement characters, and then overlooked by _is_safe_url below? The Unicode replacement character is U+FFFD, which is category "So".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The control character issue is that <a href="\x08//example.com"> will redirect to example.com but is_safe_url() would consider it safe before 011a543.
In Firefox and Chrome, <a href="\ufffd//example.com"> will be treated as path relative and redirect to something like http://localhost:8000/page-where-link-appears/%EF%BF%BD//example.com, so I don't see a problem.

# Chrome treats \ completely as / in paths but it could be part of some
# basic auth credentials so we need to check both URLs.
return _is_safe_url(url, host) and _is_safe_url(url.replace('\\', '/'), host)
Expand Down
10 changes: 3 additions & 7 deletions docs/releases/1.8.11.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@
Django 1.8.11 release notes
===========================

*Under development*
*March 4, 2016*

Django 1.8.11 fixes several bugs in 1.8.10.

Bugfixes
========

* ...
Django 1.8.11 fixes a regression on Python 2 in the 1.8.10 security release
where ``utils.http.is_safe_url()`` crashes on bytestring URLs (:ticket:`26308`).
10 changes: 3 additions & 7 deletions docs/releases/1.9.4.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@
Django 1.9.4 release notes
==========================

*Under development*
*March 4, 2016*

Django 1.9.4 fixes several bugs in 1.9.3.

Bugfixes
========

* ...
Django 1.9.4 fixes a regression on Python 2 in the 1.9.3 security release
where ``utils.http.is_safe_url()`` crashes on bytestring URLs (:ticket:`26308`).
12 changes: 12 additions & 0 deletions tests/utils_tests/test_http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# -*- encoding: utf-8 -*-
from __future__ import unicode_literals

import sys
Expand Down Expand Up @@ -114,6 +115,17 @@ def test_is_safe_url(self):
'http://testserver/confirm?email=me@example.com',
'/url%20with%20spaces/'):
self.assertTrue(http.is_safe_url(good_url, host='testserver'), "%s should be allowed" % good_url)

if six.PY2:
# Check binary URLs, regression tests for #26308
self.assertTrue(
http.is_safe_url(b'https://testserver/', host='testserver'),
"binary URLs should be allowed on Python 2"
)
self.assertFalse(http.is_safe_url(b'\x08//example.com', host='testserver'))
self.assertTrue(http.is_safe_url('àview/'.encode('utf-8'), host='testserver'))
self.assertTrue(http.is_safe_url('àview'.encode('latin-1'), host='testserver'))

# Valid basic auth credentials are allowed.
self.assertTrue(http.is_safe_url(r'http://user:pass@testserver/', host='user:pass@testserver'))
# A path without host is allowed.
Expand Down