bpo-30458: Disallow control chars in http URLs. #12755

gpshead · Apr 10, 2019

disallow control characters such as whitespace in urls put into the raw http protocol within http.client.

https://bugs.python.org/issue36276

https://bugs.python.org/issue30458

tirkarthi · Apr 10, 2019

urlopen uses _splittype where this validation could be added. Moving it to _splittype also makes sure Request object instantiation is also handled. Simple case as test for validation.

diff --git a/Lib/test/test_urllib.py b/Lib/test/test_urllib.py
index 2ac73b58d8..99bd934765 100644
--- a/Lib/test/test_urllib.py
+++ b/Lib/test/test_urllib.py
@@ -329,6 +329,14 @@ class urlopen_HttpTests(unittest.TestCase, FakeHTTPMixin, FakeFTPMixin):
         finally:
             self.unfakehttp()

+    def test_url_newline(self):
+        self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello!")
+        try:
+            with self.assertRaises(ValueError):
+                resp = urlopen("http://127.0.0.1:1234/?q=HTTP/1.1\r\nHeader: Value")
+        finally:
+            self.unfakehttp()
+
     def test_read_0_9(self):
         # "0.9" response accepted (but not "simple responses" without
         # a status line)
@@ -1512,6 +1520,11 @@ class RequestTests(unittest.TestCase):
         request.method = 'HEAD'
         self.assertEqual(request.get_method(), 'HEAD')

+    def test_url_newline(self):
+        Request = urllib.request.Request
+        with self.assertRaises(ValueError):
+            resp = Request("http://127.0.0.1:1234/?q=HTTP/1.1\r\nHeader: Value")
+

 class URL2PathNameTests(unittest.TestCase):

diff --git a/Lib/urllib/parse.py b/Lib/urllib/parse.py
index 8b6c9b1060..20242eee2d 100644
--- a/Lib/urllib/parse.py
+++ b/Lib/urllib/parse.py
@@ -997,6 +997,11 @@ def _splittype(url):
     if _typeprog is None:
         _typeprog = re.compile('([^/:]+):(.*)', re.DOTALL)

+    unquoted_url = unquote(url)
+    if (unquoted_url.startswith('http') and
+        re.search("[\x00-\x19\x7f-\x9f]", unquoted_url)):
+        raise ValueError("URL can't contain control characters. %r" % (unquoted_url,))
+
     match = _typeprog.match(url)
     if match:
         scheme, data = match.groups()

tirkarthi · Apr 10, 2019

tirkarthi reviewed Apr 10, 2019

View changes

Lib/urllib/request.py

tirkarthi · Apr 10, 2019

tirkarthi reviewed Apr 10, 2019

View changes

Lib/urllib/parse.py

tirkarthi · Apr 10, 2019

The initial reverted fix in golang was reverted as noted in golang/go#27302 (comment) because it took into account unicode characters in URL also as problematic CTL/invalid characters : golang/go@1040626#diff-6c2d018290e298803c0c9419d8739885R1136 and resorted to only using ASCII CTL characters. I assume this could be the internal test failures at Google where unicode values were present in URL.

It's not a problem with Python because urllib already tries to decode using ASCII and therefore throws a ValueError which golang assumes valid but the Python function has a comment from 2008 which could go into it's own enhancement if needed since browsers like Firefox accept unencoded unicode values in URL these days.

# Most URL schemes require ASCII. If that changes, the conversion
# can be relaxed.

cpython/Lib/urllib/parse.py

Line 971 in 750d74f

url = url.encode("ASCII").decode()

tirkarthi · Apr 10, 2019

Sorry, this has been sending me circles from morning. Checking out the latest golang source this was further relaxed golang/go@f1d662f#diff-130fa30904f41d0d113c732265e91542R63 to use check only for space and 0x7f instead of anything between 0x7f <= r && r <= 0x9f

First fix (r < ' ' || 0x7f <= r && r <= 0x9f)

golang/go@829c5df#diff-130fa30904f41d0d113c732265e91542

