Skip to content

bpo-43123: revise logic for cr-lf characters in email header names and values #24475

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 25 commits into from
Closed

bpo-43123: revise logic for cr-lf characters in email header names and values #24475

wants to merge 25 commits into from

Conversation

rrhodes
Copy link
Contributor

@rrhodes rrhodes commented Feb 7, 2021 •
edited
Loading

What?

  1. Raise ValueError if an email header name includes linefeed or carriage return characters, and
  2. allow linefeed and carriage return characters in email header values, so long as a non-space character follows. Where this applies, append a space character to signal continuation of the same header value.

Why?

  1. To prevent arbitrary MIME headers being injected into email, and
  2. to allow a continuation of header values where linefeed or carriage return characters exist, like this sample message.

To quote Martin Ortner, author of this issue:

I discussed the [header injection] vulnerability in private with one of the email module maintainers and he considers that it's not a vulnerability.

https://bugs.python.org/issue43123

Ross Rhodes added 17 commits December 27, 2020 21:08
LMTP Unix-domain socket assumes global default timeout when timeout n…
b09e617 
…ot provided
Merge branch 'master' of https://github.com/python/cpython into master
efa5de7
Merge branch 'master' of https://github.com/python/cpython into master
30dad41
Merge branch 'master' of https://github.com/python/cpython into master
f771ae9
Merge branch 'master' of https://github.com/python/cpython into master
e34f890
Merge branch 'master' of https://github.com/python/cpython into master
6af621a
Resolve merge conflict
ca38f0f
Merge branch 'master' of https://github.com/python/cpython into master
2e4bfbe
Merge branch 'master' of https://github.com/python/cpython into master
5c4984c
Merge branch 'master' of https://github.com/python/cpython into master
cbc76d1
Merge branch 'master' of https://github.com/python/cpython into master
7201dc0
Merge branch 'master' of https://github.com/python/cpython into master
ce22e00
Merge branch 'master' of https://github.com/python/cpython into master
567f4f9
Resolve smtplib merge conflicts
3b32256
Resolve merge conflicts
5abd67e
Raise ValueError if email header name includes CR-LF character
46c7659
Add news entry and verify header name is string for newline checks
2f9e2ba
@rrhodes rrhodes requested a review from a team as a code owner February 7, 2021 13:19
@rrhodes
Copy link
Contributor Author

rrhodes commented Feb 9, 2021

@bitdancer, @terryjreedy, @maxking, sorry to ping you out of the blue, but it looks like you're the only three to contribute to the email policy code in the past. Any of you happy to review these changes for me, please?

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Mar 12, 2021
@tintinweb
Copy link

sorry for the delay @rrhodes. I'm the one that originally reported the issue (not a python core-dev nor rfc2045 pro 🤓).

IMHO instead of bailing when finding a \n([!\s]+) in the value it should be encoded as a value continuation (\nSOMETEXT -> \n<space>SOMETEXT). I think otherwise this might break content like this:

image

tintinweb
tintinweb reviewed May 28, 2021
View reviewed changes
tintinweb
tintinweb reviewed May 28, 2021
View reviewed changes
@rrhodes
Copy link
Contributor Author

rrhodes commented May 29, 2021

Thanks for the feedback @tintinweb, I'll take a look into this in the next couple days. 👍🏻

@rrhodes rrhodes removed the request for review from a team May 29, 2021 21:47
@rrhodes
Copy link
Contributor Author

rrhodes commented May 29, 2021 •
edited
Loading

@tintinweb I think I've understood the changes you were requesting here: you were happy with the logic for header names, but wanted support for CR and LF characters in header values in certain cases, which were previously always raising a ValueError.

Further feedback welcome. If I've misunderstood what is required, I'm happy to discuss this in more detail.

@rrhodes rrhodes changed the title bpo-43123: Raise ValueError if email header name includes CR-LF characters bpo-43123: revise logic for cr-lf characters in email header names and values May 30, 2021
@ambv ambv requested a review from a team August 29, 2021 11:12
@ambv
Copy link
Contributor

ambv commented Aug 29, 2021

I don't understand @tintinweb's suggestion to relax the check for header values. That check is in place right now in the email library code. What this PR was meant to be about was also introducing a similar check for header names. Introducing the latter while relaxing the former looks wrong to me.

@rrhodes
Copy link
Contributor Author

rrhodes commented Oct 8, 2021

Closing this PR: it's been stale for over six months.

@rrhodes rrhodes closed this Oct 8, 2021
@martinortner martinortner mannequin mentioned this pull request Aug 4, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tintinweb tintinweb tintinweb left review comments

Assignees
No one assigned
Labels
awaiting review stale Stale PR or inactive for long period of time.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rrhodes @tintinweb @ambv @the-knights-who-say-ni @bedevere-bot