Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91b5b37
gh-102988: Detect email address parsing errors and return empty tuple…
tdwyer Aug 22, 2023
4afc952
Also strip out escaped commas before counting
tdwyer Aug 22, 2023
30d3ed7
Only account for comma in double-quotes and added tests for this
tdwyer Sep 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Doc/whatsnew/3.13.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,14 @@ dbm
from the database.
(Contributed by Dong-hee Na in :gh:`107122`.)

email
-----

* :func:`email.utils.getaddresses` and :func:`email.utils.parseaddr` now return
``('', '')`` 2-tuples in more situations where invalid email addresses are
encountered instead of potentially inaccurate values.
(Contributed by Thomas Dwyer for :gh:`102988` to ameliorate CVE-2023-27043.)

io
--

Expand Down
71 changes: 65 additions & 6 deletions Lib/email/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,62 @@ def formataddr(pair, charset='utf-8'):
return address


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

Choose a reason for hiding this comment

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

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:

Copy link
Contributor Author

@tdwyer tdwyer Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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('[email protected]])(<[email protected]>')
('', '')
>>> 
>>> parseaddr('[email protected])(<[email protected]>')
('', '')
>>> 
>>> parseaddr('[email protected] <[email protected]>')
('', '')
>>> parseaddr('[email protected]..<[email protected]>')
('', '')
>>> 
>>> getaddresses(['[email protected]..<[email protected]>'])
[('', '')]
>>> 
>>> getaddresses(['[email protected]])(<[email protected]>'])
[('', '')]
>>> 
>>> getaddresses(['[email protected])(<[email protected]>'])
[('', '')]

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

>>> getaddresses(['[email protected]])(<[email protected]>'])
[('', '[email protected]'), ('', ''), ('', '')]

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

[email protected])(<bob&@example.com

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

v = "('', '')"
accepted_values.append(v)

return accepted_values


def _post_parse_validation(parsed_email_header_tuples):
accepted_values = []
# The parser would have parsed a correctly formatted domain-literal
# The existence of an [ after parsing indicates a parsing failure
for v in parsed_email_header_tuples:
if '[' in v[1]:
v = ('', '')
accepted_values.append(v)

return accepted_values


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

When parsing fails for a fieldvalue, a 2-tuple of ('', '') is returned in
its place.

If the resulting list of parsed address is greater than number of
fieldvalues in the input list a parsing error has occurred, so a list
containing a single empty 2-tuple [('', '')] is returned in its place.
This is done to avoid invalid output.

Malformed input: getaddresses(['[email protected] <[email protected]>'])
Invalid output: [('', '[email protected]'), ('', '[email protected]')]
Safe output: [('', '')]
"""
fieldvalues = [str(v) for v in fieldvalues]
fieldvalues = _pre_parse_validation(fieldvalues)
all = COMMASPACE.join(v for v in fieldvalues)
a = _AddressList(all)
return a.addresslist
result = _post_parse_validation(a.addresslist)

# 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'"[^"]*,[^"]*"'
n = 0
for v in fieldvalues:
v = re.sub(pattern, '', v)
n += v.count(',') + 1

if len(result) != n:
return [('', '')]

return result


def _format_timetuple_and_zone(timetuple, zone):
Expand Down Expand Up @@ -212,9 +262,18 @@ def parseaddr(addr):
Return a tuple of realname and email address, unless the parse fails, in
which case return a 2-tuple of ('', '').
"""
addrs = _AddressList(addr).addresslist
if not addrs:
return '', ''
if isinstance(addr, list):
addr = addr[0]

if not isinstance(addr, str):
return ('', '')

addr = _pre_parse_validation([addr])[0]
addrs = _post_parse_validation(_AddressList(addr).addresslist)

if not addrs or len(addrs) > 1:
return ('', '')

return addrs[0]


Expand Down
112 changes: 83 additions & 29 deletions Lib/test/test_email/test_email.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3319,32 +3319,96 @@ def test_getaddresses(self):
[('Al Person', '[email protected]'),
('Bud Person', '[email protected]')])

def test_getaddresses_comma_in_name(self):
"""GH-106669 regression test."""
self.assertEqual(
utils.getaddresses(
[
'"Bud, Person" <[email protected]>',
'[email protected] (Al Person)',
'"Mariusz Felisiak" <[email protected]>',
]
),
[
('Bud, Person', '[email protected]'),
('Al Person', '[email protected]'),
('Mariusz Felisiak', '[email protected]'),
],
)
def test_getaddresses_parsing_errors(self):
"""Test for parsing errors from CVE-2023-27043 and CVE-2019-16056"""
eq = self.assertEqual
eq(utils.getaddresses(['[email protected](<[email protected]>']),
[('', '')])
eq(utils.getaddresses(['[email protected])<[email protected]>']),
[('', '')])
eq(utils.getaddresses(['[email protected]<<[email protected]>']),
[('', '')])
eq(utils.getaddresses(['[email protected]><[email protected]>']),
[('', '')])
eq(utils.getaddresses(['[email protected]@<[email protected]>']),
[('', '')])
eq(utils.getaddresses(['[email protected],<[email protected]>']),
[('', '[email protected]'), ('', '[email protected]')])
eq(utils.getaddresses(['[email protected];<[email protected]>']),
[('', '')])
eq(utils.getaddresses(['[email protected]:<[email protected]>']),
[('', '')])
eq(utils.getaddresses(['[email protected].<[email protected]>']),
[('', '')])
eq(utils.getaddresses(['[email protected]"<[email protected]>']),
[('', '')])
eq(utils.getaddresses(['[email protected][<[email protected]>']),
[('', '')])
eq(utils.getaddresses(['[email protected]]<[email protected]>']),
[('', '')])
Comment on lines +3347 to +3348
Copy link

