gh-121284: Fix email address header folding with parsed encoded-word

[medmunds]

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

[medmunds]

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

[medmunds]

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

[medmunds]

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.

[encukou]

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.

[bitdancer]

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.

[bitdancer]

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]

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

[bitdancer]

LGTM

[bedevere-app]

GH-131403 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-131404 is a backport of this pull request to the 3.12 branch.

[miss-islington-app]

Sorry, @medmunds and @encukou, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 295b53df2aa18deb625a7da41f7e4babfe6ef34b 3.10

[bedevere-app]

GH-131405 is a backport of this pull request to the 3.11 branch.

[miss-islington-app]

Sorry, @medmunds and @encukou, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 295b53df2aa18deb625a7da41f7e4babfe6ef34b 3.9

[bedevere-app]

GH-131411 is a backport of this pull request to the 3.10 branch.

[bedevere-app]

GH-131412 is a backport of this pull request to the 3.9 branch.