9:08 p.m.
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