Skip to content

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

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

Merged
vstinner merged 14 commits into from
Dec 15, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
663910d
gh-102988: email parseaddr() now rejects malformed address
vstinner Oct 20, 2023
730500c
Fast path
vstinner Nov 30, 2023
1a377ec
Handle escaped quotes
vstinner Nov 30, 2023
da25ed3
Check parenthesis
vstinner Nov 30, 2023
2b199bb
Credit myself in the Changelog entry
vstinner Nov 30, 2023
9b95376
Add email.utils.supports_strict_parsing attribute
vstinner Dec 5, 2023
cf50482
Simplify getaddresses()
vstinner Dec 5, 2023
0dd5745
Update parseaddr() prototype in the doc
vstinner Dec 5, 2023
637bdb8
Apply theta682's suggestion
vstinner Dec 5, 2023
8df7013
Update Lib/test/test_email/test_email.py
vstinner Dec 12, 2023
59da8f1
Restore removed test_getaddresses_comma_in_name()
vstinner Dec 12, 2023
361b517
Move implementation details to comment
vstinner Dec 12, 2023
3105a75
Simplify _strip_quoted_realnames()
vstinner Dec 12, 2023
fc6e86b
Fix _strip_quoted_realnames()
vstinner Dec 13, 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
Prev Previous commit
Next Next commit
Check parenthesis
  • Loading branch information
@vstinner
vstinner committed Dec 5, 2023
commit da25ed3b8623eaedce731c95a2ae953df187013a
45 changes: 34 additions & 11 deletions Lib/email/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,28 +104,37 @@ def formataddr(pair, charset='utf-8'):
return address


def _iter_escaped_chars(addr):

@serhiy-storchaka serhiy-storchaka Dec 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It can use regular expressions:

for m in re.finditer(r'(?s)\\?.', addr):
    yield (m.begin(), m.group())

Perhaps functions that use _iter_escaped_chars() can also be written using efficient regular expressions to avoid handling ever char separately. For example, _strip_quoted_realnames() could use r'(?s)(?:\\.|[^"])*|(")' or something like.

But do what looks good to you, I can optimize it later.

@vstinner vstinner Dec 5, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Perhaps functions that use _iter_escaped_chars() can also be written using efficient regular expressions to avoid handling ever char separately.

This PR is based on PR #108250 which uses the following code to strip quoted real names:

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

But this regular expression doesn't handle escape quote character (\"). So I wrote _strip_quoted_realnames() to replace this invalid regex.

I'm not comfortable with regex to write a parser when the grammar is that complicated. For example, antislash can also be escaped: \\. I didn't check RFC carefully. I mostly tried to support escaped quote character (\").

Also, my priority here is more to get a fix as soon as possible, since the vulnerability was reported in March, and I have to fix the CVE at work as soon as possible.

pos = 0
escape = False
for pos, ch in enumerate(addr):
if ch == '\\' and not escape:
escape = True
else:
if escape:
yield (pos, '\\' + ch)
else:
yield (pos, ch)
escape = False
Comment thread
vstinner marked this conversation as resolved.
Outdated
if escape:
yield (pos, '\\')


def _strip_quoted_realnames(addr):
"""Strip real names between quotes."""
if '"' not in addr:
# Fast path
return addr

pos = 0
start = None
remove = []
previous = None
while pos < len(addr):
ch = addr[pos]
if ch == '"' and previous != '\\':
for pos, ch in _iter_escaped_chars(addr):
if ch == '"':
if start is None:
start = pos
else:
remove.append((start, pos))
start = None
elif ch == '\\' and previous == '\\':
previous = None
previous = ch
pos += 1

result = []
pos = 0
Expand Down Expand Up @@ -180,11 +189,25 @@ def getaddresses(fieldvalues, *, strict=True):
return result


def _check_parenthesis(addr):
# Ignore parenthesis in quoted real names.
addr = _strip_quoted_realnames(addr)

opens = 0
for pos, ch in _iter_escaped_chars(addr):
if ch == '(':
opens += 1
elif ch == ')':
opens -= 1
if opens < 0:
return False
return (opens == 0)


def _pre_parse_validation(email_header_fields):
accepted_values = []
for v in email_header_fields:
s = v.replace('\\(', '').replace('\\)', '')
if s.count('(') != s.count(')'):
if not _check_parenthesis(v):
v = "('', '')"
accepted_values.append(v)

Expand Down
25 changes: 24 additions & 1 deletion Lib/test/test_email/test_email.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3638,6 +3638,19 @@ def test_mime_classes_policy_argument(self):
m = cls(*constructor, policy=email.policy.default)
self.assertIs(m.policy, email.policy.default)

def test_iter_escaped_chars(self):
self.assertEqual(list(utils._iter_escaped_chars(r'a\\b\"c\\"d')),
[(0, 'a'),
(2, '\\\\'),
(3, 'b'),
(5, '\\"'),
(6, 'c'),
(8, '\\\\'),
(9, '"'),
(10, 'd')])
self.assertEqual(list(utils._iter_escaped_chars('a\\')),
[(0, 'a'), (1, '\\')])

def test_strip_quoted_realnames(self):
def check(addr, expected):
self.assertEqual(utils._strip_quoted_realnames(addr), expected)
Expand All @@ -3646,9 +3659,19 @@ def check(addr, expected):
'Jane Doe <jane@example.net>, John Doe <john@example.net>')
check('"Jane Doe" <jane@example.net>, "John Doe" <john@example.net>',
' <jane@example.net>, <john@example.net>')
check(r'"Jane \"Doe"." <jane@example.net>',
check(r'"Jane \"Doe\"." <jane@example.net>',
' <jane@example.net>')

def test_check_parenthesis(self):
addr = 'alice@example.net'
self.assertTrue(utils._check_parenthesis(f'{addr} (Alice)'))
self.assertFalse(utils._check_parenthesis(f'{addr} )Alice('))
self.assertFalse(utils._check_parenthesis(f'{addr} (Alice))'))
self.assertFalse(utils._check_parenthesis(f'{addr} ((Alice)'))

# Ignore real name between quotes
self.assertTrue(utils._check_parenthesis(f'")Alice((" {addr}'))


# Test the iterator/generators
class TestIterators(TestEmailBase):
Expand Down