From 16e7e9bf7ecc954ddd51bb43da7747155887a12a Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 25 Apr 2025 12:55:11 +0300 Subject: [PATCH 1/4] gh-132915: Try to detect a buffer overflow in fcntl() and ioctl() SystemError is raised when buffer overflow is detected. The stack and memory can already be corrupted, so treat this error as fatal. --- ...-04-25-12-55-06.gh-issue-132915.XuKCXn.rst | 3 + Modules/fcntlmodule.c | 59 ++++++++++++------- 2 files changed, 41 insertions(+), 21 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2025-04-25-12-55-06.gh-issue-132915.XuKCXn.rst diff --git a/Misc/NEWS.d/next/Library/2025-04-25-12-55-06.gh-issue-132915.XuKCXn.rst b/Misc/NEWS.d/next/Library/2025-04-25-12-55-06.gh-issue-132915.XuKCXn.rst new file mode 100644 index 00000000000000..95a7d9e9159d59 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-04-25-12-55-06.gh-issue-132915.XuKCXn.rst @@ -0,0 +1,3 @@ +:func:`fcntl.fcntl` and :func:`fcntl.ioctl` can now detect a buffer overflow +and raise :exc:`SystemError`. The stack and memory can be corrupted in such +case, so treat this error as fatal. diff --git a/Modules/fcntlmodule.c b/Modules/fcntlmodule.c index 81a1e1c22c8a27..2a6f7cfc2efb0f 100644 --- a/Modules/fcntlmodule.c +++ b/Modules/fcntlmodule.c @@ -22,6 +22,11 @@ # include // I_FLUSHBAND #endif +#define GUARDSZ 16 +// NUL followed by random bytes. +static const char guard[GUARDSZ] = "\x00\xfa\x69\xc4\x67\xa3\x6c\x58" + "\x06\x41\x8c\x77\x54\xd6\x51\x6b"; + /*[clinic input] module fcntl [clinic start generated code]*/ @@ -80,9 +85,10 @@ fcntl_fcntl_impl(PyObject *module, int fd, int code, PyObject *arg) return PyLong_FromLong(ret); } if (PyUnicode_Check(arg) || PyObject_CheckBuffer(arg)) { -#define FCNTL_BUFSZ 1024 Py_buffer view; - char buf[FCNTL_BUFSZ+1]; /* argument plus NUL byte */ +#define FCNTL_BUFSZ 1024 + /* argument plus NUL byte plus guard to detect a buffer overflow */ + char buf[FCNTL_BUFSZ+GUARDSZ]; if (!PyArg_Parse(arg, "s*", &view)) { return NULL; @@ -95,7 +101,7 @@ fcntl_fcntl_impl(PyObject *module, int fd, int code, PyObject *arg) return NULL; } memcpy(buf, view.buf, len); - buf[len] = '\0'; + memcpy(buf + len, guard, GUARDSZ); PyBuffer_Release(&view); do { @@ -106,6 +112,10 @@ fcntl_fcntl_impl(PyObject *module, int fd, int code, PyObject *arg) if (ret < 0) { return !async_err ? PyErr_SetFromErrno(PyExc_OSError) : NULL; } + if (memcmp(buf + len, guard, GUARDSZ) != 0) { + PyErr_SetString(PyExc_SystemError, "buffer overflow1"); + return NULL; + } return PyBytes_FromStringAndSize(buf, len); #undef FCNTL_BUFSZ } @@ -199,26 +209,22 @@ fcntl_ioctl_impl(PyObject *module, int fd, unsigned long code, PyObject *arg, if (PyUnicode_Check(arg) || PyObject_CheckBuffer(arg)) { Py_buffer view; #define IOCTL_BUFSZ 1024 - char buf[IOCTL_BUFSZ+1]; /* argument plus NUL byte */ + /* argument plus NUL byte plus guard to detect a buffer overflow */ + char buf[IOCTL_BUFSZ+GUARDSZ]; if (mutate_arg && !PyBytes_Check(arg) && !PyUnicode_Check(arg)) { if (PyObject_GetBuffer(arg, &view, PyBUF_WRITABLE) == 0) { - if (view.len <= IOCTL_BUFSZ) { - memcpy(buf, view.buf, view.len); - buf[view.len] = '\0'; - do { - Py_BEGIN_ALLOW_THREADS - ret = ioctl(fd, code, buf); - Py_END_ALLOW_THREADS - } while (ret == -1 && errno == EINTR && !(async_err = PyErr_CheckSignals())); - memcpy(view.buf, buf, view.len); - } - else { - do { - Py_BEGIN_ALLOW_THREADS - ret = ioctl(fd, code, view.buf); - Py_END_ALLOW_THREADS - } while (ret == -1 && errno == EINTR && !(async_err = PyErr_CheckSignals())); + Py_ssize_t len = view.len; + void *ptr = view.buf; + if (len <= IOCTL_BUFSZ) { + memcpy(buf, ptr, len); + memcpy(buf + len, guard, GUARDSZ); + ptr = buf; } + do { + Py_BEGIN_ALLOW_THREADS + ret = ioctl(fd, code, ptr); + Py_END_ALLOW_THREADS + } while (ret == -1 && errno == EINTR && !(async_err = PyErr_CheckSignals())); if (ret < 0) { if (!async_err) { PyErr_SetFromErrno(PyExc_OSError); @@ -226,7 +232,14 @@ fcntl_ioctl_impl(PyObject *module, int fd, unsigned long code, PyObject *arg, PyBuffer_Release(&view); return NULL; } + if (ptr == buf) { + memcpy(view.buf, buf, len); + } PyBuffer_Release(&view); + if (ptr == buf && memcmp(buf + len, guard, GUARDSZ) != 0) { + PyErr_SetString(PyExc_SystemError, "buffer overflow2"); + return NULL; + } return PyLong_FromLong(ret); } if (!PyErr_ExceptionMatches(PyExc_BufferError)) { @@ -246,7 +259,7 @@ fcntl_ioctl_impl(PyObject *module, int fd, unsigned long code, PyObject *arg, return NULL; } memcpy(buf, view.buf, len); - buf[len] = '\0'; + memcpy(buf + len, guard, GUARDSZ); PyBuffer_Release(&view); do { @@ -257,6 +270,10 @@ fcntl_ioctl_impl(PyObject *module, int fd, unsigned long code, PyObject *arg, if (ret < 0) { return !async_err ? PyErr_SetFromErrno(PyExc_OSError) : NULL; } + if (memcmp(buf + len, guard, GUARDSZ) != 0) { + PyErr_SetString(PyExc_SystemError, "buffer overflow3"); + return NULL; + } return PyBytes_FromStringAndSize(buf, len); #undef IOCTL_BUFSZ } From c1ee62edcafa57bfafe0a2d86665cad14a3e225e Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sat, 26 Apr 2025 17:21:46 +0300 Subject: [PATCH 2/4] 8 bytes is enough. --- Modules/fcntlmodule.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/fcntlmodule.c b/Modules/fcntlmodule.c index e26e8fb61073d2..f61d5ff83ef0ba 100644 --- a/Modules/fcntlmodule.c +++ b/Modules/fcntlmodule.c @@ -24,8 +24,7 @@ #define GUARDSZ 16 // NUL followed by random bytes. -static const char guard[GUARDSZ] = "\x00\xfa\x69\xc4\x67\xa3\x6c\x58" - "\x06\x41\x8c\x77\x54\xd6\x51\x6b"; +static const char guard[GUARDSZ] = "\x00\xfa\x69\xc4\x67\xa3\x6c\x58"; /*[clinic input] module fcntl From 2e5785edbb09734097cad9c4aee22a05bac71e13 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 27 Apr 2025 21:30:48 +0300 Subject: [PATCH 3/4] Update Modules/fcntlmodule.c Co-authored-by: Victor Stinner --- Modules/fcntlmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/fcntlmodule.c b/Modules/fcntlmodule.c index f61d5ff83ef0ba..61b9d69bbabc15 100644 --- a/Modules/fcntlmodule.c +++ b/Modules/fcntlmodule.c @@ -22,7 +22,7 @@ # include // I_FLUSHBAND #endif -#define GUARDSZ 16 +#define GUARDSZ 8 // NUL followed by random bytes. static const char guard[GUARDSZ] = "\x00\xfa\x69\xc4\x67\xa3\x6c\x58"; From 55de9626c3177338e75ac58e2caa7017f94b1de0 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 28 Apr 2025 11:08:03 +0300 Subject: [PATCH 4/4] Remove debugging remnants. --- Modules/fcntlmodule.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Modules/fcntlmodule.c b/Modules/fcntlmodule.c index 61b9d69bbabc15..ebcacd2fb0ece1 100644 --- a/Modules/fcntlmodule.c +++ b/Modules/fcntlmodule.c @@ -112,7 +112,7 @@ fcntl_fcntl_impl(PyObject *module, int fd, int code, PyObject *arg) return !async_err ? PyErr_SetFromErrno(PyExc_OSError) : NULL; } if (memcmp(buf + len, guard, GUARDSZ) != 0) { - PyErr_SetString(PyExc_SystemError, "buffer overflow1"); + PyErr_SetString(PyExc_SystemError, "buffer overflow"); return NULL; } return PyBytes_FromStringAndSize(buf, len); @@ -236,7 +236,7 @@ fcntl_ioctl_impl(PyObject *module, int fd, unsigned long code, PyObject *arg, } PyBuffer_Release(&view); if (ptr == buf && memcmp(buf + len, guard, GUARDSZ) != 0) { - PyErr_SetString(PyExc_SystemError, "buffer overflow2"); + PyErr_SetString(PyExc_SystemError, "buffer overflow"); return NULL; } return PyLong_FromLong(ret); @@ -270,7 +270,7 @@ fcntl_ioctl_impl(PyObject *module, int fd, unsigned long code, PyObject *arg, return !async_err ? PyErr_SetFromErrno(PyExc_OSError) : NULL; } if (memcmp(buf + len, guard, GUARDSZ) != 0) { - PyErr_SetString(PyExc_SystemError, "buffer overflow3"); + PyErr_SetString(PyExc_SystemError, "buffer overflow"); return NULL; } return PyBytes_FromStringAndSize(buf, len);