From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56062) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqoUt-00044L-Q3 for qemu-devel@nongnu.org; Thu, 14 Apr 2016 17:08:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aqoUq-00033J-Jj for qemu-devel@nongnu.org; Thu, 14 Apr 2016 17:08:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52390) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqoUq-00033E-CF for qemu-devel@nongnu.org; Thu, 14 Apr 2016 17:08:16 -0400 References: <1460060981-5338-1-git-send-email-eblake@redhat.com> From: Max Reitz Message-ID: <571006BC.3070603@redhat.com> Date: Thu, 14 Apr 2016 23:08:12 +0200 MIME-Version: 1.0 In-Reply-To: <1460060981-5338-1-git-send-email-eblake@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="uTHoswC3n4NIWVnP0qtEhaGdHuadEJsfE" Subject: Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't kill server on client that doesn't request TLS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: pbonzini@redhat.com, alex@alex.org.uk This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --uTHoswC3n4NIWVnP0qtEhaGdHuadEJsfE Content-Type: multipart/mixed; boundary="idDdsMU62eDcCcIf9lPXvDaWcUDr6DMmg" From: Max Reitz To: Eric Blake , qemu-devel@nongnu.org Cc: pbonzini@redhat.com, alex@alex.org.uk Message-ID: <571006BC.3070603@redhat.com> Subject: Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't kill server on client that doesn't request TLS References: <1460060981-5338-1-git-send-email-eblake@redhat.com> In-Reply-To: <1460060981-5338-1-git-send-email-eblake@redhat.com> --idDdsMU62eDcCcIf9lPXvDaWcUDr6DMmg Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 07.04.2016 22:29, Eric Blake wrote: > Upstream NBD is documenting that servers MAY choose to operate > in a conditional mode, where it is up to the client whether to > use TLS. For qemu's case, we want to always be in FORCEDTLS > mode, because of the risk of man-in-the-middle attacks, and since > we never export more than one device; likewise, the qemu client > will ALWAYS send NBD_OPT_STARTTLS as its first option. But now > that SELECTIVETLS servers exist, it is feasible to encounter a > (non-qemu) client that does not do NBD_OPT_STARTTLS first, but > rather wants to take advantage of the conditional modes it might > find elsewhere. >=20 > Since we require TLS, we are within our rights to drop connections > on any client that doesn't negotiate it right away, or which > attempts to negotiate it incorrectly, without violating the intent > of the NBD Protocol. However, it's better to allow the client to > continue trying, on the grounds that maybe the client will get the > hint to send NBD_OPT_STARTTLS. >=20 > Signed-off-by: Eric Blake > --- >=20 > My earlier patch was arguably incomplete: > https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg01265.html >=20 > But as it is already in a pull request, and as this one is > a bit more controversial, it's best to keep it as a separate patch. >=20 > nbd/server.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) >=20 > diff --git a/nbd/server.c b/nbd/server.c > index 7843584..2b727f0 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -450,9 +450,12 @@ static int nbd_negotiate_options(NBDClient *client= ) >=20 > default: > TRACE("Option 0x%x not permitted before TLS", clientfl= ags); > + if (nbd_negotiate_drop_sync(client->ioc, length) !=3D = length) { > + return -EIO; > + } > nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_TLS_RE= QD, > clientflags); > - return -EINVAL; > + break; > } What about NBD_OPT_EXPORTNAME? The specification says that this option does not allow for errors, and so the session must be terminated if this option is sent in FORCEDTLS mode without TLS having been negotiated. Max > } else if (fixedNewstyle) { > switch (clientflags) { > @@ -470,6 +473,9 @@ static int nbd_negotiate_options(NBDClient *client)= > return nbd_negotiate_handle_export_name(client, length= ); >=20 > case NBD_OPT_STARTTLS: > + if (nbd_negotiate_drop_sync(client->ioc, length) !=3D = length) { > + return -EIO; > + } > if (client->tlscreds) { > TRACE("TLS already enabled"); > nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_IN= VALID, > @@ -479,7 +485,7 @@ static int nbd_negotiate_options(NBDClient *client)= > nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_PO= LICY, > clientflags); > } > - return -EINVAL; > + break; > default: > TRACE("Unsupported option 0x%x", clientflags); > if (nbd_negotiate_drop_sync(client->ioc, length) !=3D = length) { >=20 --idDdsMU62eDcCcIf9lPXvDaWcUDr6DMmg-- --uTHoswC3n4NIWVnP0qtEhaGdHuadEJsfE Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXEAa8AAoJEDuxQgLoOKytkhQH/Rn0alMX4keC31Avh9WDtCSt TJ++1wkzD1C/Q48ujkU8L1kyY1yFU1shDkGwel/brFZWTFrljxP+9LhxOkdbNRVZ HiYOw9CCTef72JlHcKyZoLRQsv6o5AeBk159YWVv60oyPFx/mNhSl4dJXK0zd4X3 3mrN40706zr5ZDAmwBEqlAD6aG2tqrb9AGtIKI8O2FZcLQhoiY4v+BRcWpjYJGUu JvK1OHdZNIZYStaMsLj7Nt425Kb/6OZPf+sMDJ6eCpwa52vTRqRQzijtwHMaHVuZ EUtUSWdrQJEGjcmHPwDbIHtgoLpIn4v0BLCo59DtkuYo7Sl1awonr0kQdENpZY8= =hgIq -----END PGP SIGNATURE----- --uTHoswC3n4NIWVnP0qtEhaGdHuadEJsfE--