From 6322049d1285f652e5a33144271200c1fe0c3746 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 24 May 2024 21:00:55 +0300 Subject: [PATCH 1/5] gh-119511: Fix OOM vulnerability in imaplib The IMAP4 client could consume an arbitrary amount of memory when trying to connent to a malicious server, because it read a "literal" data with a single read(size) call, and BufferedReader.read() allocates the bytes object of the specified size before reading. Now the IMAP4 client reads data by chunks, therefore the amount of used memory is limited by the amount of the data actually been sent by the server. --- Lib/imaplib.py | 10 ++++++++++ Lib/test/test_imaplib.py | 12 ++++++++++++ .../2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst | 2 ++ 3 files changed, 24 insertions(+) create mode 100644 Misc/NEWS.d/next/Security/2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst diff --git a/Lib/imaplib.py b/Lib/imaplib.py index e576c29e67dc0a..f7e901821eda5d 100644 --- a/Lib/imaplib.py +++ b/Lib/imaplib.py @@ -52,6 +52,9 @@ # search command can be quite large, so we now use 1M. _MAXLINE = 1000000 +# Data larger than this will be read in chunks, to prevent extreme +# overallocation. +_SAFE_BUF_SIZE = 1 << 20 # Commands @@ -315,6 +318,13 @@ def open(self, host='', port=IMAP4_PORT, timeout=None): def read(self, size): """Read 'size' bytes from remote.""" + cursize = min(size, _SAFE_BUF_SIZE) + data = self.file.read(cursize) + while cursize < size and len(data) == cursize: + delta = min(cursize, size - cursize) + data += self.file.read(delta) + cursize += delta + return data return self.file.read(size) diff --git a/Lib/test/test_imaplib.py b/Lib/test/test_imaplib.py index 79bf7dbdbb81a0..e9ffeae990e7fb 100644 --- a/Lib/test/test_imaplib.py +++ b/Lib/test/test_imaplib.py @@ -907,6 +907,18 @@ def handle(self): self.assertRaises(imaplib.IMAP4.error, self.imap_class, *server.server_address) + def test_truncated_large_literal(self): + class BadHandler(SimpleIMAPHandler): + def handle(self): + self._send_textline('* OK {%d}' % size) + self._send_textline('IMAP4rev1') + + for w in range(15, 64): + size = 1 << w + with self.reaped_server(BadHandler) as server: + self.assertRaises(imaplib.IMAP4.abort, + self.imap_class, *server.server_address) + @threading_helper.reap_threads def test_simple_with_statement(self): # simplest call diff --git a/Misc/NEWS.d/next/Security/2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst b/Misc/NEWS.d/next/Security/2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst new file mode 100644 index 00000000000000..d610a6938ae0c7 --- /dev/null +++ b/Misc/NEWS.d/next/Security/2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst @@ -0,0 +1,2 @@ +Fix a vulnerability in the :mod:`imaplib` module, when connecting to a +malicious server could cause an arbitrary amount of memory to be consumed. From 284a553625c5650aec4df825305bb3a0e2aab9f4 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Thu, 30 May 2024 13:06:46 +0300 Subject: [PATCH 2/5] Update Lib/imaplib.py --- Lib/imaplib.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/imaplib.py b/Lib/imaplib.py index f7e901821eda5d..db708580a0abf6 100644 --- a/Lib/imaplib.py +++ b/Lib/imaplib.py @@ -325,7 +325,6 @@ def read(self, size): data += self.file.read(delta) cursize += delta return data - return self.file.read(size) def readline(self): From d7abc31bd554500342906685bde24e03966e561f Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 20 Jan 2025 12:09:42 -0800 Subject: [PATCH 3/5] modernize the test, expand the NEWS entry. --- Lib/test/test_imaplib.py | 8 ++++---- .../2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst | 9 +++++++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_imaplib.py b/Lib/test/test_imaplib.py index e9ffeae990e7fb..a11920144e01fa 100644 --- a/Lib/test/test_imaplib.py +++ b/Lib/test/test_imaplib.py @@ -913,11 +913,11 @@ def handle(self): self._send_textline('* OK {%d}' % size) self._send_textline('IMAP4rev1') - for w in range(15, 64): - size = 1 << w + for exponent in range(15, 64): + size = 1 << exponent with self.reaped_server(BadHandler) as server: - self.assertRaises(imaplib.IMAP4.abort, - self.imap_class, *server.server_address) + with self.assertRaises(imaplib.IMAP4.abort): + self.imap_class(*server.server_address) @threading_helper.reap_threads def test_simple_with_statement(self): diff --git a/Misc/NEWS.d/next/Security/2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst b/Misc/NEWS.d/next/Security/2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst index d610a6938ae0c7..101eafaab789cb 100644 --- a/Misc/NEWS.d/next/Security/2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst +++ b/Misc/NEWS.d/next/Security/2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst @@ -1,2 +1,7 @@ -Fix a vulnerability in the :mod:`imaplib` module, when connecting to a -malicious server could cause an arbitrary amount of memory to be consumed. +Fix a potential denial of service vulnerability in the :mod:`imaplib` module. +When connecting to a malicious server, it could cause an arbitrary amount of +memory to be allocated. On many systems this is harmless as unused virtual +memory is only a mapping, but if this hit a virtual address size limit it could +lead to a :exc:`MemoryError` or other process crash. On unusual systems or +builds where all allocated memory is touched and backed by actual ram or +storage it could've consumed resources doing so until similarly crashing. From d71a7eee60cb10c26e1ad3f24b8d10b826235dfe Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 20 Jan 2025 12:17:29 -0800 Subject: [PATCH 4/5] use a subTest for sizes --- Lib/test/test_imaplib.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_imaplib.py b/Lib/test/test_imaplib.py index e77be57f0824ef..2fbf83b264d5b4 100644 --- a/Lib/test/test_imaplib.py +++ b/Lib/test/test_imaplib.py @@ -902,6 +902,7 @@ def handle(self): self.imap_class, *server.server_address) def test_truncated_large_literal(self): + size = 0 class BadHandler(SimpleIMAPHandler): def handle(self): self._send_textline('* OK {%d}' % size) @@ -909,9 +910,10 @@ def handle(self): for exponent in range(15, 64): size = 1 << exponent - with self.reaped_server(BadHandler) as server: - with self.assertRaises(imaplib.IMAP4.abort): - self.imap_class(*server.server_address) + with self.subTest(f"size=2e{size}"): + with self.reaped_server(BadHandler) as server: + with self.assertRaises(imaplib.IMAP4.abort): + self.imap_class(*server.server_address) @threading_helper.reap_threads def test_simple_with_statement(self): From f6a41df0cb2dfaf06b7a54e944568bfca02a7b5b Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 20 Jan 2025 12:19:43 -0800 Subject: [PATCH 5/5] wording tweak. --- .../2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Misc/NEWS.d/next/Security/2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst b/Misc/NEWS.d/next/Security/2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst index 101eafaab789cb..f7b4031120e643 100644 --- a/Misc/NEWS.d/next/Security/2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst +++ b/Misc/NEWS.d/next/Security/2024-05-24-21-00-52.gh-issue-119511.jKrXQ8.rst @@ -1,7 +1,7 @@ -Fix a potential denial of service vulnerability in the :mod:`imaplib` module. -When connecting to a malicious server, it could cause an arbitrary amount of -memory to be allocated. On many systems this is harmless as unused virtual -memory is only a mapping, but if this hit a virtual address size limit it could -lead to a :exc:`MemoryError` or other process crash. On unusual systems or -builds where all allocated memory is touched and backed by actual ram or -storage it could've consumed resources doing so until similarly crashing. +Fix a potential denial of service in the :mod:`imaplib` module. When connecting +to a malicious server, it could cause an arbitrary amount of memory to be +allocated. On many systems this is harmless as unused virtual memory is only a +mapping, but if this hit a virtual address size limit it could lead to a +:exc:`MemoryError` or other process crash. On unusual systems or builds where +all allocated memory is touched and backed by actual ram or storage it could've +consumed resources doing so until similarly crashing.