Libguestfs - Feb 2022 - [PATCH nbdkit v2 2/7] tls-fallback: Fix filter for new .block_size callback

This filter doesn't call the next_open function in the non-TLS case,
and therefore it never opens the plugin.  This leaves the internal
state of nbdkit a bit strange.  There is no plugin context allocated,
and the last filter in the chain has a context c_next pointer of NULL.

This works, provided we intercept every possible callback, check the
non-TLS case, and prevent it from calling the next function (because
it would dereference the NULL c_next).

To avoid a crash in backend_block_size we must therefore provide a
.block_size callback in this filter.
---
 filters/tls-fallback/tls-fallback.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/filters/tls-fallback/tls-fallback.c
b/filters/tls-fallback/tls-fallback.c
index fab9e58b..b34e0431 100644
--- a/filters/tls-fallback/tls-fallback.c
+++ b/filters/tls-fallback/tls-fallback.c
@@ -138,6 +138,20 @@ tls_fallback_get_size (nbdkit_next *next,
   return next->get_size (next);
 }
 
+static int
+tls_fallback_block_size (nbdkit_next *next,
+                         void *handle,
+                         uint32_t *minimum,
+                         uint32_t *preferred,
+                         uint32_t *maximum)
+{
+  if (NOT_TLS) {
+    *minimum = *preferred = *maximum = 0;
+    return 0;
+  }
+  return next->block_size (next, minimum, preferred, maximum);
+}
+
 static int
 tls_fallback_can_write (nbdkit_next *next,
                         void *handle)
@@ -215,6 +229,7 @@ static struct nbdkit_filter filter = {
   .open               = tls_fallback_open,
   .export_description = tls_fallback_export_description,
   .get_size           = tls_fallback_get_size,
+  .block_size         = tls_fallback_block_size,
   .can_write          = tls_fallback_can_write,
   .can_flush          = tls_fallback_can_flush,
   .is_rotational      = tls_fallback_is_rotational,
-- 
2.35.1

On Thu, Feb 17, 2022 at 02:36:43PM +0000, Richard W.M. Jones
wrote:
> This filter doesn't call the next_open function in the non-TLS case,
> and therefore it never opens the plugin.  This leaves the internal
> state of nbdkit a bit strange.  There is no plugin context allocated,
> and the last filter in the chain has a context c_next pointer of NULL.
> 
> This works, provided we intercept every possible callback, check the
> non-TLS case, and prevent it from calling the next function (because
> it would dereference the NULL c_next).
> 
> To avoid a crash in backend_block_size we must therefore provide a
> .block_size callback in this filter.
> ---
>  filters/tls-fallback/tls-fallback.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
ACK.

Would you like to squash this in, and/or have me commit this separately?

commit 8c00ca2fe418aeecf0818feed227a72e76d87f18
Author: Eric Blake <eblake at redhat.com>
Date:   Thu Feb 17 10:24:50 2022 -0600

    tls-fallback: Enhance comments about required callbacks
    
    Rich ran into a crash while trying to add a new .block_size callback
    reachable during the client connection handshake (via .prepare), and
    nearly worked around the crash by reintroducing CVE-2019-14850 instead
    of the proper fix of implementing the new callback.  Add some comments
    to help us avoid a similar regression the next time we add a callback.

diff --git a/filters/tls-fallback/tls-fallback.c
b/filters/tls-fallback/tls-fallback.c
index fab9e58b..64e84926 100644
--- a/filters/tls-fallback/tls-fallback.c
+++ b/filters/tls-fallback/tls-fallback.c
@@ -107,7 +107,11 @@ tls_fallback_open (nbdkit_next_open *next, nbdkit_context
*nxdata,
                    const char *exportname, int is_tls)
 {
   /* We do NOT want to call next() when insecure, because we don't
-   * know how long it will take.
+   * know how long it will take.  See also CVE-2019-14850 in
+   * nbdkit-security.pod.  But that means that this filter must
+   * override every possible callback that can be reached during
+   * handshake, to avoid passing through a non-TLS call to a missing
+   * backend.
    */
   if (!is_tls)
     return &message; /* See NOT_TLS for this choice of handle */


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org