On 02/09/22 23:07, Eric Blake wrote: > As mentioned in the previous patch, we left the state of the buffer > undefined if we fail pread prior to attempting NBD_CMD_READ. Better > is to tweak the generator to sanitize the buffer unconditionally, as a > way of hardening against potential bugs in client applications that > fail to check for errors, but which should not be leaking > uninitialized data. In the process, we can document that the contents > of buf are now merely unspecified, rather than undefined (valgrind > will no longer complain if you read buf, regardless of how nbd_pread > failed). > > As always calling memset() can be a performance hit if the client also > sanitizes the buffer or is willing to take the audit risk, the next > patch will add a knob that allows the client to control when this > happens. > ---

> > + (* Sanitize read buffers before any error is possible. *) > + List.iter ( > + function > + | BytesOut (n, count) > + | BytesPersistOut (n, count) -> > + pr " memset (%s, 0, %s);\n" n count > + | _ -> () > + ) args; > + > (* Check current state is permitted for this call. *) > if permitted_states <> [] then ( > let value = match errcode with It tends to assist reviewers if you include a diff in the cover letter (or better yet, in the Notes section of the patch, suitably quoted/escaped) that shows the effect of the patch on the generated C source code.

That said: Acked-by: Laszlo Ersek <lersek(a)redhat.com&gt; Thanks, Laszlo

On Thu, Feb 10, 2022 at 09:38:30AM +0000, Richard W.M. Jones wrote: > On Wed, Feb 09, 2022 at 04:07:26PM -0600, Eric Blake wrote: > > + "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"]; > > + }; > > Could it be worth mentioning CVE-2022-0485 by name in the text here? > And/or linking to: > https://listman.redhat.com/archives/libguestfs/2022-February/msg00104.html Good idea, I went with: diff --git i/generator/API.ml w/generator/API.ml index 9b7eb545..aaba9951 100644 --- i/generator/API.ml +++ w/generator/API.ml @@ -784,14 +784,19 @@ "set_pread_initialize", { 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. +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 (see CVE-2022-0485 in +L<libnbd-security(3)> for an example of a security hole in an +application built against an earlier version of libnbd that lacked +consistent pre-initialization). 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 (although the time spent in +pre-initialization may pale in comparison to time spent waiting on +network packets). Calling this function with C<request> set to false tells libnbd to skip the buffer initialization step in read commands."; > > Anyway the whole patch series looks good, so: > > Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; Series is now checked in with amendments, as ab52914b..e0953cb7

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")