Latest fix reverting the above (b < ' ' || b == 0x7f)

golang/go@f1d662f#diff-130fa30904f41d0d113c732265e91542R63

func stringContainsCTLByte(s string) bool {
	for i := 0; i < len(s); i++ {
		b := s[i]
		if b < ' ' || b == 0x7f {
			return true
		}
	}
	return false
}

Thanks, this seems to have been changed with 448a541 in this PR

tirkarthi · Apr 10, 2019

See also an older thread from Victor on fixing similar issues : https://mail.python.org/pipermail/python-dev/2017-July/148699.html

gpshead · Apr 10, 2019

See also an older thread from Victor on fixing similar issues : https://mail.python.org/pipermail/python-dev/2017-July/148699.html

Yep. I'm trying to make this fix as surgical as possible to only prevent the actual problem of allowing invalid paths with whitespace to pass through to the protocol level without changing public APIs of other functions such as parse.quote or (deprecated) parse.splittype and such.

tirkarthi · Apr 10, 2019

This seems to cause issue with test.test_xmlrpc.SimpleServerTestCase.test_partial_post where the below URL is passed to regex and matches due to \n throwing ValueError and the test hangs in the CI. I am not sure what the test does and the value passed to regex more seems like a Response than URL to me.

/RPC2 HTTP/1.0
Content-Length: 100

bye

tirkarthi · Apr 10, 2019

Actual test :

with contextlib.closing(http.client.HTTPConnection(ADDR, PORT)) as conn:
    conn.request('POST', '/RPC2 HTTP/1.0\r\nContent-Length: 100\r\n\r\nbye')

So /RPC2 HTTP/1.0\r\nContent-Length: 100\r\n\r\nbye is passed as URL that is validated now? This seemed to have been introduced as part of https://bugs.python.org/issue14001 which fixes DDoS vulnerability on it's own :)

def request(self, method, url, body=None, headers={}, *,
            encode_chunked=False):
    """Send a complete request to the server."""
    self._send_request(method, url, body, headers, encode_chunked)

gpshead · Apr 30, 2019

I have made the requested changes; please review again

bedevere-bot · Apr 30, 2019

Thanks for making the requested changes!

@orsenthil, @vstinner: please review the changes made to this pull request.

tirkarthi · Apr 30, 2019

tirkarthi approved these changes Apr 30, 2019

View changes

LGTM. Getting this to testing will help.

Thanks @gpshead for the PR :)

This PR is also adopted in urllib3 : urllib3/urllib3#1591

miss-islington · May 1, 2019

Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖

miss-islington · May 1, 2019

Sorry, @gpshead, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker c4e671eec20dfcb29b18596a89ef075f826c9f96 3.7

miss-islington · May 1, 2019

Sorry, @gpshead, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker c4e671eec20dfcb29b18596a89ef075f826c9f96 3.6

miss-islington · May 1, 2019

Sorry, @gpshead, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker c4e671eec20dfcb29b18596a89ef075f826c9f96 2.7

bedevere-bot · May 1, 2019

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian root 3.x has failed when building commit c4e671e.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/27/builds/2776) and take a look at the build logs.
Check if the failure is related to this commit (c4e671e) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/27/builds/2776

