Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jan 19, 2025

Conversation

medmunds
Copy link
Contributor

@medmunds medmunds commented Aug 6, 2024

Fixes #80222 (bpo-36041).

Email generators using email.policy.default could incorrectly omit the quote ('"') characters from a quoted-string during header refolding, leading to invalid address headers and enabling header spoofing. This change restores the quote characters on a bare-quoted-string as the header is refolded, and escapes backslash and quote chars in the string.

Fix is adapted from PR #12054.

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

Email generators using email.policy.default could incorrectly omit the
quote ('"') characters from a quoted-string during header refolding,
leading to invalid address headers and enabling header spoofing. This
change restores the quote characters on a bare-quoted-string as the
header is refolded, and escapes backslash and quote chars in the string.
@medmunds
Copy link
Contributor Author

Requesting a review from @bitdancer (who probably knows the most about this code, and was actively involved in the discussion on the original report five years ago).

(See also PR #122754 which aims to fix a similar security issue with encoded-words.)

Copy link
Member

@encukou encukou left a 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 & RFC, this is correct.
I'd appreciate an additional review from an email expert though.

@medmunds

This comment was marked as outdated.

Copy link
Member

@bitdancer bitdancer left a comment

Choose a reason for hiding this comment

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

Great work, thank you. If I'd gotten that test you modified correct in the first place, this would have been fixed long ago.

Lib/email/_header_value_parser.py Outdated Show resolved Hide resolved
Lib/email/_header_value_parser.py Outdated Show resolved Hide resolved
Lib/email/_header_value_parser.py Outdated Show resolved Hide resolved
@bedevere-app
Copy link

bedevere-app bot commented Jan 18, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@medmunds
Copy link
Contributor Author

I have made the requested changes; please review again.

@bitdancer thanks for the review!

@bedevere-app
Copy link

bedevere-app bot commented Jan 18, 2025

Thanks for making the requested changes!

@bitdancer, @encukou: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested review from bitdancer and encukou January 18, 2025 21:41
@medmunds
Copy link
Contributor Author

@encukou and @bitdancer thanks for the reviews. Any chance you could also look at the related PR #122754, which fixes a similar problem in refolding rfc2047 encoded-words?

@bitdancer
Copy link
Member

Sure, I've added it to my list. I'm finally back to contributing more actively after years away, but it may still take me a while to get to it ;)

Copy link
Member

@bitdancer bitdancer left a comment

Choose a reason for hiding this comment

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

LGTM

@bitdancer bitdancer added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Jan 19, 2025
@bitdancer bitdancer merged commit 5aaf416 into python:main Jan 19, 2025
54 of 57 checks passed
@miss-islington-app
Copy link

Thanks @medmunds for the PR, and @bitdancer for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10, 3.11, 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 19, 2025
…ing (pythonGH-122753)

Email generators using email.policy.default could incorrectly omit the
quote ('"') characters from a quoted-string during header refolding,
leading to invalid address headers and enabling header spoofing. This
change restores the quote characters on a bare-quoted-string as the
header is refolded, and escapes backslash and quote chars in the string.
(cherry picked from commit 5aaf416)

Co-authored-by: Mike Edmunds <medmunds@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 19, 2025
…ing (pythonGH-122753)

Email generators using email.policy.default could incorrectly omit the
quote ('"') characters from a quoted-string during header refolding,
leading to invalid address headers and enabling header spoofing. This
change restores the quote characters on a bare-quoted-string as the
header is refolded, and escapes backslash and quote chars in the string.
(cherry picked from commit 5aaf416)

Co-authored-by: Mike Edmunds <medmunds@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Jan 19, 2025

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

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jan 19, 2025
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 19, 2025
…ing (pythonGH-122753)

Email generators using email.policy.default could incorrectly omit the
quote ('"') characters from a quoted-string during header refolding,
leading to invalid address headers and enabling header spoofing. This
change restores the quote characters on a bare-quoted-string as the
header is refolded, and escapes backslash and quote chars in the string.
(cherry picked from commit 5aaf416)

Co-authored-by: Mike Edmunds <medmunds@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Jan 19, 2025

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Jan 19, 2025
@miss-islington-app
Copy link

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

cherry_picker 5aaf41685834901e4ed0a40f4c055b92991a0bb5 3.10

@bedevere-app
Copy link

bedevere-app bot commented Jan 19, 2025

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

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Jan 19, 2025
@miss-islington-app
Copy link

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

cherry_picker 5aaf41685834901e4ed0a40f4c055b92991a0bb5 3.9

bitdancer pushed a commit that referenced this pull request Jan 19, 2025
…ring (GH-122753) (#129007)

gh-80222: Fix email address header folding with long quoted-string (GH-122753)

Email generators using email.policy.default could incorrectly omit the
quote ('"') characters from a quoted-string during header refolding,
leading to invalid address headers and enabling header spoofing. This
change restores the quote characters on a bare-quoted-string as the
header is refolded, and escapes backslash and quote chars in the string.
(cherry picked from commit 5aaf416)

Co-authored-by: Mike Edmunds <medmunds@gmail.com>
bitdancer pushed a commit that referenced this pull request Jan 19, 2025
…ring (GH-122753) (#129008)

gh-80222: Fix email address header folding with long quoted-string (GH-122753)

Email generators using email.policy.default could incorrectly omit the
quote ('"') characters from a quoted-string during header refolding,
leading to invalid address headers and enabling header spoofing. This
change restores the quote characters on a bare-quoted-string as the
header is refolded, and escapes backslash and quote chars in the string.
(cherry picked from commit 5aaf416)

Co-authored-by: Mike Edmunds <medmunds@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Jan 21, 2025

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

@bedevere-app bedevere-app bot removed the needs backport to 3.10 only security fixes label Jan 21, 2025
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 21, 2025
…ing (python#122753)

Email generators using email.policy.default could incorrectly omit the
quote ('"') characters from a quoted-string during header refolding,
leading to invalid address headers and enabling header spoofing. This
change restores the quote characters on a bare-quoted-string as the
header is refolded, and escapes backslash and quote chars in the string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs backport to 3.9 only security fixes topic-email type-security A security issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

email: folding of quoted string in display_name violates RFC
5 participants