◐ Shell
reader mode source ↗
Skip to content

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

Closed
tdwyer wants to merge 3 commits into
python:mainfrom
tdwyer:refix_issues102988
Closed

gh-102988: Detect email address parsing errors and return empty tuple to indicate the parsing error (old API)#108250
tdwyer wants to merge 3 commits into
python:mainfrom
tdwyer:refix_issues102988

Conversation

@tdwyer

@tdwyer tdwyer commented Aug 22, 2023 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

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.

📚 Documentation preview 📚: https://cpython-previews--108250.org.readthedocs.build/

@tdwyer
pythongh-102988: Detect email address parsing errors and return empty… …
91b5b37 
… tuple to indicate the parsing error (old API)
@tdwyer tdwyer requested a review from a team as a code owner August 22, 2023 00:16
@bedevere-bot bedevere-bot mentioned this pull request Aug 21, 2023
Closed
@tdwyer

tdwyer commented Aug 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Note, I just have the regex looking for both Double Quotes and Single Quotes but I think only Double-Quotes are allowed to specify a Real Name part.

Let me know if Single Quotes can NOT be used to wrap the Real Name e.g. 'Bob Bobby' <bbobby@example.com>

I'll remove that from the regex if so

Additionally, I'm also stripping out escaped commas before counting e.g. \,
Let me know if I should not be doing that.

@stratakis

stratakis commented Sep 6, 2023

Copy link
Copy Markdown
Contributor

Any updates on that?

@tdwyer

tdwyer commented Sep 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

After some investigation, I confirmed that I was being too carful before. Only commas in double-quotes should be accounted for not commas in single-quotes nor commas with prefixed with a backslash

A comma in single-quotes and a comma prefixed by a backslash trigger the bug. However, a comma in double-quotes is parsed correctly. This is how all of those cases behave in:

Python 2.7.17
Python 3.6.9
Python 3.11.5

 s = "'ae.com,' <b@e.com>"
 parseaddr(s)
('', "'ae.com")
 getaddresses([s])
[('', "'ae.com"), ("'", 'b@e.com')]

 s = '"ae.com," <b@e.com>'
 getaddresses([s])
[('ae.com,', 'b@e.com')]
 parseaddr(s)
('ae.com,', 'b@e.com')

 s = 'ae.com\,<b@e.com>'
 parseaddr(s)
('', 'ae.com\\')
 getaddresses([s])
[('', 'ae.com\\'), ('', 'b@e.com')]

Additionally, somehow the fix for CVE-2019-16056 regarding alice@example.com <bob@example.com> seems to have been undone and is exploitable in:

Python 2.7.17
Python 3.6.9
Python 3.11.5

 getaddresses(['a@e.com <b@e.com>']) 
[('', 'a@e.com'), ('', 'b@e.com')]
  
 parseaddr('a@e.com <b@e.com>')                                         
('', 'a@e.com')
  
 parseaddr('a@e.com<b@e.com>')                                          
('', 'a@e.com')

So, this PR fixes both CVE-2019-16056 and CVE-2023-27043

@frenzymadness

frenzymadness commented Sep 20, 2023

Copy link
Copy Markdown
Contributor

What is necessary here to move this PR forward?

@vstinner vstinner mentioned this pull request Oct 20, 2023
Merged
@vstinner

vstinner commented Oct 20, 2023

Copy link
Copy Markdown
Member

My colleague Lumir Balhar @frenzymadness ran an impact check on this PR on Fedora: in short, there is no impact, the test suite of all Python packages (in Fedora) pass with the change. While there were some build errors, they were unrelated to the email issue. For details, see https://copr.fedorainfracloud.org/coprs/lbalhar/email-CVE/builds/ COPR which as more than 4300 builds.

I created a copy of this PR to add a strict parameter: PR #111116.

@ambv

ambv commented Oct 27, 2023

Copy link
Copy Markdown
Contributor

Closing this in favor of Victor's updated version.

@ambv ambv closed this Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@theta682 theta682 theta682 left review comments

@frenzymadness frenzymadness frenzymadness left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tdwyer @stratakis @frenzymadness @vstinner @ambv @theta682 @bedevere-bot