GeezerVison

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

GeezerVison is showing github.com — gh-121284: Fix email address header folding with parsed encoded-word by medmunds · Pull Request #122754 · python/cpython

Conversation

medmunds commented Aug 6, 2024 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

Fixes #121284.

[This fixes a #security-issue. PSRT instructed me to handle the fix publicly.]

Email generators using email.policy.default may convert an RFC 2047 encoded-word to unencoded form during header refolding. In a structured header, this could allow 'specials' chars outside a quoted-string, leading to invalid address headers and enabling spoofing. This change ensures a parsed encoded-word that contains specials is kept as an encoded-word while the header is refolded.

The issue is very similar to PR #122753 (and has the same security implications), but this PR involves refolding an encoded-word; the other PR involves refolding a quoted-string. The fixes required are different.

Issue: email: invalid RFC 2047 address header after refolding with email.policy.default #121284

Sorry, something went wrong.

Uh oh!

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

All reactions


          pythongh-121284: Fix email address header folding with parsed encoded…

c540c9f

…-word

Email generators using email.policy.default may convert an RFC 2047
encoded-word to unencoded form during header refolding. In a structured
header, this could allow 'specials' chars outside a quoted-string,
leading to invalid address headers and enabling spoofing. This change
ensures a parsed encoded-word that contains specials is kept as an
encoded-word while the header is refolded.

medmunds requested a review from a team as a code owner

August 6, 2024 19:50

bedevere-app Bot mentioned this pull request

Aug 6, 2024

email: invalid RFC 2047 address header after refolding with email.policy.default #121284

Closed

bedevere-app Bot added the awaiting review label

Aug 6, 2024

sethmlarson added type-security

A security issue

topic-email labels

Aug 7, 2024

medmunds commented Sep 9, 2024

Copy link

Copy Markdown

Contributor Author

Requesting a review from @serhiy-storchaka, who recently reviewed a similar security fix around encoded words in email headers (#122233).

And pinging @bitdancer, who probably knows the most about this section of the code.

(Also hoping both of you might be able to review PR #122753, which tries to fix a related security issue first reported 5 years ago.)

All reactions

Sorry, something went wrong.

Uh oh!

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

picnixz requested a review from serhiy-storchaka

September 9, 2024 19:01


          Merge branch 'main' into fix-issue-121284

9c05613

medmunds mentioned this pull request

Oct 29, 2024

gh-80222: Fix email address header folding with long quoted-string #122753

Merged

medmunds added 2 commits

January 18, 2025 14:06


          Merge branch 'main' into fix-issue-121284

a8d23d2


          Be more specific in news

45159cd

medmunds commented Jan 18, 2025

Copy link

Copy Markdown

Contributor Author

(A nicer fix would be to decide separately whether each refolded segment needs rfc2047 encoding, quoted-string handling, or no special treatment. But that would require giving _refold_parse_tree() info about whether it's working in a structured or unstructured header, which seems too involved for a security patch.)

All reactions

Sorry, something went wrong.

Uh oh!

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

medmunds commented Jan 19, 2025 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor Author

Btw, although this may seem like it's too obscure to matter much, it's actually pretty easy to stumble into vulnerable code. E.g., calling email.utils.formataddr() (correctly!) with user-supplied content can generate exactly the sort of (valid) encoded-word that gets mishandled by email.policy.default.

All reactions

Sorry, something went wrong.

Uh oh!

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

encukou approved these changes

Feb 27, 2025

View reviewed changes

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

By my reading of the code, this is correct.
I'd appreciate an additional review from an email expert, but, if there are no objections I plat to merge this next week.

Sorry, something went wrong.

Uh oh!

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

medmunds and sethmlarson reacted with thumbs up emoji

All reactions

👍 2 reactions

bedevere-app Bot added awaiting merge and removed awaiting review labels

Feb 27, 2025

bitdancer commented Mar 4, 2025

Copy link

Copy Markdown

Member

Please give me another day to look at this before you merge. I think it's fine, but there are a couple things I want to check.

medmunds and encukou reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

bitdancer commented Mar 5, 2025

Copy link

Copy Markdown

Member

OK, there is actually a pretty straightforward solution to this problem, and the functionality is better:

diff --git a/Lib/email/_header_value_parser.py b/Lib/email/_header_value_parser.py
index c0e856306d3..9a51b943733 100644
--- a/Lib/email/_header_value_parser.py
+++ b/Lib/email/_header_value_parser.py
@@ -1053,7 +1053,7 @@ def get_fws(value):
     fws = WhiteSpaceTerminal(value[:len(value)-len(newvalue)], 'fws')
     return fws, newvalue
 
-def get_encoded_word(value):
+def get_encoded_word(value, terminal_type='vtext'):
     """ encoded-word = "=?" charset "?" encoding "?" encoded-text "?="
 
     """
@@ -1092,7 +1092,7 @@ def get_encoded_word(value):
             ew.append(token)
             continue
         chars, *remainder = _wsp_splitter(text, 1)
-        vtext = ValueTerminal(chars, 'vtext')
+        vtext = ValueTerminal(chars, terminal_type)
         _validate_xtext(vtext)
         ew.append(vtext)
         text = ''.join(remainder)
@@ -1134,7 +1134,7 @@ def get_unstructured(value):
         valid_ew = True
         if value.startswith('=?'):
             try:
-                token, value = get_encoded_word(value)
+                token, value = get_encoded_word(value, 'utext')
             except _InvalidEwError:
                 valid_ew = False
             except errors.HeaderParseError:
@@ -1163,7 +1163,7 @@ def get_unstructured(value):
         # the parser to go in an infinite loop.
         if valid_ew and rfc2047_matcher.search(tok):
             tok, *remainder = value.partition('=?')
-        vtext = ValueTerminal(tok, 'vtext')
+        vtext = ValueTerminal(tok, 'utext')
         _validate_xtext(vtext)
         unstructured.append(vtext)
         value = ''.join(remainder)
@@ -2813,7 +2813,7 @@ def _refold_parse_tree(parse_tree, *, policy):
             continue
         tstr = str(part)
         if not want_encoding:
-            if part.token_type == 'ptext':
+            if part.token_type in ('ptext', 'vtext'):
                 # Encode if tstr contains special characters.
                 want_encoding = not SPECIALSNL.isdisjoint(tstr)
             else:

At this point I no longer remember what 'vtext' was supposed to stand for, but 'utext' is obviously 'unstructured text token' ;) The documentation of these bits of the code could use some improvement (not to mention the code itself!), but this fixes the problem pretty much in the way the original code was intended to work, if we imagine that the failure to check whether or not we were dealing with structured text was a bug as opposed to me forgetting about the distinction ;)

medmunds reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

medmunds and others added 2 commits

March 5, 2025 10:32


          Merge branch 'main' into fix-issue-121284

e63ec20


          squash! pythongh-121284: Fix email address header folding with parsed…

c249511

… encoded-word

[Better fix from @bitdancer.]

Co-authored-by: R David Murray <[email protected]>

medmunds commented Mar 5, 2025

Copy link

Copy Markdown

Contributor Author

Nice! That feels much better, and allows unencoding encoded-words in refolding when it's safe. I've updated the PR with your change.

All reactions

Sorry, something went wrong.

Uh oh!

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

bitdancer approved these changes

Mar 6, 2025

View reviewed changes

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

LGTM

Sorry, something went wrong.

Uh oh!

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

All reactions


          Merge branch 'main' into fix-issue-121284

1da1d57

bitdancer added needs backport to 3.9 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

Mar 17, 2025


          Merge branch 'main' into fix-issue-121284

ede0ce8

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

GeezerVison

Conversation

medmunds commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medmunds commented Sep 9, 2024

Uh oh!

medmunds commented Jan 18, 2025

Uh oh!

medmunds commented Jan 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

encukou left a comment

Choose a reason for hiding this comment

Uh oh!

bitdancer commented Mar 4, 2025

Uh oh!

bitdancer commented Mar 5, 2025

Uh oh!

medmunds commented Mar 5, 2025

Uh oh!

bitdancer left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-app Bot commented Mar 18, 2025

Uh oh!

bedevere-app Bot commented Mar 18, 2025

Uh oh!

miss-islington-app Bot commented Mar 18, 2025

Uh oh!

bedevere-app Bot commented Mar 18, 2025

Uh oh!

miss-islington-app Bot commented Mar 18, 2025

Uh oh!

bedevere-app Bot commented Mar 18, 2025

Uh oh!

bedevere-app Bot commented Mar 18, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

medmunds commented Aug 6, 2024 •

edited

Loading

Uh oh!

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

medmunds commented Jan 19, 2025 •

edited

Loading

Uh oh!

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