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.

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

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

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 2 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'"[^"]*,[^"]*"|\'[^\']*,[^\']\'*|\\,'
Copy link

Choose a reason for hiding this comment

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

Does this regex support multiple commas inside quotes?

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

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.

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tdwyer tdwyer Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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
108 changes: 79 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,92 @@ 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"""
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.


def test_parseaddr_parsing_errors(self):
"""Test for parsing errors from CVE-2023-27043"""
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]>']),
('', ''))

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 +3772,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