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

tdwyer · 2023-03-24T04:32:54Z

Pull Request title

gh-102988: 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 simple 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.

…error

bedevere-bot · 2023-03-24T04:32:58Z

Most changes to Python require a NEWS entry.

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

cpython-cla-bot · 2023-03-24T04:33:20Z

All commit authors signed the Contributor License Agreement.

…on()

bedevere-bot · 2023-03-24T21:20:36Z

Most changes to Python require a NEWS entry.

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

gpshead

thanks for the PR!

gpshead · 2023-04-28T21:32:13Z

Lib/email/utils.py

+    for v in fieldvalues:
+        s = str(v).replace('\\(', '').replace('\\)', '')
+        if s.count('(') != s.count(')'):
+            fieldvalues.remove(v)


this loop is modifying the apparent list it is iterating over within the loop. that makes reasoning about its exact behavior hard. removing an item could mean you wind up skipping an item, appending an item could make the loop iterate over that. furthermore remove is O(n)... you're re-finding the item to remove to remove it. Also, this code modifys the passed in list in place before returning it.

A better code pattern for this is to build up a new list and return that. never modifying the input.

Something like:

accepted_values = [] for v in fieldvalues: s = v.replace('\\(', '').replace('\\)', '') if s.count('(') != s.count(')'): v = ('', '') accepted_values.append(v) return accepted_values

gpshead · 2023-04-28T21:32:46Z

Lib/email/utils.py

+    """Validate the parsed values are syntactically correct"""
+    for v in parsedvalues:
+        if '[' in v[1]:
+            parsedvalues.remove(v)


same design comment as above, don't modify the list being iterated over and don't modify the argument in place.

gpshead · 2023-04-28T21:38:57Z

Lib/email/utils.py

+
+    n = 0
+    for v in fieldvalues:
+        n += str(v).count(',') + 1


get rid of this str(v)

gpshead · 2023-04-28T21:42:09Z

Lib/email/utils.py


 def getaddresses(fieldvalues):
    """Return a list of (REALNAME, EMAIL) for each fieldvalue."""
+    fieldvalues = _pre_parse_validation(fieldvalues)
    all = COMMASPACE.join(str(v) for v in fieldvalues)


The existing code already calls str(v) here... this is a problem. It means the input can be anything. But we probably cannot change it in a patch release for a security fix.

To avoid propagating this mistake into more lines of code spread out, I suggest changing this function to do:

fieldvalues = [str(v) for v in fieldvalues]

on the first line and get rid of all subsequent str(v) calls on anything from fieldvalues in this function or in functions it calls.

gpshead · 2023-04-28T21:44:05Z

Lib/email/utils.py

+            parsedvalues.append(('', ''))
+
+    return parsedvalues
+

 def getaddresses(fieldvalues):
    """Return a list of (REALNAME, EMAIL) for each fieldvalue."""


add documentation mentioning that fieldvalues that could not be parsed may cause a ('', '') item to be returned in their place.

gpshead · 2023-04-28T21:44:41Z

Lib/email/utils.py

+def _pre_parse_validation(fieldvalues):
+    """Validate the field values are syntactically correct"""
+    for v in fieldvalues:
+        s = str(v).replace('\\(', '').replace('\\)', '')


See later comments on str(v) being bad, we can get rid of it here.

gpshead · 2023-04-28T21:48:54Z

Lib/email/utils.py

@@ -106,12 +106,42 @@ def formataddr(pair, charset='utf-8'):
    return address


+def _pre_parse_validation(fieldvalues):


the name "fieldvalues" is non-specific. i realize it comes from the name in getaddress() but we should make it more clear what these are. email_address_fields perhaps?

Related: I don't this docstring adds meaningful value. naming the function and parameter right along with it being short code is sufficiently self explanatory for this internal function. get rid of the docstring.

gpshead · 2023-04-28T21:51:00Z

Lib/email/utils.py

+def _post_parse_validation(parsedvalues):
+    """Validate the parsed values are syntactically correct"""
+    for v in parsedvalues:
+        if '[' in v[1]:


Add a comment explaining why only [ is not allowed. ideally with a link to the relevant RFC or similar.

gpshead · 2023-04-28T21:51:40Z

Lib/email/utils.py

+    return fieldvalues
+
+
+def _post_parse_validation(parsedvalues):


parsed_email_address_tuples perhaps?

bedevere-bot · 2023-04-28T21:52:03Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Lib/test/test_email/test_email.py

Lib/email/utils.py

bedevere-bot · 2023-05-27T01:09:07Z

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 · 2023-05-27T01:09:20Z

Most changes to Python require a NEWS entry.

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

theta682

stylistic suggestions

theta682 · 2023-05-28T21:30:12Z

Lib/test/test_email/test_email.py

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


Suggested change

[('' ,'')])

