urllib.parse space handling CVE-2023-24329 appears unfixed

Everyone (including the submitter of the now public exploit who submitted the issue half a year ago to security@python.org and the NVD) seems to think that #99421 "accidently fixed" CVE-2023-24329.

Did the Python Security Response Team verify that this vulnerability that was reported to them and that is now public was fixed by #99421?

The PoC from the submitter still works for me with the Debian package 3.11.2-4, which surprised me and makes me wonder whether the fix had any effect at all on the stripping of leading blanks issue in the CVE.

Linked PRs

@pablogsal

@pablogsal

The backport was merged here #99446 no?

@pablogsal

@pablogsal #99446 is a backport of #99421 that does not seem to fix CVE-2023-24329:

$ cat test.py 
import urllib.request
from urllib.parse import urlparse
def safeURLOpener(inputLink):
    block_host = ["instagram.com", "youtube.com", "tiktok.com", "example.com"]
    input_hostname = urlparse(inputLink).hostname
    if input_hostname in block_host:
        print("input hostname is forbidden")
        return
    target = urllib.request.urlopen(inputLink)
    content = target.read()
    print(content)

safeURLOpener("https://example.com")
safeURLOpener(" https://example.com")  # CVE-2023-24329
safeURLOpener("+https://example.com")  # 99421
$ python3.10 test.py 
input hostname is forbidden
b'<!doctype html>\n<html>\n<head>\n    <title>Example Domain</title>\n\n    <meta charset="utf-8" />\n    <meta http-equiv="Content-type" content="text/html; charset=utf-8" />\n    <meta name="viewport" content="width=device-width, initial-scale=1" />\n    <style type="text/css">\n    body {\n        background-color: #f0f0f2;\n        margin: 0;\n        padding: 0;\n        font-family: -apple-system, system-ui, BlinkMacSystemFont, "Segoe UI", "Open Sans", "Helvetica Neue", Helvetica, Arial, sans-serif;\n        \n    }\n    div {\n        width: 600px;\n        margin: 5em auto;\n        padding: 2em;\n        background-color: #fdfdff;\n        border-radius: 0.5em;\n        box-shadow: 2px 3px 7px 2px rgba(0,0,0,0.02);\n    }\n    a:link, a:visited {\n        color: #38488f;\n        text-decoration: none;\n    }\n    @media (max-width: 700px) {\n        div {\n            margin: 0 auto;\n            width: auto;\n        }\n    }\n    </style>    \n</head>\n\n<body>\n<div>\n    <h1>Example Domain</h1>\n    <p>This domain is for use in illustrative examples in documents. You may use this\n    domain in literature without prior coordination or asking for permission.</p>\n    <p><a href="https://www.iana.org/domains/example">More information...</a></p>\n</div>\n</body>\n</html>\n'
input hostname is forbidden
$ python3.11 test.py 
input hostname is forbidden
b'<!doctype html>\n<html>\n<head>\n    <title>Example Domain</title>\n\n    <meta charset="utf-8" />\n    <meta http-equiv="Content-type" content="text/html; charset=utf-8" />\n    <meta name="viewport" content="width=device-width, initial-scale=1" />\n    <style type="text/css">\n    body {\n        background-color: #f0f0f2;\n        margin: 0;\n        padding: 0;\n        font-family: -apple-system, system-ui, BlinkMacSystemFont, "Segoe UI", "Open Sans", "Helvetica Neue", Helvetica, Arial, sans-serif;\n        \n    }\n    div {\n        width: 600px;\n        margin: 5em auto;\n        padding: 2em;\n        background-color: #fdfdff;\n        border-radius: 0.5em;\n        box-shadow: 2px 3px 7px 2px rgba(0,0,0,0.02);\n    }\n    a:link, a:visited {\n        color: #38488f;\n        text-decoration: none;\n    }\n    @media (max-width: 700px) {\n        div {\n            margin: 0 auto;\n            width: auto;\n        }\n    }\n    </style>    \n</head>\n\n<body>\n<div>\n    <h1>Example Domain</h1>\n    <p>This domain is for use in illustrative examples in documents. You may use this\n    domain in literature without prior coordination or asking for permission.</p>\n    <p><a href="https://www.iana.org/domains/example">More information...</a></p>\n</div>\n</body>\n</html>\n'
Traceback (most recent call last):
  File "/tmp/test.py", line 15, in <module>
    safeURLOpener("+https://example.com")  # 99421
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/test.py", line 9, in safeURLOpener
    target = urllib.request.urlopen(inputLink)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/urllib/request.py", line 216, in urlopen
    return opener.open(url, data, timeout)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/urllib/request.py", line 519, in open
    response = self._open(req, data)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/urllib/request.py", line 541, in _open
    return self._call_chain(self.handle_open, 'unknown',
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/urllib/request.py", line 496, in _call_chain
    result = func(*args)
             ^^^^^^^^^^^
  File "/usr/lib/python3.11/urllib/request.py", line 1419, in unknown_open
    raise URLError('unknown url type: %s' % type)
urllib.error.URLError: <urlopen error unknown url type: +https>
$

@gpshead

CC: @gpshead

Backport CVE-2023-24329 to all in-service releases: urlparse does not correctly handle schemes that begin with ASCII digits, '+', '-', and '.' characters #102293

BTW, does this patch (CVE-2023-24329) require a backport to 3.10, 3.9 and older branches?

I noticed that the bug is currently only backported to the 3.11 branch, but it actually affects all versions prior to 3.11.

@CharlieZhao95

@CharlieZhao95 that's what I was asking for in #102293 - backport of the security vulnerability fix for CVE-2023-24329 to all in-service releases (3.7-3.10).

The request for the backports has been closed as a duplicate of this issue by @gpshead

Maybe it's worth taking a step back and looking at the problem in a wider context.

In the PoC, the vulnerability arises not because parse() returns the wrong answer, but because it interprets the url differently from urlopen(). If they were both wrong in the same way it would be harmless. Why is there more than one piece of code which parses URLs? The DRY principle should apply.

Closely related, note that urlparse() does not have a vulnerability at all - any vulnerability is in code which relies on it and does so in a way in which it creates a vulnerability. In the PoC, the vulnerability is in the code for safeURLOpener().

The way in which urlparse() is implemented is fragile and bug prone. As a general principle, parsing code should not look ahead for known delimiters, it should systematically work from the start, advancing over characters tested to be legitimate. So
urlparse('example.com@!$%^&*()_+-={}[]:;"\\|?query#frag')
should stop parsing at the '%' as that is not a legal character when not followed by two hex digits. It may return that "example.com@!$" is the path and there are extra characters after the URL (this style of parsing is often convenient when parsing items which may contain things to be parsed), or report failure due to an invalid URL. Instead, an early stage of processing skips ahead to the '?' and '#', so it claims there is a path, query, and fragment. While it could then validate these pieces and realise that the path is invalid, this can be forgotten and makes it unnecessarily difficult and dangerous to make the parser accept a valid URL followed by other characters (because it would need to reliably undo any parsing of anything past the valid part).

We will backport something that makes sense if we determine this is a security issue, that's why I duped the other issue here. Backporting the existing commit further does not make sense to me until the leading space issue, if present as reported here, is resolved. (I haven't taken the time to look. this is not an emergency)

