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

On Thu, Feb 17, 2022 at 10:30:13AM -0600, Eric Blake
wrote:
> 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?
I was actually thinking about squashing my patches 1-4 together.
They're all really the same change, but I kept them separate for ease
of review.  What do you think?

But I think this patch:
> 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 */
... would stay separate, and you can push it before or after.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Thu, Feb 17, 2022 at 04:41:31PM +0000, Richard W.M. Jones
wrote:
> On Thu, Feb 17, 2022 at 10:30:13AM -0600, Eric Blake wrote:
> > 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?
> 
> I was actually thinking about squashing my patches 1-4 together.
> They're all really the same change, but I kept them separate for ease
> of review.  What do you think?
Seems reasonable (I'll confirm it again when I get through reviewing 4).
> 
> But I think this patch:
> 
> > 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
> ... would stay separate, and you can push it before or after.
Before - it is now commit 8c00ca2f ;)

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