[Libguestfs] [libnbd PATCH 3/3] api: Add new API nbd_set_pread_initialize()
Laszlo Ersek
lersek at redhat.com
Thu Feb 10 14:01:03 UTC 2022
On 02/09/22 23:07, Eric Blake wrote:
> 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")
>
Wow, this is a *lot* of work for an API that most likely no libnbd
client application will use. After all -- are *we* going to be bold
enough to disable pre-init? It's one thing to make an audit once and
then disable pre-init, but that's a "standing promise" then, and if we
miss an error check anywhere *later on*, and it turns into a security
issue, someone will feel very uncomfortable. Because, "just forgetting
an error check" is different from "forgetting an error check despite
having promised we'd never make that mistake". Now that we have pre-init
built-in and even enabled by default, personally I don't see myself ever
disabling it, performance be damned. (But even wrt. performance, our
comment in nbd_internal_command_common() says: "no measurable overhead".)
I understand this patch may exist mainly so that no user complain about
uselessly wasted cycles... I just hope we'll never actually use this
API, beyond the unit tests. :)
Acked-by: Laszlo Ersek <lersek at redhat.com>
Thanks
Laszlo
More information about the Libguestfs
mailing list