From c79706af4e7475bf58861a143b77b77a54e7a1cd Mon Sep 17 00:00:00 2001

From: Eric Blake <eblake@redhat.com>

Date: Wed, 9 Feb 2022 15:39:49 -0600

Subject: [PATCH] api: Add new API nbd_set_pread_initialize()

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 when compared to the 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.

In addition to adding two new APIs, this patch also causes changes to

the four existing APIs nbd_{aio_,}pread{,_structured}, with those

generated lib/api.c changes looking like:

| --- lib/api.c.bak	2022-02-10 08:17:09.973381979 -0600

| +++ lib/api.c	2022-02-10 08:22:27.503428024 -0600

| @@ -2871,7 +2914,8 @@ nbd_pread (struct nbd_handle *h, void *b

|      debug (h, "enter: buf=<buf> count=%zu offset=%" PRIu64 " flags=0x%x", count, offset, flags);

|    }

|

| -  memset (buf, 0, count);

| +  if (h->pread_initialize)

| +    memset (buf, 0, count);

|    if (unlikely (!pread_in_permitted_state (h))) {

|      ret = -1;

|      goto out;

Message-Id: <20220209220726.1902761-4-eblake@redhat.com>

Acked-by: Laszlo Ersek <lersek@redhat.com>

[eblake: enhance commit message to show generated file diff, mention CVE

in doc text]

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

(cherry picked from commit e0953cb71250947bb97b25e34ff1ea34bd504bf3)

---

 generator/API.ml                              | 90 ++++++++++++++++---

 generator/C.ml                                |  3 +-

 .../libnbd/libnbd_110_defaults_test.go        | 10 ++-

 .../libnbd_120_set_non_defaults_test.go       | 12 +++

 lib/handle.c                                  | 17 +++-

 lib/internal.h                                |  5 +-

 ocaml/tests/test_110_defaults.ml              |  4 +-

 ocaml/tests/test_120_set_non_defaults.ml      |  5 +-

 python/t/110-defaults.py                      |  3 +-

 python/t/120-set-non-defaults.py              |  4 +-

 tests/errors.c                                | 25 +++++-

 11 files changed, 156 insertions(+), 22 deletions(-)

diff --git a/generator/API.ml b/generator/API.ml

index d8df7c8..00ab34f 100644

--- a/generator/API.ml

+++ b/generator/API.ml

@@ -778,6 +778,49 @@ the time of compilation.";

                 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 (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.";

+    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;

@@ -1825,11 +1868,16 @@ C<LIBNBD_CMD_FLAG_DF>.

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

   };

@@ -1907,12 +1955,16 @@ more than one fragment (if that is supported - some servers cannot do

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

-                Link "set_strict_mode"];

+                Link "set_strict_mode"; Link "set_pread_initialize"];

   };

   "pwrite", {

@@ -2420,14 +2472,19 @@ as described in L<libnbd(3)/Completion callbacks>.

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

     see_also = [SectionLink "Issuing asynchronous commands";

                 Link "aio_pread_structured"; Link "pread";

-                Link "set_strict_mode"];

+                Link "set_strict_mode"; Link "set_pread_initialize"];

   };

   "aio_pread_structured", {

@@ -2449,13 +2506,18 @@ as described in L<libnbd(3)/Completion callbacks>.

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

                 Link "aio_pread"; Link "pread_structured";

-                Link "set_strict_mode"];

+                Link "set_strict_mode"; Link "set_pread_initialize"];

   };

   "aio_pwrite", {

@@ -3093,6 +3155,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 4a5bb58..2b6198c 100644

--- a/generator/C.ml

+++ b/generator/C.ml

@@ -496,7 +496,8 @@ let generate_lib_api_c () =

       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/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go b/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go

index b3ceb45..ca7c1c4 100644

--- a/golang/src/libguestfs.org/libnbd/libnbd_110_defaults_test.go

+++ b/golang/src/libguestfs.org/libnbd/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/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go b/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go

index f112456..029f0db 100644

--- a/golang/src/libguestfs.org/libnbd/libnbd_120_set_non_defaults_test.go

+++ b/golang/src/libguestfs.org/libnbd/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")

diff --git a/lib/handle.c b/lib/handle.c

index 67aa875..ac6c16e 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/lib/internal.h b/lib/internal.h

index 0e205ab..525499a 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/ocaml/tests/test_110_defaults.ml b/ocaml/tests/test_110_defaults.ml

index b36949f..04aa744 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 67928bb..f949807 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/python/t/110-defaults.py b/python/t/110-defaults.py

index fb961cf..a4262da 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 3da0c23..e71c6ad 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/tests/errors.c b/tests/errors.c

index f597b7e..0298da8 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,

-- 

2.31.1