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-102988: Detect email address parsing errors and return empty tuple to indicate the parsing error (old API) #105127

Merged
merged 9 commits into from
Jul 10, 2023

Conversation

tdwyer
Copy link
Contributor

@tdwyer tdwyer commented May 31, 2023

This PR is designed to detect parsing errors and return an empty tuple to indicate the parsing error. Additionally, this PR updates the test_email.py to check for these bugs, as well as, adds some other wacky Address Headers that are in the examples of RFC 2822 and makes sure they are being parsed correctly.

I realize that this PR dose not actually track down the bug and fix it. It simply detects the error has happened and returns a parsing error. However, Lib/email/utils.py is a much simpler file than Lib/email/_parseaddr.py, so it is much easier to review this change. Additionally, there are actually multiple bugs which are causing erroneous output. Tracing the code flow for each and fixing them would be prone to error considering all of the wacky stuff that RFC 2822 allows for in Address headers. Finally, this change is actually rather simple.

… tuple to indicate the parsing error (old API)
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@tdwyer
Copy link
Contributor Author

tdwyer commented May 31, 2023

I had to create a new PR to replace the last one because Git got all messed up.

This is the old PR: #102990

@tdwyer tdwyer changed the title gh-102988: Detect email address parsing errors and return empty tupleto indicate the parsing error (old API) gh-102988: Detect email address parsing errors and return empty tuple to indicate the parsing error (old API) May 31, 2023
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@bedevere-bot

This comment was marked as duplicate.

Lib/email/utils.py Outdated Show resolved Hide resolved
Lib/email/utils.py Outdated Show resolved Hide resolved
Lib/test/test_email/test_email.py Outdated Show resolved Hide resolved
@bedevere-bot

This comment was marked as duplicate.

@tdwyer
Copy link
Contributor Author

tdwyer commented Jun 7, 2023

Hum... it looks like CVE-2019-16056 was only fixed for parseaddr() and not getaddresses(). This PR happens to fix that too.
https://www.cve.org/CVERecord?id=CVE-2019-16056

getaddresses()

a = ['a@b.com<z@x.com>']
getaddresses(a)
[('', 'a@b.com'), ('', 'z@x.com')]

parseaddr()

a = ['a@b.com<z@x.com>']
parseaddr(a)
('', 'a@b.com<z@x.com>')

@tdwyer
Copy link
Contributor Author

tdwyer commented Jun 7, 2023

Should I add that NEWS entry that the Bot is asking for?

@CharlieZhao95
Copy link
Contributor

Should I add that NEWS entry that the Bot is asking for?

I think it is needed, NEWS entry is required for most code modifications 😄. Please see: https://devguide.python.org/core-developers/committing/#updating-news-and-what-s-new-in-python

@tdwyer
Copy link
Contributor Author

tdwyer commented Jun 14, 2023

I added NEWS entry, and all Unit tests have passed.

@tdwyer

This comment was marked as resolved.

@CharlieZhao95

This comment was marked as resolved.

@gpshead

This comment was marked as resolved.

@zhuofeng6

This comment was marked as spam.

@gpshead gpshead added the type-security A security issue label Jul 10, 2023
@gpshead gpshead self-assigned this Jul 10, 2023
@gpshead
Copy link
Member

gpshead commented Jul 10, 2023

Docs updated, auto-merge set. The backports beyond 3.12 will require manual work to get version numbered things right. I'd like to let this bake in 3.12beta4 & 3.12rc1 for a while first to see if any practical issues arise before landing the backports in older releases.

it is a security issue so we could backport it all the way to 3.8, but lets see how things go first. it is not high severity.

@gpshead gpshead merged commit 18dfbd0 into python:main Jul 10, 2023
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry, @tdwyer and @gpshead, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 18dfbd035775c15533d13a98e56b1d2bf5c65f00 3.11

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 10, 2023
… tuple to indicate the parsing error (old API) (pythonGH-105127)

Detect email address parsing errors and return empty tuple to indicate the parsing error (old API). This fixes or at least ameliorates CVE-2023-27043.

---------

(cherry picked from commit 18dfbd0)

Co-authored-by: Thomas Dwyer <github@tomd.tel>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
@miss-islington
Copy link
Contributor

Sorry @tdwyer and @gpshead, I had trouble checking out the 3.10 backport branch.
Please retry by removing and re-adding the "needs backport to 3.10" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 18dfbd035775c15533d13a98e56b1d2bf5c65f00 3.10

@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Jul 10, 2023
gpshead added a commit that referenced this pull request Jul 10, 2023
…y tuple to indicate the parsing error (old API) (GH-105127) (#106612)

gh-102988: Detect email address parsing errors and return empty tuple to indicate the parsing error (old API) (GH-105127)

Detect email address parsing errors and return empty tuple to indicate the parsing error (old API). This fixes or at least ameliorates CVE-2023-27043.

---------

(cherry picked from commit 18dfbd0)

Co-authored-by: Thomas Dwyer <github@tomd.tel>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
@tdwyer-wish
Copy link

tdwyer-wish commented Jul 12, 2023

Hello @gpshead Are you going to take care of the backports/cherrypick for 3.10 and 3.11 I'd do them but I've never done that before... Hum, actually I don't think I have permission to do that.

@gpshead
Copy link
Member

gpshead commented Jul 12, 2023

i'll do them, the issue remains assigned to me as a reminder. (anyone can do it, it's just a matter of creating your PR as a fork of the relevant release branch and selecting the matching base branch in the github ui at pr creation time)

@CharlieZhao95
Copy link
Contributor

Hello @gpshead Are you going to take care of the backports/cherrypick for 3.10 and 3.11 I'd do them but I've never done that before... Hum, actually I don't think I have permission to do that.

Please see https://devguide.python.org/core-developers/committing/#backporting-changes-to-an-older-version for details.

@gpshead
Copy link
Member

gpshead commented Jul 12, 2023

this appears to have caused #106669

@@ -87,7 +92,7 @@ of the new API.
This method returns a list of 2-tuples of the form returned by ``parseaddr()``.
*fieldvalues* is a sequence of header field values as might be returned by
:meth:`Message.get_all <email.message.Message.get_all>`. Here's a simple
example that gets all the recipients of a message::
Copy link
Contributor

Choose a reason for hiding this comment

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

@tdwyer Is this right? Isn’t the next paragraph still a literal block? Why have you removed this colon?

gpshead added a commit to gpshead/cpython that referenced this pull request Jul 14, 2023
…g errors and return empty tuple to indicate the parsing error (old API) (python#105127)"

This reverts commit 18dfbd0.

See python#106669.
gpshead added a commit that referenced this pull request Jul 21, 2023
…#105127)" (#106733)

This reverts commit 18dfbd0.
Adds a regression test from the issue.

See #106669.
gpshead added a commit to gpshead/cpython that referenced this pull request Jul 21, 2023
… parsing errors ... (pythonGH-105127)" (pythonGH-106733)

This reverts commit 18dfbd0.
Adds a regression test from the issue.

See python#106669..
(cherry picked from commit a31dea1)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
gpshead added a commit that referenced this pull request Jul 21, 2023
…ors ... (GH-105127)" (GH-106733) (#106941)

This reverts commit 18dfbd0.
Adds a regression test from the issue.

See #106669..
(cherry picked from commit a31dea1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes type-security A security issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants