When we created nbd.Buffer to handle both aio_pread and aio_pwrite, we provided two different approaches to construct an instance. The constructor nbd.Buffer(int) allocates uninitialized data, to be filled in by aio_pread (rather than wasting time pre-zeroing the data just to be changed again - although that speedup is only possible when opting in to h.set_pread_initialized(false)). Meanwhile, the classmethod nbd.Buffer.from_bytearray(...) allocates initialized data, to be consumed by aio_pwrite. However, this scheme makes it trivially easy to use the wrong instance creation approach for a given handle operation, and thereby leak uninitialized heap contents into the destination with something as simple-looking as h.aio_pwrite(nbd.Buffer(512), 0). It doesn't help that while bytearray(10) is all zeroes (a conscientious choice of the Python language), and therefore nbd.Buffer.from_bytearray(10).is_zero() is deterministically True, nbd.Buffer(10).is_zero() is non-deterministic; nor that nbd.Buffer(10).to_bytearray() can allow Python code to play with uninitialized data. It is always a dangerous practice to leak heap contents; however, as this leak is triggered only by poor client Python code, and not under control of anything the server sends, we don't think it rises to the level of the heap leak that was assigned CVE-2022-0485 as plugged back in commit 8d444b41. The solution employed here is to mark when a buffer has been initialized, in nbd.Buffer.from_bytearray() and h.aio_pread[_structured], as well as force-initialize an uninitialized buffer before b.to_bytearray() or h.aio_pwrite. Furthermore, we can make b.is_zero() pretend an uninitialized buffer is all zeroes (since the user can no longer easily get at any other contents). The approach is not bulletproof - if h.aio_pread triggers a subsequent read error in the C code (such as client-side bounds check or server error response) AND the user has disabled h.set_pread_initialized(), we've already marked the buffer initialized at that point even though the buffer may not be fully written. But that's okay - we've already documented that a user opting in to that setting is responsible for safe buffer practices, and should not dereference a read buffer when an error is reported while retiring the command. Generated code changes as follows: | --- python/methods.h.bak 2022-06-06 07:43:33.167181594 -0500 | +++ python/methods.h 2022-06-06 07:43:40.494190038 -0500 | @@ -31,6 +31,7 @@ | struct py_aio_buffer { | Py_ssize_t len; | void *data; | + bool initialized; | }; | | extern char **nbd_internal_py_get_string_list (PyObject *); | --- python/methods.c.bak 2022-06-06 07:43:33.165181592 -0500 | +++ python/methods.c 2022-06-06 07:56:25.109094074 -0500 | @@ -3201,6 +3201,7 @@ nbd_internal_py_aio_pread (PyObject *sel | completion_user_data->buf = buf; | offset_u64 = offset; | | + buf_buf->initialized = true; | ret = nbd_aio_pread (h, buf_buf->data, buf_buf->len, offset_u64, completion, flags_u32); | completion_user_data = NULL; | if (ret == -1) { | @@ -3274,6 +3275,7 @@ nbd_internal_py_aio_pread_structured (Py | Py_INCREF (py_chunk_fn); | chunk_user_data->fn = py_chunk_fn; | | + buf_buf->initialized = true; | ret = nbd_aio_pread_structured (h, buf_buf->data, buf_buf->len, offset_u64, chunk, completion, flags_u32); | chunk_user_data = NULL; | completion_user_data = NULL; | @@ -3335,6 +3337,10 @@ nbd_internal_py_aio_pwrite (PyObject *se | completion_user_data->buf = buf; | offset_u64 = offset; | | + if (!buf_buf->initialized) { | + memset (buf_buf->data, 0, buf_buf->len); | + buf_buf->initialized = true; | + } | ret = nbd_aio_pwrite (h, buf_buf->data, buf_buf->len, offset_u64, completion, flags_u32); | completion_user_data = NULL; | if (ret == -1) { | --- python/nbd.py.bak 2022-06-06 07:43:33.169181596 -0500 | +++ python/nbd.py 2022-06-06 07:43:40.516190063 -0500 | @@ -150,7 +150,9 @@ class Buffer(object): | def is_zero(self, offset=0, size=-1): | '''Returns true if and only if all bytes in the buffer are zeroes. | | - Note that a freshly allocated buffer is uninitialized, not zero. | + Note that although a freshly allocated buffer is uninitialized, | + this will report it as all zeroes, as it will be force-initialized | + to zero before any code that can access the buffer's contents. | | By default this tests the whole buffer, but you can restrict | the test to a sub-range of the buffer using the optional Fixes: f05b8ec5 ("python: Change aio_buffer into nbd.Buffer class.", v0.9.8) --- generator/Python.ml | 23 +++++++++++++++++++---- python/handle.c | 9 ++++++++- python/t/510-aio-pwrite.py | 13 ++++++++++++- python/t/580-aio-is-zero.py | 7 +++++++ 4 files changed, 46 insertions(+), 6 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 392be87..8239803 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -37,6 +37,7 @@ let struct py_aio_buffer { Py_ssize_t len; void *data; + bool initialized; }; extern char **nbd_internal_py_get_string_list (PyObject *); @@ -79,7 +80,7 @@ let "aio_buffer_from_bytearray"; "aio_buffer_to_bytearray"; "aio_buffer_size"; - "aio_buffer_is_zero" ] @ List.map fst handle_calls); + "aio_buffer_is_zero"] @ List.map fst handle_calls); pr "\n"; pr "#endif /* LIBNBD_METHODS_H */\n" @@ -111,7 +112,7 @@ let "aio_buffer_from_bytearray"; "aio_buffer_to_bytearray"; "aio_buffer_size"; - "aio_buffer_is_zero" ] @ List.map fst handle_calls); + "aio_buffer_is_zero"] @ List.map fst handle_calls); pr " { NULL, NULL, 0, NULL }\n"; pr "};\n"; pr "\n"; @@ -408,6 +409,7 @@ let pr "))\n"; pr " goto out;\n"; + (* Two passes over parameters. Any 'goto err' must be in first pass. *) pr " h = get_handle (py_h);\n"; pr " if (!h) goto out;\n"; List.iter ( @@ -475,7 +477,18 @@ let ) args; pr "\n"; - (* Call the underlying C function. *) + (* Second pass, and call the underlying C function. *) + List.iter ( + function + | BytesPersistIn (n, _) -> + pr " if (!%s_buf->initialized) {\n" n; + pr " memset (%s_buf->data, 0, %s_buf->len);\n" n n; + pr " %s_buf->initialized = true;\n" n; + pr " }\n" + | BytesPersistOut (n, _) -> + pr " %s_buf->initialized = true;\n" n + | _ -> () + ) args; pr " ret = nbd_%s (h" name; List.iter ( function @@ -790,7 +803,9 @@ let def is_zero(self, offset=0, size=-1): '''Returns true if and only if all bytes in the buffer are zeroes. - Note that a freshly allocated buffer is uninitialized, not zero. + Note that although a freshly allocated buffer is uninitialized, + this will report it as all zeroes, as it will be force-initialized + to zero before any code that can access the buffer's contents. By default this tests the whole buffer, but you can restrict the test to a sub-range of the buffer using the optional diff --git a/python/handle.c b/python/handle.c index ed04b76..d42a563 100644 --- a/python/handle.c +++ b/python/handle.c @@ -127,6 +127,7 @@ nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args) return NULL; } + buf->initialized = false; if (!PyArg_ParseTuple (args, "n:nbd_internal_py_alloc_aio_buffer", &buf->len)) { free (buf); @@ -202,6 +203,7 @@ nbd_internal_py_aio_buffer_from_bytearray (PyObject *self, PyObject *args) } memcpy (buf->data, data, len); Py_XDECREF (arr); + buf->initialized = true; ret = PyCapsule_New (buf, aio_buffer_name, free_aio_buffer); if (ret == NULL) { @@ -228,6 +230,11 @@ nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject *args) if (buf == NULL) return NULL; + if (!buf->initialized) { + memset (buf->data, 0, buf->len); + buf->initialized = true; + } + return PyByteArray_FromStringAndSize (buf->data, buf->len); } @@ -288,7 +295,7 @@ nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args) return NULL; } - if (is_zero (buf->data + offset, size)) + if (!buf->initialized || is_zero (buf->data + offset, size)) Py_RETURN_TRUE; else Py_RETURN_FALSE; diff --git a/python/t/510-aio-pwrite.py b/python/t/510-aio-pwrite.py index 438cc54..89599fc 100644 --- a/python/t/510-aio-pwrite.py +++ b/python/t/510-aio-pwrite.py @@ -1,5 +1,5 @@ # libnbd Python bindings -# Copyright (C) 2010-2019 Red Hat Inc. +# Copyright (C) 2010-2022 Red Hat Inc. # # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -50,4 +50,15 @@ with open(datafile, "rb") as f: assert buf == content +# Also check that an uninitialized buffer doesn't leak heap contents +buf3 = nbd.Buffer(512) +cookie = h.aio_pwrite(buf3, 0, flags=nbd.CMD_FLAG_FUA) +while not h.aio_command_completed(cookie): + h.poll(-1) + +with open(datafile, "rb") as f: + content = f.read() + +assert nbd.Buffer.from_bytearray(content).is_zero() + os.unlink(datafile) diff --git a/python/t/580-aio-is-zero.py b/python/t/580-aio-is-zero.py index 49dea5e..1a9a7d3 100644 --- a/python/t/580-aio-is-zero.py +++ b/python/t/580-aio-is-zero.py @@ -69,3 +69,10 @@ assert not buf.is_zero(2**20-1, 1) assert buf.is_zero(2**20, 1) assert not buf.is_zero(0, 1) assert buf.is_zero(2**21-1, 1) + +# A buffer created with only a size is generally uninitialized until +# used with aio_pread; but it will be zeroed if accessed prematurely +buf = nbd.Buffer(1024) +assert buf.size() == 1024 +assert buf.is_zero() +assert nbd.Buffer.from_bytearray(buf.to_bytearray()).is_zero() -- 2.36.1