From 38d0a152a92b29cf2d1300463edb757bfa0f133d Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Sun, 15 Mar 2020 01:24:16 +0000 Subject: [PATCH 1/7] validate address parts --- Lib/email/headerregistry.py | 10 ++++++++++ Lib/test/test_email/test_headerregistry.py | 12 ++++++++++++ 2 files changed, 22 insertions(+) diff --git a/Lib/email/headerregistry.py b/Lib/email/headerregistry.py index cc1d1912918149..cee8f6ac0262ae 100644 --- a/Lib/email/headerregistry.py +++ b/Lib/email/headerregistry.py @@ -48,6 +48,12 @@ def __init__(self, display_name='', username='', domain='', addr_spec=None): raise a_s.all_defects[0] username = a_s.local_part domain = a_s.domain + else: + self._validate_part(username) + self._validate_part(domain) + + self._validate_part(display_name) + self._display_name = display_name self._username = username self._domain = domain @@ -99,6 +105,10 @@ def __eq__(self, other): self.username == other.username and self.domain == other.domain) + def _validate_part(self, value): + """Parts cannot contain CRLF for security reasons.""" + if '\r\n' in value: + raise ValueError("invalid address; address parts cannot contain CRLF") class Group: diff --git a/Lib/test/test_email/test_headerregistry.py b/Lib/test/test_email/test_headerregistry.py index 38f7ddbf06d5cd..0244157bed857a 100644 --- a/Lib/test/test_email/test_headerregistry.py +++ b/Lib/test/test_email/test_headerregistry.py @@ -1437,6 +1437,18 @@ def test_il8n(self): # with self.assertRaises(ValueError): # Address('foo', 'wők', 'example.com') + def test_crlf_in_display_name_raises(self): + with self.assertRaisesRegex(ValueError, "invalid address"): + Address(display_name='example.com\r\n') + + def test_crlf_in_username_raises(self): + with self.assertRaisesRegex(ValueError, "invalid address"): + Address(username='hello\r\n') + + def test_crlf_in_domain_raises(self): + with self.assertRaisesRegex(ValueError, "invalid address"): + Address(domain='example.com\r\n') + def test_non_ascii_username_in_addr_spec_raises(self): with self.assertRaises(ValueError): Address('foo', addr_spec='wők@example.com') From d8424e57218af49418ab33cbde54cc660f74bceb Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Sun, 15 Mar 2020 01:28:40 +0000 Subject: [PATCH 2/7] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Security/2020-03-15-01-28-36.bpo-39073.6Szd3i.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Security/2020-03-15-01-28-36.bpo-39073.6Szd3i.rst diff --git a/Misc/NEWS.d/next/Security/2020-03-15-01-28-36.bpo-39073.6Szd3i.rst b/Misc/NEWS.d/next/Security/2020-03-15-01-28-36.bpo-39073.6Szd3i.rst new file mode 100644 index 00000000000000..05f913c53b6428 --- /dev/null +++ b/Misc/NEWS.d/next/Security/2020-03-15-01-28-36.bpo-39073.6Szd3i.rst @@ -0,0 +1 @@ +Validate email.headerregistry.Address to disallow CRLF in address parts (username, domain, display_name). \ No newline at end of file From de6ef8cb920400d3155c12fcb9ce221b3fd10736 Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Mon, 16 Mar 2020 13:29:44 +0000 Subject: [PATCH 3/7] do validation at once, on \r and \n, refactor tests --- Lib/email/headerregistry.py | 15 +++++-------- Lib/test/test_email/test_headerregistry.py | 26 +++++++++++++--------- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/Lib/email/headerregistry.py b/Lib/email/headerregistry.py index cee8f6ac0262ae..ce4799d1ba8555 100644 --- a/Lib/email/headerregistry.py +++ b/Lib/email/headerregistry.py @@ -31,6 +31,11 @@ def __init__(self, display_name='', username='', domain='', addr_spec=None): without any Content Transfer Encoding. """ + + inputs = ''.join(filter(None, (display_name, username, domain, addr_spec))) + if '\r' in inputs or '\n' in inputs: + raise ValueError("invalid inputs; address parts cannot contain CR / LF") + # This clause with its potential 'raise' may only happen when an # application program creates an Address object using an addr_spec # keyword. The email library code itself must always supply username @@ -48,11 +53,6 @@ def __init__(self, display_name='', username='', domain='', addr_spec=None): raise a_s.all_defects[0] username = a_s.local_part domain = a_s.domain - else: - self._validate_part(username) - self._validate_part(domain) - - self._validate_part(display_name) self._display_name = display_name self._username = username @@ -105,11 +105,6 @@ def __eq__(self, other): self.username == other.username and self.domain == other.domain) - def _validate_part(self, value): - """Parts cannot contain CRLF for security reasons.""" - if '\r\n' in value: - raise ValueError("invalid address; address parts cannot contain CRLF") - class Group: def __init__(self, display_name=None, addresses=None): diff --git a/Lib/test/test_email/test_headerregistry.py b/Lib/test/test_email/test_headerregistry.py index 0244157bed857a..09390f71a85ada 100644 --- a/Lib/test/test_email/test_headerregistry.py +++ b/Lib/test/test_email/test_headerregistry.py @@ -1437,17 +1437,21 @@ def test_il8n(self): # with self.assertRaises(ValueError): # Address('foo', 'wők', 'example.com') - def test_crlf_in_display_name_raises(self): - with self.assertRaisesRegex(ValueError, "invalid address"): - Address(display_name='example.com\r\n') - - def test_crlf_in_username_raises(self): - with self.assertRaisesRegex(ValueError, "invalid address"): - Address(username='hello\r\n') - - def test_crlf_in_domain_raises(self): - with self.assertRaisesRegex(ValueError, "invalid address"): - Address(domain='example.com\r\n') + def test_crlf_in_constructor_args_raises(self): + cases = ( + dict(display_name='example.com\r'), + dict(display_name='example.com\n'), + dict(display_name='example.com\r\n'), + dict(username='wok\r'), + dict(username='wok\n'), + dict(username='wok\r\n'), + dict(addr_spec='wok@example.com\r'), + dict(addr_spec='wok@example.com\n'), + dict(addr_spec='wok@example.com\r\n') + ) + for kwargs in cases: + with self.subTest(kwargs=kwargs), self.assertRaisesRegex(ValueError, "invalid inputs"): + Address(**kwargs) def test_non_ascii_username_in_addr_spec_raises(self): with self.assertRaises(ValueError): From 6cac3a0dd7b7658d3fa9f7acf26f154979353a0f Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Mon, 16 Mar 2020 13:35:12 +0000 Subject: [PATCH 4/7] rm extra spacing --- Lib/email/headerregistry.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/email/headerregistry.py b/Lib/email/headerregistry.py index ce4799d1ba8555..969872a21436e4 100644 --- a/Lib/email/headerregistry.py +++ b/Lib/email/headerregistry.py @@ -53,7 +53,6 @@ def __init__(self, display_name='', username='', domain='', addr_spec=None): raise a_s.all_defects[0] username = a_s.local_part domain = a_s.domain - self._display_name = display_name self._username = username self._domain = domain @@ -105,6 +104,7 @@ def __eq__(self, other): self.username == other.username and self.domain == other.domain) + class Group: def __init__(self, display_name=None, addresses=None): From d10d3839fee079b81820252e2a4524ae553550f0 Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Mon, 16 Mar 2020 14:49:14 +0000 Subject: [PATCH 5/7] Add display_name test --- Lib/test/test_email/test_headerregistry.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_email/test_headerregistry.py b/Lib/test/test_email/test_headerregistry.py index 09390f71a85ada..07715a9ebd2f64 100644 --- a/Lib/test/test_email/test_headerregistry.py +++ b/Lib/test/test_email/test_headerregistry.py @@ -1439,9 +1439,12 @@ def test_il8n(self): def test_crlf_in_constructor_args_raises(self): cases = ( - dict(display_name='example.com\r'), - dict(display_name='example.com\n'), - dict(display_name='example.com\r\n'), + dict(display_name='foo\r'), + dict(display_name='foo\n'), + dict(display_name='foo\r\n'), + dict(domain='example.com\r'), + dict(domain='example.com\n'), + dict(domain='example.com\r\n'), dict(username='wok\r'), dict(username='wok\n'), dict(username='wok\r\n'), From 81a0231ec1125744179ed3f087aa072da471eead Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Mon, 16 Mar 2020 14:50:17 +0000 Subject: [PATCH 6/7] update message --- Lib/email/headerregistry.py | 2 +- Lib/test/test_email/test_headerregistry.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/email/headerregistry.py b/Lib/email/headerregistry.py index 969872a21436e4..5d84fc0d82d0b0 100644 --- a/Lib/email/headerregistry.py +++ b/Lib/email/headerregistry.py @@ -34,7 +34,7 @@ def __init__(self, display_name='', username='', domain='', addr_spec=None): inputs = ''.join(filter(None, (display_name, username, domain, addr_spec))) if '\r' in inputs or '\n' in inputs: - raise ValueError("invalid inputs; address parts cannot contain CR / LF") + raise ValueError("invalid arguments; address parts cannot contain CR or LF") # This clause with its potential 'raise' may only happen when an # application program creates an Address object using an addr_spec diff --git a/Lib/test/test_email/test_headerregistry.py b/Lib/test/test_email/test_headerregistry.py index 07715a9ebd2f64..82e121350ffbf5 100644 --- a/Lib/test/test_email/test_headerregistry.py +++ b/Lib/test/test_email/test_headerregistry.py @@ -1453,7 +1453,7 @@ def test_crlf_in_constructor_args_raises(self): dict(addr_spec='wok@example.com\r\n') ) for kwargs in cases: - with self.subTest(kwargs=kwargs), self.assertRaisesRegex(ValueError, "invalid inputs"): + with self.subTest(kwargs=kwargs), self.assertRaisesRegex(ValueError, "invalid arguments"): Address(**kwargs) def test_non_ascii_username_in_addr_spec_raises(self): From 526b7509ee50cf539b7a01ee15a9f008b71dd94d Mon Sep 17 00:00:00 2001 From: Ashwin Ramaswami Date: Mon, 16 Mar 2020 14:33:04 -0400 Subject: [PATCH 7/7] Update Misc/NEWS.d/next/Security/2020-03-15-01-28-36.bpo-39073.6Szd3i.rst --- .../next/Security/2020-03-15-01-28-36.bpo-39073.6Szd3i.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Security/2020-03-15-01-28-36.bpo-39073.6Szd3i.rst b/Misc/NEWS.d/next/Security/2020-03-15-01-28-36.bpo-39073.6Szd3i.rst index 05f913c53b6428..6c9447b897bf63 100644 --- a/Misc/NEWS.d/next/Security/2020-03-15-01-28-36.bpo-39073.6Szd3i.rst +++ b/Misc/NEWS.d/next/Security/2020-03-15-01-28-36.bpo-39073.6Szd3i.rst @@ -1 +1 @@ -Validate email.headerregistry.Address to disallow CRLF in address parts (username, domain, display_name). \ No newline at end of file +Disallow CR or LF in email.headerregistry.Address arguments to guard against header injection attacks.