Eric Blake
2022-Feb-17 16:30 UTC
[Libguestfs] [PATCH nbdkit v2 2/7] tls-fallback: Fix filter for new .block_size callback
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
Richard W.M. Jones
2022-Feb-17 16:41 UTC
[Libguestfs] [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