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-123067: Denial of Service Vulnerability in `http.cookies._unquote()` by ch4n3-yoon · Pull Request #123066 · python/cpython

Conversation

@ch4n3-yoon

@ch4n3-yoon ch4n3-yoon commented Aug 16, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Summary

Refactor and improve the _unquote() method in http.cookies to address the performance issues identified in CVE-2024-7592, enhancing the handling of escape sequences to prevent potential DoS vulnerabilities.

Changes

  • Updated regex patterns to optimize matching and substitution.
  • Removed inefficient loop constructs, replacing them with a streamlined regex substitution process.

Context

This update comes after the Django team acknowledged the potential for a DoS vulnerability within their use of the http.cookies module. The vulnerability has been formally reserved CVE-2024-7592.

Please review these changes and provide your feedback.

  • Issue: CVE-2024-7592: Denial of Service Vulnerability in http.cookies._unquote() #123067

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ch4n3-yoon
Refactor function to improve efficiency and regex handling
26117bc
@ghost

ghost commented Aug 16, 2024 •
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.
@bedevere-app

bedevere-app Bot commented Aug 16, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added the awaiting review label Aug 16, 2024
@ch4n3-yoon ch4n3-yoon closed this Aug 16, 2024
@ch4n3-yoon

ch4n3-yoon commented Aug 16, 2024

Copy link
Copy Markdown
Contributor Author

Oh, I'm sorry for missing to create an issue.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ch4n3-yoon ch4n3-yoon changed the title CVE-2024-7592: Denial of Service Vulnerability in http.cookies._unquote() gh-123067: Denial of Service Vulnerability in http.cookies._unquote() Aug 16, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Aug 16, 2024
CVE-2024-7592: Denial of Service Vulnerability in http.cookies._unquote() #123067
Closed
@ch4n3-yoon ch4n3-yoon reopened this Aug 16, 2024
@bedevere-app

bedevere-app Bot commented Aug 16, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sethmlarson sethmlarson added the type-security A security issue label Aug 16, 2024
@picnixz picnixz added needs backport to 3.8 needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Aug 16, 2024
@sethmlarson

sethmlarson commented Aug 16, 2024

Copy link
Copy Markdown
Contributor

@ch4n3-yoon Can you create a news entry for this change with blurb?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sethmlarson
sethmlarson reviewed Aug 16, 2024
View reviewed changes
Comment thread Lib/http/cookies.py
j = o_match.start(0)
if q_match:
k = q_match.start(0)
if q_match and (not o_match or k < j): # QuotePatt matched

@sethmlarson sethmlarson Aug 16, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This appears to be removing all backslashes from the cookie, where the new behavior is only removing backslashes in front of quotes. Is that what we're expecting to change with this PR?

Sorry, something went wrong.

Uh oh!

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

@sethmlarson sethmlarson Aug 16, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

From reading the RFC, I think we should preserve the original behavior of unquoting all single-character preceded by a backslash. If you agree, can we add a test case that ensures this behavior is preserved.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
serhiy-storchaka
serhiy-storchaka reviewed Aug 16, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

_quate() can produce not only \", but also \\.

Both octal encoding and simple backslash-escaping should be handled at the same pass. Your code produces incorrect value for \134\042.

I created an alternative PR #123075 which handles such cases correctly and has some tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@picnixz

picnixz commented Aug 17, 2024

Copy link
Copy Markdown
Member

Closing since #123075 was considered instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@picnixz picnixz closed this Aug 17, 2024
@picnixz picnixz removed type-security A security issue needs backport to 3.8 needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@sethmlarson sethmlarson sethmlarson left review comments

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review

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.

4 participants

@ch4n3-yoon @sethmlarson @picnixz @serhiy-storchaka
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.