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

tdwyer · 2023-08-22T00:16:04Z

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.

Issue: [CVE-2023-27043] Parsing errors in email/_parseaddr.py lead to incorrect value in email address part of tuple #102988

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

… tuple to indicate the parsing error (old API)

tdwyer · 2023-08-22T00:22:21Z

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.

theta682 · 2023-08-22T02:48:08Z

Lib/email/utils.py

+ accepted_values = []
+ for v in email_header_fields:
+ s = v.replace('\\(', '').replace('\\)', '')
+ if s.count('(') != s.count(')'):


This does not check )( or any other combination when ) proceeds (. Probably it could be better to iterate through the string and manually increment and decrement verifying that the counter never goes below 0 and it is exactly 0 at the end

Suggested change

if s.count('(') != s.count(')'):

counter = 0

for c in s:

if c == '(':

counter += 1

elif c == ')':

counter -= 1

if counter < 0:

break

if counter != 0:

Looks like this is not necessary. The fix already accounts for this and none of those are an issue.

My current solution works, because instead of trying to actually parse things I just detect an error has occurred by making sure that the resulting number of address 2-Tuples matches the number of Headers that were parsed. Which is why I went this direction. It's super hard to fix the actual parsing logic of RFC 2822 headers :P

./python Python 3.13.0a0 (heads/blah_1:a1cc74c4ee, Aug 21 2023, 18:40:57) [GCC 7.5.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> >>> from email.utils import parseaddr, getaddresses >>> >>> >>> parseaddr('alice@example.org])(<bob@example.com>') ('', '') >>> >>> parseaddr('alice@example.org)(<bob@example.com>') ('', '') >>> >>> parseaddr('alice@example.org <bob@example.com>') ('', '') >>> parseaddr('alice@example.org..<bob@example.com>') ('', '') >>> >>> getaddresses(['alice@example.org..<bob@example.com>']) [('', '')] >>> >>> getaddresses(['alice@example.org])(<bob@example.com>']) [('', '')] >>> >>> getaddresses(['alice@example.org)(<bob@example.com>']) [('', '')]

With the un-patched python your attack results in multiple Tuples being returned

>>> getaddresses(['alice@example.org])(<bob@example.com>']) [('', 'alice@example.org'), ('', ''), ('', '')]

A cleaner way to say it is this. There are countless ways to trigger this parsing error but there is only one error e.g. the parser will return an abnormal number of output tuples. So, rather than trying to detect every possible input which could trigger the bug, I just detect the one error that they all result in.

After looking at this again, it is interesting that my solution works here… why do I check for matching parentheses at all ?

blah@example.com)(<bob&@example.com

I have ideas, but I’ll look into it.

theta682 · 2023-08-22T02:56:35Z

Lib/email/utils.py

+
+ # When a comma is used in the Real Name part it is not a deliminator
+ # So strip those out before counting the commas
+ pattern = r'"[^"]*,[^"]*"|\'[^\']*,[^\']\'*|\\,'


Does this regex support multiple commas inside quotes?

Suggested change

pattern = r'"[^"]*,[^"]*"|\'[^\']*,[^\']\'*|\\,'

pattern = r'''"[^"]*,[^"]*"|'[^']*,[^']'*|\\,'''

The regex works for multiple commas because the Not-In-List [^"] allows for other commas. There just simply has to be at least one comma in between the Quotes.

I'm just curious. The regex proposed by me simply ignores whatever is in single or double quotes and seems to be much simpler than this one. Is there any benefit in ignoring only the quoted parts containing at least one comma? I think it has no effect on the comma-counting logic.

@frenzymadness I don't see your suggested regex. In any case, perhaps what you are suggesting would be fine. I'll think on it. However, from a security perspective I'd like to be as specific as possible, because things get super tricky and I'd not want to implement a fix that is latter proven to be able to be bypassed. It's only the commas in quotes that are an issue here and should be accounted for.

In ether case, I plan on at least implementing the Triple Single-Quote syntax so I don't need to escape the single-quotes.

Additionally, I want looks some more to make sure that single-quotes are valid chars to isolate the Real Name part of the header. It's looking like only Double-Quotes to that, if so then only commas in Double-Quotes should be stripped.

The regex has been proposed in this comment: #102988 (comment)

Okay, I'll consider that. I've been busy with work but I plan on adding the extra test_email.py and updating the regex over this weekend.

theta682 · 2023-08-22T03:03:46Z

Lib/test/test_email/test_email.py

+ eq(utils.getaddresses(['alice@example.org]<bob@example.com>']),
+ [('', '')])


Suggested change

eq(utils.getaddresses(['alice@example.org]<bob@example.com>']),

[('', '')])

eq(utils.getaddresses(['alice@example.org]<bob@example.com>']),

[('', '')])

eq(utils.getaddresses(['alice@example.org])(<bob@example.com>']),

[('', '')])

However, not a bad idea to add extra tests. That way if someone else changes this code in the future these other corner cases will be tested for.

stratakis · 2023-09-06T12:20:41Z

Any updates on that?

tdwyer · 2023-09-09T00:20:07Z

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 · 2023-09-20T14:10:59Z

What is necessary here to move this PR forward?

vstinner · 2023-10-20T15:28:40Z

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 · 2023-10-27T16:11:37Z

Closing this in favor of Victor's updated version.

pythongh-102988: Detect email address parsing errors and return empty…

91b5b37

… tuple to indicate the parsing error (old API)

tdwyer requested a review from a team as a code owner August 22, 2023 00:16

bedevere-bot mentioned this pull request Aug 21, 2023

[CVE-2023-27043] Parsing errors in email/_parseaddr.py lead to incorrect value in email address part of tuple #102988

Closed

bedevere-bot added the awaiting review label Aug 22, 2023

Also strip out escaped commas before counting

4afc952

theta682 reviewed Aug 22, 2023

View reviewed changes

Only account for comma in double-quotes and added tests for this

30d3ed7

vstinner mentioned this pull request Oct 20, 2023

[CVE-2023-27043] gh-102988: Reject malformed addresses in email.parseaddr() #111116

Merged

ambv closed this Oct 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tdwyer commented Aug 22, 2023 •

edited by github-actions bot

Loading

tdwyer commented Aug 22, 2023 •

edited

Loading

theta682 Aug 22, 2023

tdwyer Aug 22, 2023 •

edited

Loading

tdwyer Aug 23, 2023

theta682 Aug 22, 2023

tdwyer Aug 22, 2023 •

edited

Loading

frenzymadness Aug 23, 2023

tdwyer Aug 25, 2023 •

edited

Loading

frenzymadness Aug 26, 2023

tdwyer Aug 31, 2023

theta682 Aug 22, 2023

tdwyer Aug 22, 2023 •

edited

Loading

stratakis commented Sep 6, 2023

tdwyer commented Sep 9, 2023 •

edited

Loading

frenzymadness commented Sep 20, 2023

vstinner commented Oct 20, 2023

ambv commented Oct 27, 2023

- if s.count('(') != s.count(')'):
+ counter = 0
+ for c in s:
+ if c == '(':
+ counter += 1
+ elif c == ')':
+ counter -= 1
+ if counter < 0:
+ break
+ if counter != 0:

	pattern = r'"[^"],[^"]"\|\'[^\'],[^\']\'\|\\,'
	pattern = r'''"[^"],[^"]"\|'[^'],[^']'\|\\,'''

		eq(utils.getaddresses(['alice@example.org]<bob@example.com>']),
		[('', '')])

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

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

Conversation

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

tdwyer commented Aug 22, 2023 • edited Loading

theta682 Aug 22, 2023

Choose a reason for hiding this comment

tdwyer Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

tdwyer Aug 23, 2023

Choose a reason for hiding this comment

theta682 Aug 22, 2023

Choose a reason for hiding this comment

tdwyer Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

frenzymadness Aug 23, 2023

Choose a reason for hiding this comment

tdwyer Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

frenzymadness Aug 26, 2023

Choose a reason for hiding this comment

tdwyer Aug 31, 2023

Choose a reason for hiding this comment

theta682 Aug 22, 2023

Choose a reason for hiding this comment

tdwyer Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

stratakis commented Sep 6, 2023

tdwyer commented Sep 9, 2023 • edited Loading

frenzymadness commented Sep 20, 2023

vstinner commented Oct 20, 2023

ambv commented Oct 27, 2023

tdwyer commented Aug 22, 2023 •

edited by github-actions bot

Loading

tdwyer commented Aug 22, 2023 •

edited

Loading

tdwyer Aug 22, 2023 •

edited

Loading

tdwyer Aug 22, 2023 •

edited

Loading

tdwyer Aug 25, 2023 •

edited

Loading

tdwyer Aug 22, 2023 •

edited

Loading

tdwyer commented Sep 9, 2023 •

edited

Loading