From 4f72e60b202e49b324251fa79ac8cd5fc1d60605 Mon Sep 17 00:00:00 2001 From: Jamie Davis Date: Sat, 24 Feb 2018 20:49:30 -0500 Subject: [PATCH 1/4] Prevent low-grade poplib REDOS (CVE-2018-1060) The regex to test a mail server's timestamp is susceptible to catastrophic backtracking on long evil responses from the server. Happily, the maximum length of malicious inputs is 2K thanks to a limit introduced in the fix for CVE-2013-1752. A 2KB evil response from the mail server would result in small slowdowns (milliseconds vs. microseconds) accumulated over many apop calls. This is a potential DOS vector via accumulated slowdowns. Replace it with a similar non-vulnerable regex. The new regex is RFC compliant. The old regex was non-compliant in edge cases. Co-authored-by: Tim Peters Co-authored-by: Christian Heimes --- Lib/poplib.py | 2 +- Lib/test/test_poplib.py | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/Lib/poplib.py b/Lib/poplib.py index 6bcfa5cfeba37b..d8a62c0343276e 100644 --- a/Lib/poplib.py +++ b/Lib/poplib.py @@ -308,7 +308,7 @@ def rpop(self, user): return self._shortcmd('RPOP %s' % user) - timestamp = re.compile(br'\+OK.*(<[^>]+>)') + timestamp = re.compile(br'\+OK.[^<]*(<.*>)') def apop(self, user, password): """Authorisation diff --git a/Lib/test/test_poplib.py b/Lib/test/test_poplib.py index 4d7a4394086ae5..48cd058f73dd54 100644 --- a/Lib/test/test_poplib.py +++ b/Lib/test/test_poplib.py @@ -306,9 +306,19 @@ def test_noop(self): def test_rpop(self): self.assertOK(self.client.rpop('foo')) - def test_apop(self): + def test_apop_normal(self): self.assertOK(self.client.apop('foo', 'dummypassword')) + def test_apop_REDOS(self): + # Replace welcome with very long evil welcome. + # NB The upper bound on welcome length is currently 2048. + # At this length, evil input makes each apop call take + # on the order of milliseconds instead of microseconds. + evil_welcome = b'+OK' + (b'<' * 1000000) + with test_support.swap_attr(self.client, 'welcome', evil_welcome): + # The evil welcome is invalid, so apop should throw. + self.assertRaises(poplib.error_proto, self.client.apop, 'arthur', 'kingofbritons') + def test_top(self): expected = (b'+OK 116 bytes', [b'From: postmaster@python.org', b'Content-Type: text/plain', From 140d70ad8a7cdc702f793beddeaeba135c27c193 Mon Sep 17 00:00:00 2001 From: Jamie Davis Date: Fri, 2 Mar 2018 10:31:51 -0500 Subject: [PATCH 2/4] Prevent difflib REDOS (CVE-2018-1061) The default regex for IS_LINE_JUNK is susceptible to catastrophic backtracking. This is a potential DOS vector. Replace it with an equivalent non-vulnerable regex. Also introduce unit and REDOS tests for difflib. Co-authored-by: Tim Peters Co-authored-by: Christian Heimes --- Lib/difflib.py | 2 +- Lib/test/test_difflib.py | 22 +++++++++++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/Lib/difflib.py b/Lib/difflib.py index 82964719dc8333..043a169c28be9b 100644 --- a/Lib/difflib.py +++ b/Lib/difflib.py @@ -1083,7 +1083,7 @@ def _qformat(self, aline, bline, atags, btags): import re -def IS_LINE_JUNK(line, pat=re.compile(r"\s*#?\s*$").match): +def IS_LINE_JUNK(line, pat=re.compile(r"\s*(?:#\s*)?$").match): r""" Return 1 for ignorable line: iff `line` is blank or contains a single '#'. diff --git a/Lib/test/test_difflib.py b/Lib/test/test_difflib.py index 156b523c38c1fa..977953c6474415 100644 --- a/Lib/test/test_difflib.py +++ b/Lib/test/test_difflib.py @@ -466,13 +466,33 @@ def _assert_type_error(self, msg, generator, *args): list(generator(*args)) self.assertEqual(msg, str(ctx.exception)) +class TestJunkAPIs(unittest.TestCase): + def test_is_line_junk_true(self): + for line in ['#', ' ', ' #', '# ', ' # ', '']: + self.assertTrue(difflib.IS_LINE_JUNK(line), 'should be junk: {}'.format(line)) + + def test_is_line_junk_false(self): + for line in ['##', ' ##', '## ', 'abc ', 'abc #', 'abc # ', 'Mr. Moose is up!']: + self.assertFalse(difflib.IS_LINE_JUNK(line), 'should not be junk: {}'.format(line)) + + def test_is_line_junk_REDOS(self): + evil_input = ('\t' * 1000000) + '##' + self.assertFalse(difflib.IS_LINE_JUNK(evil_input)) + + def test_is_character_junk_true(self): + for char in [' ', '\t']: + self.assertTrue(difflib.IS_CHARACTER_JUNK(char), 'should be junk: {}'.format(char)) + + def test_is_character_junk_false(self): + for char in ['a', '#', '\n', '\f', '\r', '\v']: + self.assertFalse(difflib.IS_CHARACTER_JUNK(char), 'should not be junk: {}'.format(char)) def test_main(): difflib.HtmlDiff._default_prefix = 0 Doctests = doctest.DocTestSuite(difflib) run_unittest( TestWithAscii, TestAutojunk, TestSFpatches, TestSFbugs, - TestOutputFormat, TestBytes, Doctests) + TestOutputFormat, TestBytes, TestJunkAPIs, Doctests) if __name__ == '__main__': test_main() From 7bf95598193f14cd4c01680c7cbcbfea3fb702ac Mon Sep 17 00:00:00 2001 From: Jamie Davis Date: Fri, 2 Mar 2018 10:32:12 -0500 Subject: [PATCH 3/4] Update ACKS and News --- Misc/ACKS | 1 + .../next/Security/2018-03-02-10-24-52.bpo-32981.O_qDyj.rst | 4 ++++ 2 files changed, 5 insertions(+) create mode 100644 Misc/NEWS.d/next/Security/2018-03-02-10-24-52.bpo-32981.O_qDyj.rst diff --git a/Misc/ACKS b/Misc/ACKS index dee022f2ff0a64..f41a48de7918be 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -356,6 +356,7 @@ Jonathan Dasteel Pierre-Yves David A. Jesse Jiryu Davis Jake Davis +Jamie (James C.) Davis Ratnadeep Debnath Merlijn van Deen John DeGood diff --git a/Misc/NEWS.d/next/Security/2018-03-02-10-24-52.bpo-32981.O_qDyj.rst b/Misc/NEWS.d/next/Security/2018-03-02-10-24-52.bpo-32981.O_qDyj.rst new file mode 100644 index 00000000000000..9ebabb44f91e7c --- /dev/null +++ b/Misc/NEWS.d/next/Security/2018-03-02-10-24-52.bpo-32981.O_qDyj.rst @@ -0,0 +1,4 @@ +Regexes in difflib and poplib were vulnerable to catastrophic backtracking. +These regexes formed potential DOS vectors (REDOS). They have been +refactored. This resolves CVE-2018-1060 and CVE-2018-1061. +Patch by Jamie Davis. From ef83b0c4ce8ba21d5b46f966c5a6f261eea0daca Mon Sep 17 00:00:00 2001 From: Jamie Davis Date: Fri, 2 Mar 2018 13:04:39 -0500 Subject: [PATCH 4/4] Address review comments --- Lib/test/test_difflib.py | 10 +++++----- Lib/test/test_poplib.py | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_difflib.py b/Lib/test/test_difflib.py index 977953c6474415..aaefe6db02918f 100644 --- a/Lib/test/test_difflib.py +++ b/Lib/test/test_difflib.py @@ -469,11 +469,11 @@ def _assert_type_error(self, msg, generator, *args): class TestJunkAPIs(unittest.TestCase): def test_is_line_junk_true(self): for line in ['#', ' ', ' #', '# ', ' # ', '']: - self.assertTrue(difflib.IS_LINE_JUNK(line), 'should be junk: {}'.format(line)) + self.assertTrue(difflib.IS_LINE_JUNK(line), repr(line)) def test_is_line_junk_false(self): - for line in ['##', ' ##', '## ', 'abc ', 'abc #', 'abc # ', 'Mr. Moose is up!']: - self.assertFalse(difflib.IS_LINE_JUNK(line), 'should not be junk: {}'.format(line)) + for line in ['##', ' ##', '## ', 'abc ', 'abc #', 'Mr. Moose is up!']: + self.assertFalse(difflib.IS_LINE_JUNK(line), repr(line)) def test_is_line_junk_REDOS(self): evil_input = ('\t' * 1000000) + '##' @@ -481,11 +481,11 @@ def test_is_line_junk_REDOS(self): def test_is_character_junk_true(self): for char in [' ', '\t']: - self.assertTrue(difflib.IS_CHARACTER_JUNK(char), 'should be junk: {}'.format(char)) + self.assertTrue(difflib.IS_CHARACTER_JUNK(char), repr(char)) def test_is_character_junk_false(self): for char in ['a', '#', '\n', '\f', '\r', '\v']: - self.assertFalse(difflib.IS_CHARACTER_JUNK(char), 'should not be junk: {}'.format(char)) + self.assertFalse(difflib.IS_CHARACTER_JUNK(char), repr(char)) def test_main(): difflib.HtmlDiff._default_prefix = 0 diff --git a/Lib/test/test_poplib.py b/Lib/test/test_poplib.py index 48cd058f73dd54..a52e6baebc2171 100644 --- a/Lib/test/test_poplib.py +++ b/Lib/test/test_poplib.py @@ -317,7 +317,7 @@ def test_apop_REDOS(self): evil_welcome = b'+OK' + (b'<' * 1000000) with test_support.swap_attr(self.client, 'welcome', evil_welcome): # The evil welcome is invalid, so apop should throw. - self.assertRaises(poplib.error_proto, self.client.apop, 'arthur', 'kingofbritons') + self.assertRaises(poplib.error_proto, self.client.apop, 'a', 'kb') def test_top(self): expected = (b'+OK 116 bytes',