[('', '')])

theta682 · 2023-05-28T21:30:20Z

Lib/test/test_email/test_email.py

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


Suggested change

[('' ,'')])

[('', '')])

theta682 · 2023-05-28T21:30:34Z

Lib/test/test_email/test_email.py

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


Suggested change

[('' ,'')])

[('', '')])

theta682 · 2023-05-28T21:30:42Z

Lib/test/test_email/test_email.py

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


Suggested change

[('' ,'')])

[('', '')])

theta682 · 2023-05-28T21:30:54Z

Lib/test/test_email/test_email.py

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


Suggested change

[('' ,'')])

[('', '')])

theta682 · 2023-05-28T21:34:04Z

Lib/test/test_email/test_email.py

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


Suggested change

('' ,''))

[('', '')])

theta682 · 2023-05-28T21:34:09Z

Lib/test/test_email/test_email.py

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


Suggested change

('' ,''))

[('', '')])

theta682 · 2023-05-28T21:34:13Z

Lib/test/test_email/test_email.py

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


Suggested change

('' ,''))

[('', '')])

theta682 · 2023-05-28T21:34:18Z

Lib/test/test_email/test_email.py

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


Suggested change

('' ,''))

[('', '')])

theta682 · 2023-05-28T21:34:23Z

Lib/test/test_email/test_email.py

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


Suggested change

('' ,''))

[('', '')])

Detect parsing errors and return empty tuple to indicate the parsing …

d45802b

…error

tdwyer requested a review from a team as a code owner March 24, 2023 04:32

bedevere-bot added the awaiting review label Mar 24, 2023

arhadthedev changed the title ~~Detect parsing errors and return empty tuple to indicate the parsing error~~ gh-102988: Detect parsing errors and return empty tuple to indicate the parsing error Mar 24, 2023

arhadthedev added stdlib Python modules in the Lib dir topic-email labels Mar 24, 2023

bitdancer changed the title ~~gh-102988: Detect parsing errors and return empty tuple to indicate the parsing error~~ gh-102988: Detect email address parsing errors and return empty tuple to indicate the parsing error (old API) Mar 24, 2023

Appened empty tuple for any tuple removed by def _post_parse_validati…

2ec41f1

…on()

tdwyer mentioned this pull request Mar 24, 2023

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

Closed

gpshead added the type-security A security issue label Mar 27, 2023

gpshead requested changes Apr 28, 2023

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Apr 28, 2023

gpshead self-assigned this Apr 28, 2023

gpshead requested a review from bitdancer April 28, 2023 21:53

theta682 reviewed May 4, 2023

View reviewed changes

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

CharlieZhao95 reviewed May 6, 2023

View reviewed changes

Lib/email/utils.py Show resolved Hide resolved

theta682 reviewed May 28, 2023

View reviewed changes

tdwyer requested review from barneygale, a team, corona10, ethanfurman, gvanrossum and Fidget-Spinner as code owners May 31, 2023 01:01

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) #102990

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

tdwyer commented Mar 24, 2023

bedevere-bot commented Mar 24, 2023

cpython-cla-bot bot commented Mar 24, 2023 •

edited

Loading

bedevere-bot commented Mar 24, 2023

gpshead left a comment

gpshead Apr 28, 2023

gpshead Apr 28, 2023

gpshead Apr 28, 2023

gpshead Apr 28, 2023

gpshead Apr 28, 2023

gpshead Apr 28, 2023

gpshead Apr 28, 2023

gpshead Apr 28, 2023

gpshead Apr 28, 2023

gpshead Apr 28, 2023

bedevere-bot commented Apr 28, 2023

bedevere-bot commented May 27, 2023

bedevere-bot commented May 27, 2023

theta682 left a comment

theta682 May 28, 2023

theta682 May 28, 2023

theta682 May 28, 2023

theta682 May 28, 2023

theta682 May 28, 2023

theta682 May 28, 2023

theta682 May 28, 2023

theta682 May 28, 2023

theta682 May 28, 2023

theta682 May 28, 2023

		@@ -106,12 +106,42 @@ def formataddr(pair, charset='utf-8'):
		return address


		def _pre_parse_validation(fieldvalues):

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

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

Conversation

tdwyer commented Mar 24, 2023

Pull Request title

bedevere-bot commented Mar 24, 2023

cpython-cla-bot bot commented Mar 24, 2023 • edited Loading

bedevere-bot commented Mar 24, 2023

gpshead left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Apr 28, 2023

bedevere-bot commented May 27, 2023

bedevere-bot commented May 27, 2023

theta682 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpython-cla-bot bot commented Mar 24, 2023 •

edited

Loading