GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-121650 : detect newlines in headers by basbloemsaat · Pull Request #121812 · python/cpython

Conversation

@basbloemsaat

@basbloemsaat basbloemsaat commented Jul 15, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

@encukou as promised, the fix for this issue.

This PR fixes both issues addressed in the issue, both the newlines at the end as well as the encoded newlines.

As this has security implication, it may need to be backported as well.

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

Sorry, something went wrong.

Uh oh!

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

All reactions
basbloemsaat added 2 commits July 14, 2024 14:31
@basbloemsaat
detect line splits, also at the end
d3ca18a
@basbloemsaat
also detect encoded newlines
891d4f8
@basbloemsaat
basbloemsaat requested a review from a team as a code owner July 15, 2024 19:53
@bedevere-app bedevere-app Bot mentioned this pull request Jul 15, 2024
email.policy.default - gotcha with re-using parsed headers with embedded newlines #121650
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Jul 15, 2024
@basbloemsaat
Merge branch 'main' into fix-issue-121650-detect-newlines-in-headers
67d4121
ZeroIntensity
ZeroIntensity requested changes Jul 15, 2024
View reviewed changes

@ZeroIntensity ZeroIntensity 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.

I wasn't able to confirm that this PR fixes #121650. The original reproducer still contains the embedded newline:

from email import message_from_string
from email.policy import default

email_in = """\
To: [email protected]
From: External Sender <[email protected]>
Subject: Here's an =?UTF-8?Q?embedded_newline=0A?=
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

<html>
<head><title>An embeded newline</title></head>
<body>
  <p>I sent you an embedded newline in the subject. How do you like that?!</p>
</body>
</html>
"""

msg = message_from_string(email_in, policy=default)
msg = message_from_string(email_in, policy=default)
for header, value in msg.items():
    del msg[header]
    msg[header] = value
email_out = str(msg)
print(email_out)

Sorry, something went wrong.

Uh oh!

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

basbloemsaat reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/email/policy.py Resolved

Uh oh!

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

Comment thread Misc/NEWS.d/next/Library/2024-07-15-21-43-38.gh-issue-121650.A0FkCh.rst Resolved

Uh oh!

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

Comment thread Misc/NEWS.d/next/Library/2024-07-15-21-43-38.gh-issue-121650.A0FkCh.rst Resolved

Uh oh!

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

@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Jul 15, 2024
basbloemsaat and others added 4 commits July 16, 2024 09:36
@basbloemsaat @ZeroIntensity
Update Lib/email/policy.py
62ceb40
Co-authored-by: Peter Bierma <[email protected]>
@basbloemsaat @ZeroIntensity
Update Misc/NEWS.d/next/Library/2024-07-15-21-43-38.gh-issue-121650.A…
aa4fd24
…0FkCh.rst

Co-authored-by: Peter Bierma <[email protected]>
@basbloemsaat
also check parsed headers for newlines and returns
23fdf74
@basbloemsaat
More specific update blurb
d210941
@basbloemsaat

basbloemsaat commented Jul 16, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

I wasn't able to confirm that this PR fixes #121650. The original reproducer still contains the embedded newline:
...

I missed headers that were parsed from a message, as in original issue. I updated the fix and the tests.

All reactions

Sorry, something went wrong.

Uh oh!

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

@basbloemsaat
basbloemsaat requested a review from ZeroIntensity July 16, 2024 09:13
@basbloemsaat
Merge branch 'main' into fix-issue-121650-detect-newlines-in-headers
118c709
ZeroIntensity
ZeroIntensity approved these changes Jul 16, 2024
View reviewed changes

@ZeroIntensity ZeroIntensity 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.

Confirmed that this fixes #121650. I'm pretty sure this is a security fix (as you could previously inject email headers using this method), so this should need a backport all the way to 3.8

Sorry, something went wrong.

Uh oh!

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

All reactions
@encukou

encukou commented Jul 16, 2024

Copy link
Copy Markdown
Member

@warsaw, @bitdancer, @maxking: as the email experts, do you have any comments?

All reactions

Sorry, something went wrong.

Uh oh!

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

encukou
encukou previously approved these changes Jul 16, 2024
View reviewed changes

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

The fix looks good to me! Thank you for digging into it!

Sorry, something went wrong.

Uh oh!

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

basbloemsaat reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jul 16, 2024
@encukou
encukou dismissed their stale review July 19, 2024 16:06

There's more

@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting merge labels Jul 19, 2024
@encukou

encukou commented Jul 19, 2024

Copy link
Copy Markdown
Member

I take back the review. There's more to this, unfortunately :(

Here's another reproducer:

from email import message_from_string
from email.policy import default

email_in = """\
To: [email protected]
From: External Sender <[email protected]>  =?UTF-8?Q?embedded_newline=0A?=Smuggled-Data: Bad
Subject: foo <bar> Here's an
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

<html>
<head><title>An embeded newline</title></head>
<body>
  <p>I sent you an embedded newline in the subject. How do you like that?!</p>
</body>
</html>
"""

msg = message_from_string(email_in, policy=default)
print(msg)
for header, value in msg.items():
    del msg[header]
    msg[header] = value
email_out = str(msg)
print(email_out)
basbloemsaat reacted with confused emoji
All reactions
  • 😕 1 reaction

Sorry, something went wrong.

Uh oh!

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

@basbloemsaat

basbloemsaat commented Jul 19, 2024

Copy link
Copy Markdown
Contributor Author

I take back the review. There's more to this, unfortunately :(

I'll look into this...

All reactions

Sorry, something went wrong.

Uh oh!

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

basbloemsaat added 3 commits July 19, 2024 19:28
@basbloemsaat
Merge branch 'main' into fix-issue-121650-detect-newlines-in-headers
08e5fe1
@basbloemsaat
Merge branch 'fix-issue-121650-detect-newlines-in-headers' of github.…
c5e9264
…com:basbloemsaat/cpython into fix-issue-121650-detect-newlines-in-headers
@basbloemsaat
check all headers for newlines
5a163b5
@basbloemsaat
basbloemsaat requested a review from encukou July 19, 2024 22:07
@basbloemsaat

basbloemsaat commented Jul 19, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@encukou I tried all header types. This eliminates all newlines. Two notes:

  • date headers (and derivatives) parse the date, and the offending code is eliminated that way
  • the MIME-Version header doesn't decode the encoded newline, so it doesn't break the message. Improving the parsing of that header would mean rewriting the parsing code to do so, but I think that goes beyond the scope of this ticket.
All reactions

Sorry, something went wrong.

Uh oh!

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

@basbloemsaat
Merge branch 'main' into fix-issue-121650-detect-newlines-in-headers
0efe290
@encukou encukou mentioned this pull request Jul 24, 2024
gh-121650: Encode newlines in headers, and verify headers are sound #122233
Merged
@encukou

encukou commented Jul 24, 2024

Copy link
Copy Markdown
Member

After reading up on the email module, I propose to fix the issue in a different part of the code: see #122233.

All reactions

Sorry, something went wrong.

Uh oh!

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

@encukou

encukou commented Aug 1, 2024

Copy link
Copy Markdown
Member

Closing in favour of #122233.
Thank you for the work here, @basbloemsaat! And sorry that I “stole” the issue.

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

@encukou encukou closed this Aug 1, 2024
@basbloemsaat
basbloemsaat deleted the fix-issue-121650-detect-newlines-in-headers branch August 1, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZeroIntensity ZeroIntensity ZeroIntensity approved these changes
@encukou encukou Awaiting requested review from encukou

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

3 participants

@basbloemsaat @encukou @ZeroIntensity
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.