gh-102153: fix CVE-2023-24329

[xiaoge1001]

fix CVE-2023-24329

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

[cpython-cla-bot]

All commit authors signed the Contributor License Agreement.

[xiaoge1001]

Before applying #102470:

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

After applying #102470:

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

[CharlieZhao95]

BTW, this solution is mentioned at the end of the issue finder's article, see: https://pointernull.com/security/python-url-parse-problem.html for more details.

[xiaoge1001]

BTW, this solution is mentioned at the end of the issue finder's article, see: https://pointernull.com/security/python-url-parse-problem.html for more details.

Yes, I read this article, too. About the leading blank problem, I can solve it using lstrip() function even without reading this article.

[xiaoge1001]

#102153 (comment)

As described by @illia-v, I think it is necessary to remove the leading blanks from the URL in urllib.parse.

[illia-v]

#102153 (comment)

As described by @illia-v, I think it is necessary to remove the leading blanks from the URL in urllib.parse.

Right, but a few characters not covered by simple .lstrip() have to be stripped too, I created an alternative – #102508.

[zhuofeng6]

i think that the PR of @xiaoge1001 is a good idea, could you review the PR, my brother @gpshead

[xiaoge1001]

Hello, is there anyone concerned about this problem?

[hugovk]

Thank you for the PRs, someone will review them and let's be patient in the meantime.

See also #102153 (comment)

[gpshead]

#102508 is more along the lines of what we'll want to consider. I'm closing this one in favor of that.

[xiaoge1001]

@gpshead I'm sorry to bother you, but I don't know urllib very well. Can you answer my question?

If I use this solution （https://github.com/python/cpython/pull/102470） to fix CVE when I don't think about compatibility, can the CVE be completely fixed? Will other problems be introduced?

[gpshead]

We aren't currently confident that we can safely backport a fix to older Python versions as something likely depends on some of the behavior around spaces. That said, this solution likely does work for a lot of code.

It is a reasonable recommendation (as I think the blog post suggested?) for people to call .lstrip() themselves on their url before passing it to urlparse from their own code rather than assuming they're running on top of a Python without the issue.

The other PR I closed this in favor of will have potentially even more side effects that could impact other uses of urlparse as it removes a wider/different set of characters than lstrip() considers from anywhere in the entire url string.

If you do go with this or any of the PRs in your own patched Python interpreter, please let us know how that went. Details like the relative size of your overall Python code base using your patched interpreter was and how many, if any, issues around the behavior change in the API came up after some period of time testing and deploying things with it. Those are all good data to have to inform decisions. (I'll be trying some testing of that nature myself)

[illia-v]

The other PR I closed this in favor of will have potentially even more side effects that could impact other uses of urlparse as it removes a wider/different set of characters than lstrip() considers from anywhere in the entire url string.

@gpshead yes, it may have more side effects. But it removes the characters only at the beginning and the end of a URL string (and a scheme if one is passed separately.)

[xiaoge1001]

We aren't currently confident that we can safely backport a fix to older Python versions as something likely depends on some of the behavior around spaces. That said, this solution likely does work for a lot of code.

It is a reasonable recommendation (as I think the blog post suggested?) for people to call .lstrip() themselves on their url before passing it to urlparse from their own code rather than assuming they're running on top of a Python without the issue.

The other PR I closed this in favor of will have potentially even more side effects that could impact other uses of urlparse as it removes a wider/different set of characters than lstrip() considers from anywhere in the entire url string.

If you do go with this or any of the PRs in your own patched Python interpreter, please let us know how that went. Details like the relative size of your overall Python code base using your patched interpreter was and how many, if any, issues around the behavior change in the API came up after some period of time testing and deploying things with it. Those are all good data to have to inform decisions. (I'll be trying some testing of that nature myself)

I plan to add constraints to fix CVE: "The first character of the URL-scheme string must be one ASCII alpha." Then I initiated a check on users for this constraint for two weeks, and did not receive any feedback on the impact. I think the impact of fixing the CVE problem should be small.