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

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>

> @@ -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"]; > };

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

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