gh-121650: Encode newlines in headers, and verify headers are sound #122233

encukou · 2024-07-24T13:55:36Z

I've spent the day reading through the email module, and RFCs, and I believe I found a better place to fix the issue.
This involved lots of experimentation, so I'm sending an alternative PR rather than a review on yours.

The generator (writer) verifies that the representation of each header is sound (a parser won't treat it as multiple headers, start-of-body, or part of another header). That should cover custom fold() implementations or Header subclasses.
- However, some user out there is probably misusing such header injection in working code, so, I added a policy attribute to turn it back.
Newlines are encoded in fold(), just like undecodable bytes and other special characters.

Overall, this means that we treat newlines as valid content of headers, but “escape” them when such a header is serialized to text.

This PR is a proof of concept. It needs tests and documentation, but I'm out of time for today, and I wanted to share what I have.

Does this look reasonable to you?

Issue: email.policy.default - gotcha with re-using parsed headers with embedded newlines #121650

This should fail for custom fold() implementations that aren't careful about newlines.

Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. --- Credit for an earlier attempt: Co-Authored-By: Bas Bloemsaat <bas@bloemsaat.org>

basbloemsaat · 2024-07-25T20:10:15Z

That sounds entirely reasonable, and conforms to the RFCs.

Two points:

what would be the use of keeping this check in email.policy.header_store_parse ?
I found one case that is not covered by this (contrived, I admit):

from email import message_from_string

email_in = """
Subject: foo <bar>\nBCC: injected@example.com
To: incoming+tag@me.example.com
From: External Sender <sender@them.example.com>

message body
"""

msg = message_from_string(email_in)
print(msg)

encukou · 2024-07-27T12:22:24Z

what would be the use of keeping this check in email.policy.header_store_parse ?

This is in the branch that handles strings (rather than custom Header object). I'm not clears what kind of format that string is supposed to be in.
Keeping the check means that if someone relied on this, they'll get the same error as before. Also, the indication that something is wrong will come earlier.

I found one case that is not covered by this (contrived, I admit)

That \n there is Python syntax, by the time it gets to message_from_string it's the same as a “real” newline.
If you use a raw string r""", or read from a file, the header remains on a single line.

I'm not touching other instances in this file, since this PR might be backported to very old versions.

encukou · 2024-07-29T15:01:50Z

@serhiy-storchaka, would you like to review this?
@warsaw, @bitdancer, @maxking, as email experts, do you have any comments?

bedevere-bot · 2024-07-29T22:21:50Z

🤖 New build scheduled with the buildbot fleet by @encukou for commit af41733 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

serhiy-storchaka

LGTM.

Lib/email/_header_value_parser.py

Lib/test/test_email/test_policy.py

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

bedevere-app · 2024-08-02T09:30:39Z

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

…s are sound (pythonGH-122233) GH-GH- Encode header parts that contain newlines Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. GH-GH- Verify that email headers are well-formed This should fail for custom fold() implementations that aren't careful about newlines. (cherry picked from commit 0976339) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

bedevere-app · 2024-08-02T12:56:47Z

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

…s are sound (pythonGH-122233) Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. This should fail for custom fold() implementations that aren't careful about newlines. (cherry picked from commit 0976339) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

bedevere-app · 2024-08-02T13:04:53Z

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

… are sound (pythonGH-122233) Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. This should fail for custom fold() implementations that aren't careful about newlines. (cherry picked from commit 0976339) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

bedevere-app · 2024-08-02T13:17:44Z

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

… are sound (pythonGH-122233) Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. This should fail for custom fold() implementations that aren't careful about newlines. (cherry picked from commit 0976339) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

bedevere-app · 2024-08-02T13:19:34Z

GH-122611 is a backport of this pull request to the 3.8 branch.

…sound (GH-122233) (#122484) gh-121650: Encode newlines in headers, and verify headers are sound (GH-122233) GH-GH- Encode header parts that contain newlines Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. GH-GH- Verify that email headers are well-formed This should fail for custom fold() implementations that aren't careful about newlines. (cherry picked from commit 0976339) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…sound (GH-122233) (#122599) * gh-121650: Encode newlines in headers, and verify headers are sound (GH-122233) - Encode header parts that contain newlines Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. - Verify that email headers are well-formed This should fail for custom fold() implementations that aren't careful about newlines. Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> (cherry picked from commit 0976339) * Document changes as made in 3.12.5

…s are sound pythongh-121650: Encode newlines in headers, and verify headers are sound (pythonGH-122233) Encode header parts that contain newlines Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. Verify that email headers are well-formed This should fail for custom fold() implementations that aren't careful about newlines. (cherry picked from commit 0976339) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…s are sound (pythonGH-122233) Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. This should fail for custom fold() implementations that aren't careful about newlines. (cherry picked from commit 0976339) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

headers are sound (pythonGH-122233) Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. This should fail for custom fold() implementations that aren't careful about newlines. (cherry picked from commit 0976339) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> This patch also contains modified commit cherry picked from c5bba85. This commit was backported to simplify the backport of the other commit fixing CVE. The only modification is a removal of one test case which tests multiple changes in Python 3.7 and it wasn't working properly with Python 3.6 where we backported only one change. Co-authored-by: bsiem <52461103+bsiem@users.noreply.github.com>

headers are sound (pythonGH-122233) Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. This should fail for custom fold() implementations that aren't careful about newlines. (cherry picked from commit 0976339) This patch also contains modified commit cherry picked from c5bba85. This commit was backported to simplify the backport of the other commit fixing CVE. The only modification is a removal of one test case which tests multiple changes in Python 3.7 and it wasn't working properly with Python 3.6 where we backported only one change. Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> Co-authored-by: bsiem <52461103+bsiem@users.noreply.github.com>

…ound (pythonGH-122233) ## Encode header parts that contain newlines Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. ## Verify that email headers are well-formed This should fail for custom fold() implementations that aren't careful about newlines. Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…ound (GH-122233) (#122611) Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. This should fail for custom fold() implementations that aren't careful about newlines. (cherry picked from commit 0976339) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…sound (GH-122233) (#122608) Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. Verify that email headers are well-formed. This should fail for custom fold() implementations that aren't careful about newlines. (cherry picked from commit 0976339) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…sound (GH-122233) (#122609) Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. This should fail for custom fold() implementations that aren't careful about newlines. (cherry picked from commit 0976339) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…ound (GH-122233) (#122610) Per RFC 2047: > [...] these encoding schemes allow the > encoding of arbitrary octet values, mail readers that implement this > decoding should also ensure that display of the decoded data on the > recipient's terminal will not cause unwanted side-effects It seems that the "quoted-word" scheme is a valid way to include a newline character in a header value, just like we already allow undecodable bytes or control characters. They do need to be properly quoted when serialized to text, though. This should fail for custom fold() implementations that aren't careful about newlines. (cherry picked from commit 0976339) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Bas Bloemsaat <bas@bloemsaat.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

Changelog: https://docs.python.org/release/3.12.5/whatsnew/changelog.html Include security fix CVE-2024-6923 Reference: https://nvd.nist.gov/vuln/detail/CVE-2024-6923 python/cpython#122233 (From OE-Core rev: 777cad793a5b07d392b1d9875530fb5480e75863) Signed-off-by: Vijay Anusuri <vanusuri@mvista.com> Signed-off-by: Steve Sakoman <steve@sakoman.com>

encukou and others added 2 commits July 24, 2024 15:30

Verify that email headers are well-formed

921cbfd

This should fail for custom fold() implementations that aren't careful about newlines.

encukou requested a review from serhiy-storchaka July 24, 2024 13:55

bedevere-app bot mentioned this pull request Jul 24, 2024

email.policy.default - gotcha with re-using parsed headers with embedded newlines #121650

Closed

encukou mentioned this pull request Jul 24, 2024

gh-121650 : detect newlines in headers #121812

Closed

Add tests and documentation

59c06c3

encukou added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Jul 27, 2024

encukou added 3 commits July 27, 2024 16:10

Add a blurb

8e7d6f1

Add the news to 3.13; there's still hope for backporting

ef65562

Don't mark up True

64dcb44

I'm not touching other instances in this file, since this PR might be backported to very old versions.

encukou marked this pull request as ready for review July 29, 2024 13:18

encukou requested a review from a team as a code owner July 29, 2024 13:18

bedevere-app bot added the awaiting core review label Jul 29, 2024

Document HeaderWriteError

af41733

encukou added needs backport to 3.8 needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Jul 29, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 29, 2024

serhiy-storchaka approved these changes Jul 30, 2024

View reviewed changes

Lib/email/_header_value_parser.py Outdated Show resolved Hide resolved

Lib/test/test_email/test_policy.py Outdated Show resolved Hide resolved

bedevere-app bot added awaiting merge and removed awaiting core review labels Jul 30, 2024

Apply suggestions from code review

596a25b

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

serhiy-storchaka approved these changes Jul 30, 2024

View reviewed changes

bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Aug 2, 2024

bedevere-app bot removed the needs backport to 3.11 only security fixes label Aug 2, 2024

bedevere-app bot removed the needs backport to 3.10 only security fixes label Aug 2, 2024

bedevere-app bot removed the needs backport to 3.9 only security fixes label Aug 2, 2024

bedevere-app bot removed the needs backport to 3.8 label Aug 2, 2024

julian-klode mentioned this pull request Sep 17, 2024

Regression: ImportError for HeaderWriteError in long-running process post-Python update for CVE-2024-6923 #124170

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-121650: Encode newlines in headers, and verify headers are sound #122233

gh-121650: Encode newlines in headers, and verify headers are sound #122233

encukou commented Jul 24, 2024 •

edited by bedevere-app bot

Loading

basbloemsaat commented Jul 25, 2024

encukou commented Jul 27, 2024

encukou commented Jul 29, 2024

bedevere-bot commented Jul 29, 2024

serhiy-storchaka left a comment

bedevere-app bot commented Aug 2, 2024

bedevere-app bot commented Aug 2, 2024

bedevere-app bot commented Aug 2, 2024

bedevere-app bot commented Aug 2, 2024

bedevere-app bot commented Aug 2, 2024

gh-121650: Encode newlines in headers, and verify headers are sound #122233

gh-121650: Encode newlines in headers, and verify headers are sound #122233

Conversation

encukou commented Jul 24, 2024 • edited by bedevere-app bot Loading

basbloemsaat commented Jul 25, 2024

encukou commented Jul 27, 2024

encukou commented Jul 29, 2024

bedevere-bot commented Jul 29, 2024

serhiy-storchaka left a comment

Choose a reason for hiding this comment

bedevere-app bot commented Aug 2, 2024

bedevere-app bot commented Aug 2, 2024

bedevere-app bot commented Aug 2, 2024

bedevere-app bot commented Aug 2, 2024

bedevere-app bot commented Aug 2, 2024

encukou commented Jul 24, 2024 •

edited by bedevere-app bot

Loading