Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
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.
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(')'): | ||
| 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): | ||
| @@ -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] | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggested change
ContributorAuthor
| ||||||||||||||
| 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""" | ||||||||||||||
| @@ -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) | ||||||||||||||
There was a problem hiding this comment.
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 below0and it is exactly0at the endUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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
With the un-patched python your attack results in multiple Tuples being returned
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.
There was a problem hiding this comment.
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.comI have ideas, but I’ll look into it.