4:07 p.m.
The recent patch series for CVE-2022-0485 demonstrated that when
applications using libnbd are not careful about error checking, the
difference on whether a data leak is at least sanitized (all zeroes,
partial reads, or data leftover from a prior read) vs. a dangerous
information leak (uninitialized data from the heap) was partly under
libnbd's control. The previous two patches changed libnbd to always
sanitize, as a security hardening technique that prevents heap leaks
no matter how buggy the client app is. But a blind memset() also adds
an execution delay, even if it doesn't show up as the hot spot in our
profiling to the slower time spent with network traffic.
At any rate, if client apps choose to pre-initialize their buffers, or
otherwise audit their code to take on their own risk about not
dereferencing a buffer on failure paths, then the time spent by libnbd
doing memset() is wasted; so it is worth adding a knob to let a user
opt in to faster execution at the expense of giving up our memset()
hardening on their behalf.
---
lib/internal.h | 5 +-
generator/API.ml | 79 +++++++++++++++++++---
generator/C.ml | 3 +-
lib/handle.c | 17 ++++-
python/t/110-defaults.py | 3 +-
python/t/120-set-non-defaults.py | 4 +-
ocaml/tests/test_110_defaults.ml | 4 +-
ocaml/tests/test_120_set_non_defaults.ml | 5 +-
tests/errors.c | 25 ++++++-
golang/libnbd_110_defaults_test.go | 10 ++-
golang/libnbd_120_set_non_defaults_test.go | 12 ++++
11 files changed, 148 insertions(+), 19 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h
index 0e205aba..525499a9 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -1,5 +1,5 @@
/* nbd client library in userspace: internal definitions
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -123,6 +123,9 @@ struct nbd_handle {
/* Full info mode. */
bool full_info;
+ /* Sanitization for pread. */
+ bool pread_initialize;
+
/* Global flags from the server. */
uint16_t gflags;
diff --git a/generator/API.ml b/generator/API.ml
index 98f90031..9b7eb545 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -778,6 +778,44 @@ "get_handshake_flags", {
Link "aio_is_created"; Link "aio_is_ready"];
};
+ "set_pread_initialize", {
+ default_call with
+ args = [Bool "request"]; ret = RErr;
+ shortdesc = "control whether libnbd pre-initializes read buffers";
+ longdesc = "\
+By default, libnbd will pre-initialize the contents of a buffer
+passed to calls such as L<nbd_pread(3)> to all zeroes prior to checking
+for any other errors, so that even if a client application passed in an
+uninitialized buffer but fails to check for errors, it will not result
+in a potential security risk caused by an accidental leak of prior heap
+contents. However, for a client application that has audited that an
+uninitialized buffer is never dereferenced, or which performs its own
+pre-initialization, libnbd's sanitization efforts merely pessimize
+performance.
+
+Calling this function with C<request> set to false tells libnbd to
+skip the buffer initialization step in read commands.";
+ see_also = [Link "get_pread_initialize";
+ Link "set_strict_mode";
+ Link "pread"; Link "pread_structured"; Link
"aio_pread";
+ Link "aio_pread_structured"];
+ };
+
+ "get_pread_initialize", {
+ default_call with
+ args = []; ret = RBool;
+ may_set_error = false;
+ shortdesc = "see whether libnbd pre-initializes read buffers";
+ longdesc = "\
+Return whether libnbd performs a pre-initialization of a buffer passed
+to L<nbd_pread(3)> and similar to all zeroes, as set by
+L<nbd_set_pread_initialize(3)>.";
+ see_also = [Link "set_pread_initialize";
+ Link "set_strict_mode";
+ Link "pread"; Link "pread_structured"; Link
"aio_pread";
+ Link "aio_pread_structured"];
+ };
+
"set_strict_mode", {
default_call with
args = [ Flags ("flags", strict_flags) ]; ret = RErr;
@@ -1831,11 +1869,16 @@ "pread", {
The C<flags> parameter must be C<0> for now (it exists for future NBD
protocol extensions).
-Note that if this command fails, it is unspecified whether the contents
-of C<buf> will read as zero or as partial results from the server."
+Note that if this command fails, and L<nbd_get_pread_initialize(3)>
+returns true, then libnbd sanitized C<buf>, but it is unspecified
+whether the contents of C<buf> will read as zero or as partial results
+from the server. If L<nbd_get_pread_initialize(3)> returns false,
+then libnbd did not sanitize C<buf>, and the contents are undefined
+on failure."
^ strict_call_description;
see_also = [Link "aio_pread"; Link "pread_structured";
- Link "get_block_size"; Link "set_strict_mode"];
+ Link "get_block_size"; Link "set_strict_mode";
+ Link "set_pread_initialize"];
example = Some "examples/fetch-first-sector.c";
};
@@ -1919,8 +1962,12 @@ "pread_structured", {
this, see L<nbd_can_df(3)>). Libnbd does not validate that the server
actually obeys the flag.
-Note that if this command fails, it is unspecified whether the contents
-of C<buf> will read as zero or as partial results from the server."
+Note that if this command fails, and L<nbd_get_pread_initialize(3)>
+returns true, then libnbd sanitized C<buf>, but it is unspecified
+whether the contents of C<buf> will read as zero or as partial results
+from the server. If L<nbd_get_pread_initialize(3)> returns false,
+then libnbd did not sanitize C<buf>, and the contents are undefined
+on failure."
^ strict_call_description;
see_also = [Link "can_df"; Link "pread";
Link "aio_pread_structured"; Link "get_block_size";
@@ -2463,8 +2510,13 @@ "aio_pread", {
Note that you must ensure C<buf> is valid until the command has
completed. Furthermore, if the C<error> parameter to
C<completion_callback> is set or if L<nbd_aio_command_completed(3)>
-reports failure, it is unspecified whether the contents
-of C<buf> will read as zero or as partial results from the server.
+reports failure, and if L<nbd_get_pread_initialize(3)> returns true,
+then libnbd sanitized C<buf>, but it is unspecified whether the
+contents of C<buf> will read as zero or as partial results from the
+server. If L<nbd_get_pread_initialize(3)> returns false, then
+libnbd did not sanitize C<buf>, and the contents are undefined
+on failure.
+
Other parameters behave as documented in L<nbd_pread(3)>."
^ strict_call_description;
example = Some "examples/aio-connect-read.c";
@@ -2492,8 +2544,13 @@ "aio_pread_structured", {
Note that you must ensure C<buf> is valid until the command has
completed. Furthermore, if the C<error> parameter to
C<completion_callback> is set or if L<nbd_aio_command_completed(3)>
-reports failure, it is unspecified whether the contents
-of C<buf> will read as zero or as partial results from the server.
+reports failure, and if L<nbd_get_pread_initialize(3)> returns true,
+then libnbd sanitized C<buf>, but it is unspecified whether the
+contents of C<buf> will read as zero or as partial results from the
+server. If L<nbd_get_pread_initialize(3)> returns false, then
+libnbd did not sanitize C<buf>, and the contents are undefined
+on failure.
+
Other parameters behave as documented in L<nbd_pread_structured(3)>."
^ strict_call_description;
see_also = [SectionLink "Issuing asynchronous commands";
@@ -3136,6 +3193,10 @@ let first_version =
"get_private_data", (1, 8);
"get_uri", (1, 8);
+ (* Added in 1.11.x development cycle, will be stable and supported in 1.12. *)
+ "set_pread_initialize", (1, 12);
+ "get_pread_initialize", (1, 12);
+
(* These calls are proposed for a future version of libnbd, but
* have not been added to any released version so far.
"get_tls_certificates", (1, ??);
diff --git a/generator/C.ml b/generator/C.ml
index 4a5bb589..2b6198c3 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -496,7 +496,8 @@ let
function
| BytesOut (n, count)
| BytesPersistOut (n, count) ->
- pr " memset (%s, 0, %s);\n" n count
+ pr " if (h->pread_initialize)\n";
+ pr " memset (%s, 0, %s);\n" n count
| _ -> ()
) args;
diff --git a/lib/handle.c b/lib/handle.c
index cbb37e89..4f00c059 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -64,6 +64,7 @@ nbd_create (void)
h->unique = 1;
h->tls_verify_peer = true;
h->request_sr = true;
+ h->pread_initialize = true;
h->uri_allow_transports = LIBNBD_ALLOW_TRANSPORT_MASK;
h->uri_allow_tls = LIBNBD_TLS_ALLOW;
@@ -393,6 +394,20 @@ nbd_unlocked_get_handshake_flags (struct nbd_handle *h)
return h->gflags;
}
+int
+nbd_unlocked_set_pread_initialize (struct nbd_handle *h, bool request)
+{
+ h->pread_initialize = request;
+ return 0;
+}
+
+/* NB: may_set_error = false. */
+int
+nbd_unlocked_get_pread_initialize (struct nbd_handle *h)
+{
+ return h->pread_initialize;
+}
+
int
nbd_unlocked_set_strict_mode (struct nbd_handle *h, uint32_t flags)
{
diff --git a/python/t/110-defaults.py b/python/t/110-defaults.py
index fb961cfd..a4262dae 100644
--- a/python/t/110-defaults.py
+++ b/python/t/110-defaults.py
@@ -1,5 +1,5 @@
# libnbd Python bindings
-# Copyright (C) 2010-2020 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
@@ -22,5 +22,6 @@ assert h.get_export_name() == ""
assert h.get_full_info() is False
assert h.get_tls() == nbd.TLS_DISABLE
assert h.get_request_structured_replies() is True
+assert h.get_pread_initialize() is True
assert h.get_handshake_flags() == nbd.HANDSHAKE_FLAG_MASK
assert h.get_opt_mode() is False
diff --git a/python/t/120-set-non-defaults.py b/python/t/120-set-non-defaults.py
index 3da0c23e..e71c6ad0 100644
--- a/python/t/120-set-non-defaults.py
+++ b/python/t/120-set-non-defaults.py
@@ -1,5 +1,5 @@
# libnbd Python bindings
-# Copyright (C) 2010-2020 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
@@ -33,6 +33,8 @@ if h.supports_tls():
assert h.get_tls() == nbd.TLS_ALLOW
h.set_request_structured_replies(False)
assert h.get_request_structured_replies() is False
+h.set_pread_initialize(False)
+assert h.get_pread_initialize() is False
try:
h.set_handshake_flags(nbd.HANDSHAKE_FLAG_MASK + 1)
assert False
diff --git a/ocaml/tests/test_110_defaults.ml b/ocaml/tests/test_110_defaults.ml
index b36949f0..04aa744a 100644
--- a/ocaml/tests/test_110_defaults.ml
+++ b/ocaml/tests/test_110_defaults.ml
@@ -1,6 +1,6 @@
(* hey emacs, this is OCaml code: -*- tuareg -*- *)
(* libnbd OCaml test case
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -28,6 +28,8 @@ let
assert (tls = NBD.TLS.DISABLE);
let sr = NBD.get_request_structured_replies nbd in
assert (sr = true);
+ let init = NBD.get_pread_initialize nbd in
+ assert (init = true);
let flags = NBD.get_handshake_flags nbd in
assert (flags = NBD.HANDSHAKE_FLAG.mask);
let opt = NBD.get_opt_mode nbd in
diff --git a/ocaml/tests/test_120_set_non_defaults.ml
b/ocaml/tests/test_120_set_non_defaults.ml
index 67928bb5..f9498076 100644
--- a/ocaml/tests/test_120_set_non_defaults.ml
+++ b/ocaml/tests/test_120_set_non_defaults.ml
@@ -1,6 +1,6 @@
(* hey emacs, this is OCaml code: -*- tuareg -*- *)
(* libnbd OCaml test case
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -42,6 +42,9 @@ let
NBD.set_request_structured_replies nbd false;
let sr = NBD.get_request_structured_replies nbd in
assert (sr = false);
+ NBD.set_pread_initialize nbd false;
+ let init = NBD.get_pread_initialize nbd in
+ assert (init = false);
(try
NBD.set_handshake_flags nbd [ NBD.HANDSHAKE_FLAG.UNKNOWN 2 ];
assert false
diff --git a/tests/errors.c b/tests/errors.c
index f597b7ee..0298da88 100644
--- a/tests/errors.c
+++ b/tests/errors.c
@@ -213,7 +213,15 @@ main (int argc, char *argv[])
}
- /* Issue a connected command when not connected. */
+ /* Issue a connected command when not connected. pread_initialize defaults
+ * to set.
+ */
+ if (nbd_get_pread_initialize (nbd) != 1) {
+ fprintf (stderr, "%s: test failed: "
+ "nbd_get_pread_initialize gave unexpected result\n",
+ argv[0]);
+ exit (EXIT_FAILURE);
+ }
buf[0] = '1';
if (nbd_pread (nbd, buf, 512, 0, 0) != -1) {
fprintf (stderr, "%s: test failed: "
@@ -294,7 +302,14 @@ main (int argc, char *argv[])
}
check (EINVAL, "nbd_aio_command_completed: ");
- /* Read from an invalid offset, client-side */
+ /* Read from an invalid offset, client-side. When pread_initialize is off,
+ * libnbd should not have touched our buffer.
+ */
+ if (nbd_set_pread_initialize (nbd, false) == -1) {
+ fprintf (stderr, "%s\n", nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+ buf[0] = '1';
strict = nbd_get_strict_mode (nbd) | LIBNBD_STRICT_BOUNDS;
if (nbd_set_strict_mode (nbd, strict) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
@@ -307,6 +322,12 @@ main (int argc, char *argv[])
exit (EXIT_FAILURE);
}
check (EINVAL, "nbd_aio_pread: ");
+ if (buf[0] != '1') {
+ fprintf (stderr, "%s: test failed: "
+ "nbd_pread incorrectly sanitized buffer on client-side error\n",
+ argv[0]);
+ exit (EXIT_FAILURE);
+ }
/* We guarantee callbacks will be freed even on all error paths. */
if (nbd_aio_pread_structured (nbd, buf, 512, -1,
diff --git a/golang/libnbd_110_defaults_test.go b/golang/libnbd_110_defaults_test.go
index b3ceb45d..ca7c1c41 100644
--- a/golang/libnbd_110_defaults_test.go
+++ b/golang/libnbd_110_defaults_test.go
@@ -1,5 +1,5 @@
/* libnbd golang tests
- * Copyright (C) 2013-2021 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -59,6 +59,14 @@ func Test110Defaults(t *testing.T) {
t.Fatalf("unexpected structured replies state")
}
+ init, err := h.GetPreadInitialize()
+ if err != nil {
+ t.Fatalf("could not get pread initialize state: %s", err)
+ }
+ if init != true {
+ t.Fatalf("unexpected pread initialize state")
+ }
+
flags, err := h.GetHandshakeFlags()
if err != nil {
t.Fatalf("could not get handshake flags: %s", err)
diff --git a/golang/libnbd_120_set_non_defaults_test.go
b/golang/libnbd_120_set_non_defaults_test.go
index f112456c..029f0db3 100644
--- a/golang/libnbd_120_set_non_defaults_test.go
+++ b/golang/libnbd_120_set_non_defaults_test.go
@@ -93,6 +93,18 @@ func Test120SetNonDefaults(t *testing.T) {
t.Fatalf("unexpected structured replies state")
}
+ err = h.SetPreadInitialize(false)
+ if err != nil {
+ t.Fatalf("could not set pread initialize state: %s", err)
+ }
+ init, err := h.GetPreadInitialize()
+ if err != nil {
+ t.Fatalf("could not get pread initialize state: %s", err)
+ }
+ if init != false {
+ t.Fatalf("unexpected pread initialize state")
+ }
+
err = h.SetHandshakeFlags(HANDSHAKE_FLAG_MASK + 1)
if err == nil {
t.Fatalf("expect failure for out-of-range flags")
--
2.34.1