Choose a reason for hiding this comment

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

Suggested change
eq(utils.getaddresses(['[email protected]]<[email protected]>']),
[('', '')])
eq(utils.getaddresses(['[email protected]]<[email protected]>']),
[('', '')])
eq(utils.getaddresses(['[email protected]])(<[email protected]>']),
[('', '')])

Copy link
Contributor Author

@tdwyer tdwyer Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

eq(utils.getaddresses(['"Alice, [email protected]" <[email protected]>']),
[('Alice, [email protected]', '[email protected]')])

def test_parseaddr_parsing_errors(self):
"""Test for parsing errors from CVE-2023-27043 and CVE-2019-16056"""
eq = self.assertEqual
eq(utils.parseaddr(['[email protected](<[email protected]>']),
('', ''))
eq(utils.parseaddr(['[email protected])<[email protected]>']),
('', ''))
eq(utils.parseaddr(['[email protected]<<[email protected]>']),
('', ''))
eq(utils.parseaddr(['[email protected]><[email protected]>']),
('', ''))
eq(utils.parseaddr(['[email protected]@<[email protected]>']),
('', ''))
eq(utils.parseaddr(['[email protected],<[email protected]>']),
('', ''))
eq(utils.parseaddr(['[email protected];<[email protected]>']),
('', ''))
eq(utils.parseaddr(['[email protected]:<[email protected]>']),
('', ''))
eq(utils.parseaddr(['[email protected].<[email protected]>']),
('', ''))
eq(utils.parseaddr(['[email protected]"<[email protected]>']),
('', ''))
eq(utils.parseaddr(['[email protected][<[email protected]>']),
('', ''))
eq(utils.parseaddr(['[email protected]]<[email protected]>']),
('', ''))
eq(utils.parseaddr(['"Alice, [email protected]" <[email protected]>']),
('Alice, [email protected]', '[email protected]'))

def test_getaddresses_nasty(self):
eq = self.assertEqual
eq(utils.getaddresses(['"Sürname, Firstname" <[email protected]>']),
[('Sürname, Firstname', '[email protected]')])
eq(utils.getaddresses(['foo: ;']), [('', '')])
eq(utils.getaddresses(
['[]*-- =~$']),
[('', ''), ('', ''), ('', '*--')])
eq(utils.getaddresses(['[]*-- =~$']), [('', '')])
eq(utils.getaddresses(
['foo: ;', '"Jason R. Mastaler" <[email protected]>']),
[('', ''), ('Jason R. Mastaler', '[email protected]')])
eq(utils.getaddresses(
[r'Pete(A nice \) chap) <pete(his account)@silly.test(his host)>']),
[('Pete (A nice ) chap his account his host)', '[email protected]')])
eq(utils.getaddresses(
['(Empty list)(start)Undisclosed recipients :(nobody(I know))']),
[('', '')])
eq(utils.getaddresses(
['Mary <@machine.tld:[email protected]>, , jdoe@test . example']),
[('Mary', '[email protected]'), ('', ''), ('', '[email protected]')])
eq(utils.getaddresses(
['John Doe <jdoe@machine(comment). example>']),
[('John Doe (comment)', '[email protected]')])
eq(utils.getaddresses(
['"Mary Smith: Personal Account" <[email protected]>']),
[('Mary Smith: Personal Account', '[email protected]')])
eq(utils.getaddresses(
['Undisclosed recipients:;']),
[('', '')])
eq(utils.getaddresses(
[r'<[email protected]>, "Giant; \"Big\" Box" <[email protected]>']),
[('', '[email protected]'), ('Giant; "Big" Box', '[email protected]')])

def test_getaddresses_embedded_comment(self):
"""Test proper handling of a nested comment"""
Expand Down Expand Up @@ -3712,16 +3776,6 @@ def test_bytes_header_parser(self):
self.assertIsInstance(msg.get_payload(), str)
self.assertIsInstance(msg.get_payload(decode=True), bytes)

def test_header_parser_multipart_is_valid(self):
# Don't flag valid multipart emails as having defects
with openfile('msg_47.txt', encoding="utf-8") as fp:
msgdata = fp.read()

parser = email.parser.Parser(policy=email.policy.default)
parsed_msg = parser.parsestr(msgdata, headersonly=True)

self.assertEqual(parsed_msg.defects, [])

def test_bytes_parser_does_not_close_file(self):
with openfile('msg_02.txt', 'rb') as fp:
email.parser.BytesParser().parse(fp)
Expand Down