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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
