[Libguestfs] [libnbd PATCH] api: Add nbd_[aio_]opt_starttls

[Eric Blake]

On Thu, Oct 06, 2022 at 03:26:20PM +0100, Richard W.M. Jones wrote:
> On Tue, Oct 04, 2022 at 07:51:18AM -0500, Eric Blake wrote:
> > Very similar to the recent addition of nbd_opt_structured_reply, with
> > the new nbd_opt_starttls() finally giving us fine-grained control over
> > NBD_OPT_STARTTLS, and allowing productive handshaking with a server in
> > SELECTIVETLS mode.
> > 
> > With this patch, it is now easy to reproduce the scenario of nbdkit's
> > CVE-2021-3716, where a plaintext meddler-in-the-middle attacker could
> > cause client denial of service when a --tls=on server failed to reset
> > state after NBD_OPT_STARTTLS.  This also exposed the fact that nbdkit
> > was not gracefully handling NBD_OPT_INFO from a plaintext client
> > connecting to a --tls=require server; the testsuite is skipped unless
> > using a fixed nbdkit (v1.33.2 or later).
> 
> I guess so.  It does seem complicated and a rather niche use case.
> Perhaps we should just document the new API rather than bothering to
> update the existing API call documentation, since almost certainly no
> one using those APIs would ever need to think about this API?

...
> > +++ b/generator/API.ml
> > @@ -599,7 +599,11 @@   "set_tls", {
> >  =item C<LIBNBD_TLS_DISABLE>
> > 
> >  Disable TLS.  (The default setting, unless using L<nbd_connect_uri(3)> with
> > -a URI that requires TLS)
> > +a URI that requires TLS).
> > +
> > +This setting is also useful during integration testing when using
> > +L<nbd_set_opt_mode(3)> and L<nbd_opt_starttls(3)> for manual
> > +control over when to request TLS negotiation.
> > 
> >  =item C<LIBNBD_TLS_ALLOW>

I think this one is important; but I added a bit more wording.  The
NBD standard talks about SELECTIVETLS vs FORCEDTLS servers.
SELECTIVETLS servers are uncommon (qemu refuses to be one, and while
nbdkit supports it with --tls=on instead of --tls=require, it
documents that it carries a risk of insecurity without also using
--filter=tls-fallback).  But the important part here is that when
talking to a SELECTIVETLS server, you want LIBNBD_TLS_DISABLE if you
plan to do anything prior to turning on TLS, because LIBNBD_TLS_ALLOW
turns on TLS before allowing any manual control over option
negotiating (that is, nbdkit's --tls=on coupled with libnbd TLS_ALLOW
intentionally defaults to automatic rather than manual TLS
negotiation).

> > @@ -657,7 +662,7 @@   "get_tls_negotiated", {
> >  After connecting you may call this to find out if the
> >  connection is using TLS.
> > 
> > -This is only really useful if you set the TLS request mode
> > +This is normally useful only if you set the TLS request mode
> >  to C<LIBNBD_TLS_ALLOW> (see L<nbd_set_tls(3)>), because in this
> >  mode we try to use TLS but fall back to unencrypted if it was
> >  not available.  This function will tell you if TLS was
> > @@ -665,8 +670,14 @@   "get_tls_negotiated", {
> > 
> >  In C<LIBNBD_TLS_REQUIRE> mode (the most secure) the connection
> >  would have failed if TLS could not be negotiated, and in
> > -C<LIBNBD_TLS_DISABLE> mode TLS is not tried.";
> > -    see_also = [Link "set_tls"; Link "get_tls"];
> > +C<LIBNBD_TLS_DISABLE> mode TLS is not tried automatically.
> > +
> > +However, when doing manual integration testing of server
> > +behavior, when you use L<nbd_set_opt_mode(3)> along with TLS
> > +request mode C<LIBNBD_TLS_DISABLE> before triggering the TLS
> > +handshake with L<nbd_opt_starttls(3)>, then this will report
> > +the result of that manual effort.";
> > +    see_also = [Link "set_tls"; Link "get_tls"; Link "opt_starttls"];
> >    };

Also useful, but also worth mentioning C<SELECTIVETLS>.

> > +++ b/generator/states-newstyle-opt-starttls.c
> > @@ -68,12 +73,14 @@  NEWSTYLE.OPT_STARTTLS.RECV_REPLY_PAYLOAD:
> >   NEWSTYLE.OPT_STARTTLS.CHECK_REPLY:
> >    uint32_t reply;
> >    struct socket *new_sock;
> > +  int err = ENOTSUP;
> > 
> >    reply = be32toh (h->sbuf.or.option_reply.reply);
> >    switch (reply) {
> >    case NBD_REP_ACK:
> >      nbd_internal_reset_size_and_flags (h);
> >      h->structured_replies = false;
> > +    h->meta_valid = false;

I enhanced the testsuite to actually test that this makes a
difference.

> >      new_sock = nbd_internal_crypto_create_session (h, h->sock);
> >      if (new_sock == NULL) {
> >        SET_NEXT_STATE (%.DEAD);
> > @@ -86,6 +93,9 @@  NEWSTYLE.OPT_STARTTLS.CHECK_REPLY:
> >        SET_NEXT_STATE (%TLS_HANDSHAKE_WRITE);
> >      return 0;
> > 
> > +  case NBD_REP_ERR_INVALID:
> > +    err = EINVAL;
> > +    /* fallthrough */
> >    default:
> >      if (handle_reply_error (h) == -1) {
> >        SET_NEXT_STATE (%.DEAD);
> > @@ -102,10 +112,16 @@  NEWSTYLE.OPT_STARTTLS.CHECK_REPLY:
> >        return 0;
> >      }
> > 
> > -    debug (h,
> > -           "server refused TLS (%s), continuing with unencrypted connection",
> > -           reply == NBD_REP_ERR_POLICY ? "policy" : "not supported");
> > -    SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
> > +    debug (h, "server refused TLS (%s)",
> > +           reply == NBD_REP_ERR_POLICY ? "policy" :
> > +           reply == NBD_REP_ERR_INVALID ? "invalid request" : "not supported");
> > +    CALL_CALLBACK (h->opt_cb.completion, &err);

And this was missing a call to nbd_internal_free_option (h).

The commit is now in as 53617c92

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