From b2b1ddab9deb93411920f005018fc36f0bf86a60 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 26 Jun 2018 19:35:58 +0300 Subject: [PATCH 1/2] bpo-33871: Fix os.sendfile(), os.writev(), os.readv(), etc. * Fix integer overflow in os.readv(), os.writev(), os.preadv() and os.pwritev() and in os.sendfile() with headers or trailers arguments (on BSD-based OSes and MacOS). * Fix sending the part of the file in os.sendfile() on MacOS. Using the trailers argument could cause sending more bytes from the input file than was specified. Thanks Ned Deily for testing on 32-bit MacOS. --- Lib/test/test_os.py | 50 +++++++++++------ Lib/test/test_posix.py | 53 +++++++++++++++++++ .../2018-06-26-19-03-56.bpo-33871.XhlrGU.rst | 4 ++ .../2018-06-26-19-35-33.bpo-33871.S4HR9n.rst | 3 ++ Modules/posixmodule.c | 37 +++++++------ 5 files changed, 115 insertions(+), 32 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2018-06-26-19-03-56.bpo-33871.XhlrGU.rst create mode 100644 Misc/NEWS.d/next/Security/2018-06-26-19-35-33.bpo-33871.S4HR9n.rst diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index e509188243f697..a140ae0d29eae1 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2532,12 +2532,14 @@ class Handler(asynchat.async_chat): def __init__(self, conn): asynchat.async_chat.__init__(self, conn) self.in_buffer = [] + self.accumulate = True self.closed = False self.push(b"220 ready\r\n") def handle_read(self): data = self.recv(4096) - self.in_buffer.append(data) + if self.accumulate: + self.in_buffer.append(data) def get_data(self): return b''.join(self.in_buffer) @@ -2618,6 +2620,8 @@ class TestSendfile(unittest.TestCase): not sys.platform.startswith("sunos") requires_headers_trailers = unittest.skipUnless(SUPPORT_HEADERS_TRAILERS, 'requires headers and trailers support') + requires_32b = unittest.skipUnless(sys.maxsize < 2**32, + 'test is only meaningful on 32-bit builds') @classmethod def setUpClass(cls): @@ -2648,17 +2652,13 @@ def tearDown(self): self.server.stop() self.server = None - def sendfile_wrapper(self, sock, file, offset, nbytes, headers=[], trailers=[]): + def sendfile_wrapper(self, *args, **kwargs): """A higher level wrapper representing how an application is supposed to use sendfile(). """ - while 1: + while True: try: - if self.SUPPORT_HEADERS_TRAILERS: - return os.sendfile(sock, file, offset, nbytes, headers, - trailers) - else: - return os.sendfile(sock, file, offset, nbytes) + return os.sendfile(*args, **kwargs) except OSError as err: if err.errno == errno.ECONNRESET: # disconnected @@ -2749,20 +2749,22 @@ def test_keywords(self): @requires_headers_trailers def test_headers(self): total_sent = 0 + expected_data = b"x" * 512 + b"y" * 256 + self.DATA[:-1] sent = os.sendfile(self.sockno, self.fileno, 0, 4096, - headers=[b"x" * 512]) + headers=[b"x" * 512, b"y" * 256]) + self.assertLessEqual(sent, 512 + 256 + 4096) total_sent += sent offset = 4096 - nbytes = 4096 - while 1: + while total_sent < len(expected_data): + nbytes = min(len(expected_data) - total_sent, 4096) sent = self.sendfile_wrapper(self.sockno, self.fileno, offset, nbytes) if sent == 0: break + self.assertLessEqual(sent, nbytes) total_sent += sent offset += sent - expected_data = b"x" * 512 + self.DATA self.assertEqual(total_sent, len(expected_data)) self.client.close() self.server.wait() @@ -2778,12 +2780,30 @@ def test_trailers(self): create_file(TESTFN2, file_data) with open(TESTFN2, 'rb') as f: - os.sendfile(self.sockno, f.fileno(), 0, len(file_data), - trailers=[b"1234"]) + os.sendfile(self.sockno, f.fileno(), 0, 5, + trailers=[b"123456", b"789"]) self.client.close() self.server.wait() data = self.server.handler_instance.get_data() - self.assertEqual(data, b"abcdef1234") + self.assertEqual(data, b"abcde123456789") + + @requires_headers_trailers + @requires_32b + def test_headers_overflow_32bits(self): + self.server.handler_instance.accumulate = False + with self.assertRaises(OSError) as cm: + os.sendfile(self.sockno, self.fileno, 0, 0, + headers=[b"x" * 2**16] * 2**15) + self.assertEqual(cm.exception.errno, errno.EINVAL) + + @requires_headers_trailers + @requires_32b + def test_trailers_overflow_32bits(self): + self.server.handler_instance.accumulate = False + with self.assertRaises(OSError) as cm: + os.sendfile(self.sockno, self.fileno, 0, 0, + trailers=[b"x" * 2**16] * 2**15) + self.assertEqual(cm.exception.errno, errno.EINVAL) @requires_headers_trailers @unittest.skipUnless(hasattr(os, 'SF_NODISKIO'), diff --git a/Lib/test/test_posix.py b/Lib/test/test_posix.py index 4f6abc6f7d6fd9..e569f7ae72daaf 100644 --- a/Lib/test/test_posix.py +++ b/Lib/test/test_posix.py @@ -20,6 +20,9 @@ _DUMMY_SYMLINK = os.path.join(tempfile.gettempdir(), support.TESTFN + '-dummy-symlink') +requires_32b = unittest.skipUnless(sys.maxsize < 2**32, + 'test is only meaningful on 32-bit builds') + class PosixTester(unittest.TestCase): def setUp(self): @@ -284,6 +287,7 @@ def test_preadv(self): finally: os.close(fd) + @unittest.skipUnless(hasattr(posix, 'preadv'), "test needs posix.preadv()") @unittest.skipUnless(hasattr(posix, 'RWF_HIPRI'), "test needs posix.RWF_HIPRI") def test_preadv_flags(self): fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT) @@ -295,6 +299,19 @@ def test_preadv_flags(self): finally: os.close(fd) + @unittest.skipUnless(hasattr(posix, 'preadv'), "test needs posix.preadv()") + @requires_32b + def test_preadv_overflow_32bits(self): + fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT) + try: + buf = [bytearray(2**16)] * 2**15 + with self.assertRaises(OSError) as cm: + os.preadv(fd, buf, 0) + self.assertEqual(cm.exception.errno, errno.EINVAL) + self.assertEqual(bytes(buf[0]), b'\0'* 2**16) + finally: + os.close(fd) + @unittest.skipUnless(hasattr(posix, 'pwrite'), "test needs posix.pwrite()") def test_pwrite(self): fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT) @@ -320,6 +337,7 @@ def test_pwritev(self): finally: os.close(fd) + @unittest.skipUnless(hasattr(posix, 'pwritev'), "test needs posix.pwritev()") @unittest.skipUnless(hasattr(posix, 'os.RWF_SYNC'), "test needs os.RWF_SYNC") def test_pwritev_flags(self): fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT) @@ -334,6 +352,17 @@ def test_pwritev_flags(self): finally: os.close(fd) + @unittest.skipUnless(hasattr(posix, 'pwritev'), "test needs posix.pwritev()") + @requires_32b + def test_pwritev_overflow_32bits(self): + fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT) + try: + with self.assertRaises(OSError) as cm: + os.pwritev(fd, [b"x" * 2**16] * 2**15, 0) + self.assertEqual(cm.exception.errno, errno.EINVAL) + finally: + os.close(fd) + @unittest.skipUnless(hasattr(posix, 'posix_fallocate'), "test needs posix.posix_fallocate()") def test_posix_fallocate(self): @@ -435,6 +464,17 @@ def test_writev(self): finally: os.close(fd) + @unittest.skipUnless(hasattr(posix, 'writev'), "test needs posix.writev()") + @requires_32b + def test_writev_overflow_32bits(self): + fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT) + try: + with self.assertRaises(OSError) as cm: + os.writev(fd, [b"x" * 2**16] * 2**15) + self.assertEqual(cm.exception.errno, errno.EINVAL) + finally: + os.close(fd) + @unittest.skipUnless(hasattr(posix, 'readv'), "test needs posix.readv()") def test_readv(self): fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT) @@ -457,6 +497,19 @@ def test_readv(self): finally: os.close(fd) + @unittest.skipUnless(hasattr(posix, 'readv'), "test needs posix.readv()") + @requires_32b + def test_readv_overflow_32bits(self): + fd = os.open(support.TESTFN, os.O_RDWR | os.O_CREAT) + try: + buf = [bytearray(2**16)] * 2**15 + with self.assertRaises(OSError) as cm: + os.readv(fd, buf) + self.assertEqual(cm.exception.errno, errno.EINVAL) + self.assertEqual(bytes(buf[0]), b'\0'* 2**16) + finally: + os.close(fd) + @unittest.skipUnless(hasattr(posix, 'dup'), 'test needs posix.dup()') def test_dup(self): diff --git a/Misc/NEWS.d/next/Library/2018-06-26-19-03-56.bpo-33871.XhlrGU.rst b/Misc/NEWS.d/next/Library/2018-06-26-19-03-56.bpo-33871.XhlrGU.rst new file mode 100644 index 00000000000000..55a5284599d120 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-06-26-19-03-56.bpo-33871.XhlrGU.rst @@ -0,0 +1,4 @@ +Fixed integer overflow in :func:`os.readv`, :func:`os.writev`, +:func:`os.preadv` and :func:`os.pwritev` and in :func:`os.sendfile` with +*headers* or *trailers* arguments (on BSD-based OSes and MacOS). +Thanks Ned Deily for testing on 32-bit MacOS. diff --git a/Misc/NEWS.d/next/Security/2018-06-26-19-35-33.bpo-33871.S4HR9n.rst b/Misc/NEWS.d/next/Security/2018-06-26-19-35-33.bpo-33871.S4HR9n.rst new file mode 100644 index 00000000000000..296a449a2655e7 --- /dev/null +++ b/Misc/NEWS.d/next/Security/2018-06-26-19-35-33.bpo-33871.S4HR9n.rst @@ -0,0 +1,3 @@ +Fixed sending the part of the file in :func:`os.sendfile` on MacOS. Using +the *trailers* argument could cause sending more bytes from the input file +than was specified. Thanks Ned Deily for testing on 32-bit MacOS. diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 435e5f61271cc4..6104c0f74dee57 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -8321,12 +8321,13 @@ os_read_impl(PyObject *module, int fd, Py_ssize_t length) } #if (defined(HAVE_SENDFILE) && (defined(__FreeBSD__) || defined(__DragonFly__) \ - || defined(__APPLE__))) || defined(HAVE_READV) || defined(HAVE_WRITEV) -static Py_ssize_t + || defined(__APPLE__))) \ + || defined(HAVE_READV) || defined(HAVE_PREADV) || defined (HAVE_PREADV2) \ + || defined(HAVE_WRITEV) || defined(HAVE_PWRITEV) || defined (HAVE_PWRITEV2) +static int iov_setup(struct iovec **iov, Py_buffer **buf, PyObject *seq, Py_ssize_t cnt, int type) { Py_ssize_t i, j; - Py_ssize_t blen, total = 0; *iov = PyMem_New(struct iovec, cnt); if (*iov == NULL) { @@ -8351,11 +8352,9 @@ iov_setup(struct iovec **iov, Py_buffer **buf, PyObject *seq, Py_ssize_t cnt, in } Py_DECREF(item); (*iov)[i].iov_base = (*buf)[i].buf; - blen = (*buf)[i].len; - (*iov)[i].iov_len = blen; - total += blen; + (*iov)[i].iov_len = (*buf)[i].len; } - return total; + return 0; fail: PyMem_Del(*iov); @@ -8652,12 +8651,20 @@ posix_sendfile(PyObject *self, PyObject *args, PyObject *kwdict) } if (i > 0) { sf.hdr_cnt = (int)i; - i = iov_setup(&(sf.headers), &hbuf, - headers, sf.hdr_cnt, PyBUF_SIMPLE); - if (i < 0) + if (iov_setup(&(sf.headers), &hbuf, + headers, sf.hdr_cnt, PyBUF_SIMPLE) < 0) return NULL; #ifdef __APPLE__ - sbytes += i; + for (i = 0; i < sf.hdr_cnt; i++) { + Py_ssize_t blen = sf.headers[i].iov_len; +# define OFF_T_MAX 0x7fffffffffffffff + if (sbytes >= OFF_T_MAX - blen) { + PyErr_SetString(PyExc_OverflowError, + "sendfile() header is too large"); + return NULL; + } + sbytes += blen; + } #endif } } @@ -8678,13 +8685,9 @@ posix_sendfile(PyObject *self, PyObject *args, PyObject *kwdict) } if (i > 0) { sf.trl_cnt = (int)i; - i = iov_setup(&(sf.trailers), &tbuf, - trailers, sf.trl_cnt, PyBUF_SIMPLE); - if (i < 0) + if (iov_setup(&(sf.trailers), &tbuf, + trailers, sf.trl_cnt, PyBUF_SIMPLE) < 0) return NULL; -#ifdef __APPLE__ - sbytes += i; -#endif } } } From b208466b8bdb9641ad3c078614d0a1f3959df366 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 26 Jun 2018 23:13:44 +0300 Subject: [PATCH 2/2] Update news entries. --- .../next/Library/2018-06-26-19-03-56.bpo-33871.XhlrGU.rst | 3 +-- .../next/Security/2018-06-26-19-35-33.bpo-33871.S4HR9n.rst | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2018-06-26-19-03-56.bpo-33871.XhlrGU.rst b/Misc/NEWS.d/next/Library/2018-06-26-19-03-56.bpo-33871.XhlrGU.rst index 55a5284599d120..9fd15356c72db4 100644 --- a/Misc/NEWS.d/next/Library/2018-06-26-19-03-56.bpo-33871.XhlrGU.rst +++ b/Misc/NEWS.d/next/Library/2018-06-26-19-03-56.bpo-33871.XhlrGU.rst @@ -1,4 +1,3 @@ Fixed integer overflow in :func:`os.readv`, :func:`os.writev`, :func:`os.preadv` and :func:`os.pwritev` and in :func:`os.sendfile` with -*headers* or *trailers* arguments (on BSD-based OSes and MacOS). -Thanks Ned Deily for testing on 32-bit MacOS. +*headers* or *trailers* arguments (on BSD-based OSes and macOS). diff --git a/Misc/NEWS.d/next/Security/2018-06-26-19-35-33.bpo-33871.S4HR9n.rst b/Misc/NEWS.d/next/Security/2018-06-26-19-35-33.bpo-33871.S4HR9n.rst index 296a449a2655e7..547342c2e9dd50 100644 --- a/Misc/NEWS.d/next/Security/2018-06-26-19-35-33.bpo-33871.S4HR9n.rst +++ b/Misc/NEWS.d/next/Security/2018-06-26-19-35-33.bpo-33871.S4HR9n.rst @@ -1,3 +1,3 @@ -Fixed sending the part of the file in :func:`os.sendfile` on MacOS. Using +Fixed sending the part of the file in :func:`os.sendfile` on macOS. Using the *trailers* argument could cause sending more bytes from the input file -than was specified. Thanks Ned Deily for testing on 32-bit MacOS. +than was specified.