Click to see traceback logs

Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
k


Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
k


Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f99fda7d3f8>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f99fda7d3f8>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f99fda7d3f8>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f99fda7d3f8>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f99fda7d3f8>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f99fda7d3f8>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f99fda7d2d8>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f99fdaf2710>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f99fd70f098>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
k


Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/test_urllib.py", line 350, in test_url_with_control_char_rejected
    urllib.request.urlopen(f"https:{schemeless_url}")
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 524, in open
    response = self._open(req, data)
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 546, in _open
    return self._call_chain(self.handle_open, 'unknown',
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 502, in _call_chain
    result = func(*args)
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 1386, in unknown_open
    raise URLError('unknown url type: %s' % type)
urllib.error.URLError: <urlopen error unknown url type: https>

======================================================================
ERROR: test_url_with_newline_header_injection_rejected (test.test_urllib.urlopen_HttpTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/test_urllib.py", line 372, in test_url_with_newline_header_injection_rejected
    urllib.request.urlopen(f"https:{schemeless_url}")
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 524, in open
    response = self._open(req, data)
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 546, in _open
    return self._call_chain(self.handle_open, 'unknown',
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 502, in _call_chain
    result = func(*args)
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 1386, in unknown_open
    raise URLError('unknown url type: %s' % type)
urllib.error.URLError: <urlopen error unknown url type: https>

----------------------------------------------------------------------

Ran 95 tests in 0.092s

FAILED (errors=2, skipped=2)


Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.


Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.


Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f718d031680>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f718d031638>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f718d031680>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f718d031638>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f718d031680>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f718d031638>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f718d04d488>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f718d04d4d0>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.
Exception ignored in: <http.client.HTTPResponse object at 0x7f718cc5e098>
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 412, in close
    super().close() # set "closed" flag
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/http/client.py", line 425, in flush
    self.fp.flush()
ValueError: I/O operation on closed file.


Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/test_urllib.py", line 350, in test_url_with_control_char_rejected
    urllib.request.urlopen(f"https:{schemeless_url}")
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 524, in open
    response = self._open(req, data)
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 546, in _open
    return self._call_chain(self.handle_open, 'unknown',
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 502, in _call_chain
    result = func(*args)
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 1386, in unknown_open
    raise URLError('unknown url type: %s' % type)
urllib.error.URLError: <urlopen error unknown url type: https>

======================================================================
ERROR: test_url_with_newline_header_injection_rejected (test.test_urllib.urlopen_HttpTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/test_urllib.py", line 372, in test_url_with_newline_header_injection_rejected
    urllib.request.urlopen(f"https:{schemeless_url}")
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 524, in open
    response = self._open(req, data)
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 546, in _open
    return self._call_chain(self.handle_open, 'unknown',
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 502, in _call_chain
    result = func(*args)
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/urllib/request.py", line 1386, in unknown_open
    raise URLError('unknown url type: %s' % type)
urllib.error.URLError: <urlopen error unknown url type: https>

----------------------------------------------------------------------

Ran 95 tests in 0.073s

FAILED (errors=2, skipped=2)

bedevere-bot · May 1, 2019

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 FreeBSD 10-STABLE Non-Debug 3.x has failed when building commit c4e671e.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/167/builds/863) and take a look at the build logs.
Check if the failure is related to this commit (c4e671e) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/167/builds/863

Click to see traceback logs

Traceback (most recent call last):
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/test/test_urllib.py", line 350, in test_url_with_control_char_rejected
    urllib.request.urlopen(f"https:{schemeless_url}")
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 524, in open
    response = self._open(req, data)
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 546, in _open
    return self._call_chain(self.handle_open, 'unknown',
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 502, in _call_chain
    result = func(*args)
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 1386, in unknown_open
    raise URLError('unknown url type: %s' % type)
urllib.error.URLError: <urlopen error unknown url type: https>

======================================================================
ERROR: test_url_with_newline_header_injection_rejected (test.test_urllib.urlopen_HttpTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/test/test_urllib.py", line 372, in test_url_with_newline_header_injection_rejected
    urllib.request.urlopen(f"https:{schemeless_url}")
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 524, in open
    response = self._open(req, data)
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 546, in _open
    return self._call_chain(self.handle_open, 'unknown',
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 502, in _call_chain
    result = func(*args)
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 1386, in unknown_open
    raise URLError('unknown url type: %s' % type)
urllib.error.URLError: <urlopen error unknown url type: https>

----------------------------------------------------------------------

Ran 95 tests in 0.406s

FAILED (errors=2, skipped=2)


Traceback (most recent call last):
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/test/test_urllib.py", line 350, in test_url_with_control_char_rejected
    urllib.request.urlopen(f"https:{schemeless_url}")
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 524, in open
    response = self._open(req, data)
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 546, in _open
    return self._call_chain(self.handle_open, 'unknown',
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 502, in _call_chain
    result = func(*args)
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 1386, in unknown_open
    raise URLError('unknown url type: %s' % type)
urllib.error.URLError: <urlopen error unknown url type: https>

======================================================================
ERROR: test_url_with_newline_header_injection_rejected (test.test_urllib.urlopen_HttpTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/test/test_urllib.py", line 372, in test_url_with_newline_header_injection_rejected
    urllib.request.urlopen(f"https:{schemeless_url}")
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 524, in open
    response = self._open(req, data)
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 546, in _open
    return self._call_chain(self.handle_open, 'unknown',
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 502, in _call_chain
    result = func(*args)
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10.nondebug/build/Lib/urllib/request.py", line 1386, in unknown_open
    raise URLError('unknown url type: %s' % type)
urllib.error.URLError: <urlopen error unknown url type: https>

----------------------------------------------------------------------

Ran 95 tests in 0.142s

FAILED (errors=2, skipped=2)

mcepl · May 1, 2019

Thanks @gpshead for the PR tacotada.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
snakecherriespickrobot

I have made a backport for openSUSE and yes it wasn't trivial.

mcepl · May 1, 2019

LGTM. Getting this to testing will help.

Thanks @gpshead for the PR :)

This PR is also adopted in urllib3 : urllib3/urllib3#1591

This patch actually breaks urllib3, I had to patch the test suite not to fail.

gpshead · May 1, 2019

This patch actually breaks urllib3, I had to patch the test suite not to fail.

thanks, based on that i changed the stdlib one to raise http.client.InvalidURL instead of ValueError. #13044

mcepl · May 3, 2019

Suggested patch for 2.7.16

I would love to get some feedback on this.

bpo-14826 bpo-36276: Disallow control chars in http URLs.
Example possible fix for those issues.

Loading status checks…

97bcc4b

gpshead added type-bugfix needs backport to 3.6 needs backport to 2.7 DO-NOT-MERGE needs backport to 3.7 labels Apr 10, 2019

the-knights-who-say-ni added the CLA signed label Apr 10, 2019

bedevere-bot added the awaiting core review label Apr 10, 2019

Move validation to _splittype & correct boundary.

Loading status checks…

bba966c

gpshead added some commits Apr 10, 2019

Move the check to http.client where it belongs.

Loading status checks…

bde613c

don't bother checking for chars above ascii.

Loading status checks…

448a541

missing f on f-string

Loading status checks…

944c5fa

confirm that repr of the url is in the ValueError.

Loading status checks…

f039a80

also assert no ' ' in the quoted url

Loading status checks…

6686d66

gpshead requested a review from vstinner Apr 10, 2019

📜🤖 Added by blurb_it.

Loading status checks…

a754bd7

gpshead removed the DO-NOT-MERGE label Apr 10, 2019

gpshead changed the title ~~bpo-14826 bpo-36276: Disallow control chars in http URLs.~~ bpo-36276: Disallow control chars in http URLs. Apr 10, 2019

gpshead requested a review from orsenthil Apr 10, 2019

gpshead changed the title ~~bpo-36276: Disallow control chars in http URLs.~~ bpo-30458: Disallow control chars in http URLs. Apr 10, 2019

gpshead added the type-security label Apr 10, 2019

ryanpetrello referenced this pull request Apr 30, 2019
Merged
prevent CVE-2019-9740 in 1.24.x #1591

gpshead self-assigned this Apr 30, 2019

Address code review comments.
Adds some comments and expands an error message per vstinner's PR review. (authored and pushed from 32,000ft on the plane to CLE for PyCon US)

Loading status checks…

54f7fca

bedevere-bot added awaiting change review and removed awaiting merge labels Apr 30, 2019

fix indentation

Loading status checks…

73db8e1

Expand the tests to cover every char.
all rejected chars are explicitly tested now.

Loading status checks…

aeb2be8

bedevere-bot removed the awaiting change review label May 1, 2019

gpshead deleted the gpshead:url-no-control-chars branch May 1, 2019

python/cpython

Join GitHub today

bpo-30458: Disallow control chars in http URLs. #12755

Conversation

gpshead commented Apr 10, 2019 • edited

gpshead added type-bugfix needs backport to 3.6 needs backport to 2.7 DO-NOT-MERGE needs backport to 3.7 labels Apr 10, 2019

the-knights-who-say-ni added the CLA signed label Apr 10, 2019

bedevere-bot added the awaiting core review label Apr 10, 2019

This comment has been minimized.

tirkarthi commented Apr 10, 2019

tirkarthi reviewed Apr 10, 2019 View changes

tirkarthi reviewed Apr 10, 2019 View changes

This comment has been minimized.

tirkarthi commented Apr 10, 2019

gpshead added some commits Apr 10, 2019

This comment has been minimized.

tirkarthi commented Apr 10, 2019

This comment has been minimized.

tirkarthi commented Apr 10, 2019

This comment has been minimized.

gpshead commented Apr 10, 2019

gpshead requested a review from vstinner Apr 10, 2019

gpshead removed the DO-NOT-MERGE label Apr 10, 2019

gpshead changed the title bpo-14826 bpo-36276: Disallow control chars in http URLs. bpo-36276: Disallow control chars in http URLs. Apr 10, 2019

gpshead requested a review from orsenthil Apr 10, 2019

gpshead changed the title bpo-36276: Disallow control chars in http URLs. bpo-30458: Disallow control chars in http URLs. Apr 10, 2019

gpshead added the type-security label Apr 10, 2019

This comment has been minimized.

tirkarthi commented Apr 10, 2019

This comment has been minimized.

tirkarthi commented Apr 10, 2019

ryanpetrello added a commit to ryanpetrello/urllib3 that referenced this pull request Apr 30, 2019

ryanpetrello added a commit to ryanpetrello/urllib3 that referenced this pull request Apr 30, 2019

ryanpetrello referenced this pull request Apr 30, 2019

prevent CVE-2019-9740 in 1.24.x #1591

gpshead self-assigned this Apr 30, 2019

ryanpetrello added a commit to ryanpetrello/urllib3 that referenced this pull request Apr 30, 2019

This comment has been minimized.

gpshead commented Apr 30, 2019

bedevere-bot added awaiting change review and removed awaiting merge labels Apr 30, 2019

This comment has been minimized.

bedevere-bot commented Apr 30, 2019

tirkarthi approved these changes Apr 30, 2019 View changes

tirkarthi left a comment

ryanpetrello added a commit to ryanpetrello/urllib3 that referenced this pull request Apr 30, 2019

Hide details View details gpshead merged commit c4e671e into python:master May 1, 2019 5 checks passed

5 checks passed

This comment has been minimized.

miss-islington commented May 1, 2019

bedevere-bot removed the awaiting change review label May 1, 2019

This comment has been minimized.

miss-islington commented May 1, 2019

This comment has been minimized.

miss-islington commented May 1, 2019

This comment has been minimized.

miss-islington commented May 1, 2019

gpshead deleted the gpshead:url-no-control-chars branch May 1, 2019

This comment has been minimized.

bedevere-bot commented May 1, 2019

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

This comment has been minimized.

bedevere-bot commented May 1, 2019

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

This comment has been minimized.

mcepl commented May 1, 2019

This comment has been minimized.

mcepl commented May 1, 2019 • edited by bedevere-bot

ryanpetrello added a commit to ryanpetrello/urllib3 that referenced this pull request May 1, 2019

This comment has been minimized.

gpshead commented May 1, 2019

arnolddumas added a commit to arnolddumas/cpython that referenced this pull request May 3, 2019

This comment has been minimized.

mcepl commented May 3, 2019

gpshead commented Apr 10, 2019 •

edited

tirkarthi reviewed Apr 10, 2019

View changes

tirkarthi reviewed Apr 10, 2019

View changes

tirkarthi approved these changes Apr 30, 2019

View changes

gpshead merged commit `c4e671e` into python:master May 1, 2019
5 checks passed

mcepl commented May 1, 2019 •

edited by bedevere-bot