>>> from urllib.parse import urlparse
>>> urlparse(" https://example.com")
ParseResult(scheme='', netloc='', path=' https://example.com', params='', query='', fragment='')

I tested it and the problem doesn't seem to be fixed. I execute urlparse(" https://example.com"), the output before and after merging #99421 is the same.

CVE-2023-24329 says that supplying a URL that starts with blank characters is bad.

If a URL-scheme is " https", it will jump out of the loop in the following code:

cpython/Lib/urllib/parse.py

Line 465 in 50b0415

if c not in scheme_chars:

After #99421 is merged, it will exit early:

cpython/Lib/urllib/parse.py

Line 463 in 2e279e8

if i > 0 and url[0].isascii() and url[0].isalpha():

The code in line 468 is not executed before and after the modification, the subsequent code execution will not change：

when input a URL that starts with blank characters，#99421 doesn't seem to have no effect.

Fix failing get_safe_url tests for latest Python 3.8 and 3.9 (#31766)

00399: CVE-2023-24329

Fix failing get_safe_url tests for latest Python 3.8 and 3.9 (#31766)

00399: CVE-2023-24329

Fix failing get_safe_url tests for latest Python 3.8 and 3.9 (#31766)

00399: CVE-2023-24329

Fix failing get_safe_url tests for latest Python 3.8 and 3.9 (#31766)

Navigation Menu

Repository navigation

urllib.parse space handling CVE-2023-24329 appears unfixed #102153

Linked PRs

78 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

urllib.parse space handling CVE-2023-24329 appears unfixed #102153

Description

Linked PRs

Activity

ned-deily commented on Feb 23, 2023

pablogsal commented on Feb 24, 2023

AdrianBunk commented on Feb 24, 2023

pablogsal commented on Feb 24, 2023

arhadthedev commented on Feb 27, 2023

CharlieZhao95 commented on Feb 28, 2023

RSAlderman commented on Feb 28, 2023

CharlesBryant-G commented on Feb 28, 2023

gpshead commented on Mar 1, 2023

xiaoge1001 commented on Mar 6, 2023

xiaoge1001 commented on Mar 6, 2023

78 remaining items

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions