GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-102153: fix CVE-2023-24329 by xiaoge1001 · Pull Request #102470 · python/cpython

Conversation

@xiaoge1001

@xiaoge1001 xiaoge1001 commented Mar 6, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

fix CVE-2023-24329

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
sxt1001 added 3 commits March 6, 2023 21:42
sxt1001
fix CVE-2023-24329
954be4d
sxt1001
add test for CVE-2023-24329
c546775
shixuantong
doc
dccba70
@bedevere-bot bedevere-bot mentioned this pull request Mar 6, 2023
urllib.parse space handling CVE-2023-24329 appears unfixed #102153
Closed
@bedevere-bot bedevere-bot added the awaiting review label Mar 6, 2023
@ghost

ghost commented Mar 6, 2023 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sxt1001 added 2 commits March 6, 2023 22:54
sxt1001
Merge branch 'main' into fix-cve
f15ec24
sxt1001
Merge branch 'main' into fix-cve
7e4b434
@xiaoge1001

xiaoge1001 commented Mar 7, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

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='')
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

CharlieZhao95
CharlieZhao95 suggested changes Mar 7, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Security/2023-03-06-22-48-08.gh-issue-102153.eiaVrE.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Mar 7, 2023
@CharlieZhao95

CharlieZhao95 commented Mar 7, 2023

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@xiaoge1001

xiaoge1001 commented Mar 7, 2023

Copy link
Copy Markdown
Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@xiaoge1001

xiaoge1001 commented Mar 7, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

#102153 (comment)

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@arhadthedev arhadthedev added type-security A security issue stdlib Standard Library Python modules in the Lib/ directory labels Mar 7, 2023
@illia-v

illia-v commented Mar 7, 2023

Copy link
Copy Markdown
Contributor

#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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shixuantong added 3 commits March 8, 2023 07:16
shixuantong
use strip() replace lstrip()
b32d74d
sxt1001
revert use strip() replace lstrip()
ebcd46c
sxt1001
Merge branch 'main' into fix-cve
43b1b8b
@xiaoge1001 xiaoge1001 mentioned this pull request Mar 8, 2023
gh-102153: Start stripping C0 control and space chars in urlsplit #102508
Merged
sxt1001
update doc
b00c0ea
Thanks CharlieZhao95
@xiaoge1001
xiaoge1001 requested a review from CharlieZhao95 March 8, 2023 03:52
@zhuofeng6

zhuofeng6 commented Mar 8, 2023

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gpshead gpshead self-assigned this Mar 8, 2023
@xiaoge1001

xiaoge1001 commented Mar 9, 2023

Copy link
Copy Markdown
Author

Hello, is there anyone concerned about this problem?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hugovk

hugovk commented Mar 9, 2023

Copy link
Copy Markdown
Member

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

See also #102153 (comment)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gpshead gpshead added the DO-NOT-MERGE label Mar 9, 2023
@gpshead

gpshead commented Mar 9, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gpshead gpshead closed this Mar 9, 2023
@xiaoge1001

xiaoge1001 commented Mar 9, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

@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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gpshead

gpshead commented Mar 9, 2023

Copy link
Copy Markdown
Member

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@illia-v

illia-v commented Mar 10, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@xiaoge1001

xiaoge1001 commented Mar 30, 2023

Copy link
Copy Markdown
Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CharlieZhao95 CharlieZhao95 Awaiting requested review from CharlieZhao95

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

awaiting core review DO-NOT-MERGE stdlib Standard Library Python modules in the Lib/ directory type-security A security issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@xiaoge1001 @CharlieZhao95 @illia-v @zhuofeng6 @hugovk @gpshead @arhadthedev @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.