Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't kill server on client that doesn't request TLS

[Eric Blake]

[adding qemu-block in cc, since Paolo can't send pull request]

On 04/07/2016 02:29 PM, 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.
> 
> 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.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
> 
> My earlier patch was arguably incomplete:
> https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg01265.html
> 
> 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.
> 
>  nbd/server.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> 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)
> 
>              default:
>                  TRACE("Option 0x%x not permitted before TLS", clientflags);
> +                if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
> +                    return -EIO;
> +                }
>                  nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_TLS_REQD,
>                                         clientflags);
> -                return -EINVAL;
> +                break;
>              }
>          } else if (fixedNewstyle) {
>              switch (clientflags) {
> @@ -470,6 +473,9 @@ static int nbd_negotiate_options(NBDClient *client)
>                  return nbd_negotiate_handle_export_name(client, length);
> 
>              case NBD_OPT_STARTTLS:
> +                if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
> +                    return -EIO;
> +                }
>                  if (client->tlscreds) {
>                      TRACE("TLS already enabled");
>                      nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_INVALID,
> @@ -479,7 +485,7 @@ static int nbd_negotiate_options(NBDClient *client)
>                      nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_POLICY,
>                                             clientflags);
>                  }
> -                return -EINVAL;
> +                break;
>              default:
>                  TRACE("Unsupported option 0x%x", clientflags);
>                  if (nbd_negotiate_drop_sync(client->ioc, length) != length) {
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature