Fix CookieError crash with control characters on CPython builds with CVE-2026-3644 patch#12395
Fix CookieError crash with control characters on CPython builds with CVE-2026-3644 patch#12395rodrigobnogueira wants to merge 15 commits intoaio-libs:masterfrom
Conversation
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #12395 +/- ##
=======================================
Coverage 98.92% 98.92%
=======================================
Files 134 134
Lines 46750 46802 +52
Branches 2429 2432 +3
=======================================
+ Hits 46248 46300 +52
Misses 373 373
Partials 129 129
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Merging this PR will not alter performance
Comparing Footnotes
|
|
Tested with Python 3.13.3 and control chars seem to be accepted. Version might need to be narrowed in the summary |
|
looks like something like |
I think you mean removed. I could see no reason that CTL characters were supposed to be allowed, the test was a generic test of octal unquoting. So my assumption is that the tests arbitrarily chose CTL characters and they were not a deliberate choice, nor did it suggest real cookies would have such characters. It seems fine to me for them to be rejected as a security concern. |
…back - _safe_set_morsel_state now returns bool; callers skip unsalvageable cookies - Handles both octal-decoded CTL chars and literal CTL chars in raw headers - Added tests for literal control character edge case (bdraco feedback) - Updated version wording to reference CVE-2026-3644 patch, not Python 3.13+ - Reworded test docstrings per Dreamsorcerer feedback
for more information, see https://pre-commit.ci
Hello @Dreamsorcerer , the tests now describe what they actually verify (graceful handling vs crashing) without implying CTL chars should be allowed. |
- Add 'unsalvageable' to docs spelling wordlist (fixes linter) - Add test_parse_cookie_header_literal_ctl_chars for Cookie header path - Remove artificial test_preserve_morsel_with_coded_value_literal_ctl_chars (a Morsel with control chars can't be constructed through normal APIs)
2b9987b to
5e12a69
Compare
d34ac62 to
78d8173
Compare
for more information, see https://pre-commit.ci
|
I added a few extra tests to fix the missing coverage reported by Codecov. |
|
Pin pyupgrade to python3.13 to work around a crash introduced in Python 3.14 alpha (tokenize.cookie_re was changed from a string to a bytes pattern in 3.14, causing pyupgrade to fail with TypeError: cannot use a bytes pattern on a string-like object when run under 3.14). This is a pyupgrade bug |
|
Wow, looks like pyupgrade still targets the Python 3.7 syntax. cc @Dreamsorcerer do we still need it that low? Would it make sense to bump the tool's version in a standalone PR? |
|
@rodrigobnogueira I haven't found any pyupgrade issue filed upstream? I recommend creating it there and linking from the commit or at least anywhere so it'd be clearer what problem you're referring to. If you can — it'd be a good idea to submit a fix upstream. |
webknjaz
left a comment
There was a problem hiding this comment.
Below are a few notes/suggestions, not a full review.
| @@ -0,0 +1,6 @@ | |||
| Fixed a crash (``CookieError``) in the cookie parser when receiving cookies | |||
| containing ASCII control characters on CPython builds with the CVE-2026-3644 | |||
There was a problem hiding this comment.
Sphinx comes with a built-in role for this
| containing ASCII control characters on CPython builds with the CVE-2026-3644 | |
| containing ASCII control characters on CPython builds with the :cve:`2026-3644` |
| @@ -0,0 +1,6 @@ | |||
| Fixed a crash (``CookieError``) in the cookie parser when receiving cookies | |||
There was a problem hiding this comment.
Would this work?
| Fixed a crash (``CookieError``) in the cookie parser when receiving cookies | |
| Fixed a crash (:external+python:exc:`~http.cookies. | |
| CookieError`) in the cookie parser when receiving cookies |
| (r'name="\012newline\012"', "name", r'"\012newline\012"'), | ||
| (r'tab="\011separated\011values"', "tab", r'"\011separated\011values"'), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Could you add IDs here as a separate arg or right in the params?
| result = parse_set_cookie_headers(['name="a\x07b"']) | ||
| # On CPython with CVE-2026-3644 patch the cookie is skipped; | ||
| # on older builds it may be accepted. Either way, no crash. | ||
| if result: |
There was a problem hiding this comment.
Let's turn this into a parametrized test with one of the params having a skip mark based on the know CPython range so the runtime is known explicitly.
There was a problem hiding this comment.
Done — turned this into a parametrized test with two cases (bel-in-value and bel-with-attribute).
| result = parse_set_cookie_headers(['bad="a\x07b"; good=value', "another=cookie"]) | ||
| # "good" is an attribute of "bad" (same header), so it's not a separate cookie. | ||
| # "another" is in a separate header and must always be preserved. | ||
| names = [name for name, _ in result] |
There was a problem hiding this comment.
Would this work with a generator expression?
| names = [name for name, _ in result] | |
| names = (name for name, _ in result) |
| names = [name for name, _ in result] | ||
| assert "good" in names |
There was a problem hiding this comment.
Structures like this tend to read better semantically, plus there's no throw-away list creation and the iteration stops early:
| names = [name for name, _ in result] | |
| assert "good" in names | |
| assert any(name == "good" for name, _ in result) |
| SimpleCookie, | ||
| _unquote as simplecookie_unquote, | ||
| ) | ||
| from unittest.mock import patch |
There was a problem hiding this comment.
Instead of calling the low-level stdlib interface, a natively integrated way would be using pytest-mock that provides a mocker fixture.
There was a problem hiding this comment.
I've never really understood the point of the mocker fixture...
| autospec=True, | ||
| side_effect=_mock_setstate, | ||
| ): | ||
| yield |
There was a problem hiding this comment.
If you migrate to the builtin monkeypatch fixture, this would be enough to move away from the generator to a regular function here. But if you want to use the mocking/spying interface, you could depend on mocker instead.
| yield | ||
|
|
||
|
|
||
| def test_cookie_helpers_cve_fallback(mock_strict_morsel: None) -> None: |
There was a problem hiding this comment.
When a fixture does not return a usable object, there's no need to inject it as a test function argument only to discard an unused local var. Instead, wire it through a mark as follows:
| def test_cookie_helpers_cve_fallback(mock_strict_morsel: None) -> None: | |
| @pytest.mark.usefixtures('mock_strict_morsel') | |
| def test_cookie_helpers_cve_fallback() -> None: |
I'm going to replace the whole setup with ruff in the near future, so haven't really looked at it. I did a manual upgrade with ruff a couple of months ago, though I only applied the safe fixes at that time, so still a few outdated bits in the code currently. |
Yeah, I saw that. That's just because those tests are outdated, right? We can backport the change to 3.13 branch to resolve that. |
|
| ) | ||
| except CookieError: | ||
| # The decoded value contains control characters rejected after | ||
| # CVE-2026-3644. Fall back to keeping the raw coded_value. |
There was a problem hiding this comment.
I'm still unclear to implications of allowing the coded_value here, rather than just rejecting the cookie entirely.
There was a problem hiding this comment.
The fallback to coded_value preserves the cookie with its original escaped representation (e.g. "\012newline\012") instead of dropping it entirely. The coded_value at this point contains only printable ASCII (backslash-escaped octals), so there are no injection or smuggling concerns. If the application later echoes this cookie back via Cookie:, the server receives exactly what it originally set.
If you'd prefer to reject the cookie entirely instead, I can simplify the function to a single try/except that returns False on any CookieError.
There was a problem hiding this comment.
I'm still leaning towards rejection. See what @bdraco thinks.
There was a problem hiding this comment.
Done! Code is simpler now. I like it.
There was a problem hiding this comment.
I agree. Reject seems to be the way to go. Someone will complain if we really need to allow this
861e965 to
d171cbd
Compare
- Use :cve:`2026-3644` and :external+python:exc: roles in changelog - Add pytest IDs to CTL chars from octal parametrize - Parametrize literal CTL char test (was two separate tests) - Use any() instead of list + in for semantic clarity - Replace unittest.mock.patch with monkeypatch fixture (no new dep) - Use @pytest.mark.usefixtures for void fixture injection - Remove pyupgrade language_version: python3.13 pin
d171cbd to
9317e23
Compare
| ) | ||
|
|
||
|
|
||
| def _safe_set_morsel_state( |
There was a problem hiding this comment.
Should we drop this function now? Feels like it should just be inline again now.
Drop _safe_set_morsel_state helper and inline the try/except at each call site, as suggested by Dreamsorcerer — the function was simplified to a trivial wrapper and no longer warrants its own definition.
|
The new tests pass but don't the existing octal tests still expect decoding while they should be skipped now if This is on Python 3.13.13 and applied this PR to aiohttp 3.13.5. |
What do these changes do?
CPython builds that include the CVE-2026-3644 patch add strict validation to
Morsel.__setstate__that rejects values containing ASCII control characters. This causes aiohttp's cookie parser to crash withCookieErrorin two scenarios:_unquotedecodes sequences like\012to literal\n, which__setstate__then rejects.\x07(BEL) is unsalvageable since both the decoded value and thecoded_valuecontain the control character.This PR adds a
_safe_set_morsel_statehelper that:__setstate__and catchesCookieErrorFalseso callers gracefully skip the unsalvageable cookie instead of crashingAre there changes in behavior for the user?
Yes. On CPython builds with the CVE-2026-3644 patch, when a server sends a cookie containing a control character:
\012): the cookie is silently skipped\x07): the cookie is silently skippedIn both cases the parser no longer crashes. On CPython builds without the patch, behavior is unchanged.
Is it a substantial burden for the maintainers to support this?
No. The helper is a thin wrapper around the existing
__setstate__calls, uses the same patterns already established in the codebase, and is well-tested across both patched and unpatched CPython builds.Related issue number
N/A — This addresses a crash introduced by CPython's CVE-2026-3644 security patch.
Checklist
CONTRIBUTORS.txtCHANGES/folder