On Thu, Oct 13, 2016 at 05:36:18PM -0500, Eric Blake wrote:
> On 10/12/2016 01:40 PM, Wouter Verhelst wrote:
> > Hi,
> > 
> > While stuck in an airport on a 9-hour layover two days ago, I (finally)
> > spent some time working on a STARTTLS implementation for the reference
> > nbd-server implementation. Configuration is fairly basic; just add a
> > "tlsdir = " configuration item to the nbd-server config file, create a
> > ca.pem, priv.pem, and cert.pem file in that location, and you're good.
> > The current implementation doesn't allow for authenticating clients
> > through certificates or other means; I will probably want to add that at
> > some point in the future, but not just yet.
> > 
> > It's not been tested yet, however, because the client side hasn't been
> > done yet.
> 
> It has at least one bug, from what I've quickly seen.  You HAVE to parse
> off length and any data the client sends before you can try to read the
> next option; you have this bug in multiple places.

Whoops.

[...]
> diff --git i/nbd-server.c w/nbd-server.c
> index 25c335b..f394690 100644
> --- i/nbd-server.c
> +++ w/nbd-server.c
> @@ -352,8 +352,14 @@ static void socket_write_tls(CLIENT* client, void
> *buf, size_t len) {
>  }
> 
>  static void socket_read(CLIENT* client, void *buf, size_t len) {
> +     void *tmp = NULL;
> +     if (!buf) {
> +             /* FIXME: Enforce maximum bound on client-provided len? */
> +             tmp = buf = malloc(len);
> +     }
>       g_assert(client->socket_read != NULL);
>       client->socket_read(client, buf, len);
> +     free(tmp);
>  }
> 
>  /**
> @@ -1457,6 +1463,7 @@ CLIENT* negotiate(int net, GArray* servers, const
> gchar* tlsdir) {
>       uint64_t magic;
>       uint32_t cflags = 0;
>       uint32_t opt;
> +     uint32_t len;
>       CLIENT* client = g_new0(CLIENT, 1);
>       client->net = net;
>       client->socket_read = socket_read_notls;
> @@ -1491,6 +1498,11 @@ CLIENT* negotiate(int net, GArray* servers, const
> gchar* tlsdir) {
>                               // so must do hard close
>                               goto hard_close;
>                       }
> +                     socket_read(client, &len, sizeof(len));
> +                     len = ntohl(len);
> +                     if(len) {
> +                             socket_read(client, NULL, len);
> +                     }
>                       send_reply(client, opt, NBD_REP_ERR_TLS_REQD, -1, "TLS 
> is required
> on this server");
>                       continue;
>               }
> @@ -1514,16 +1526,32 @@ CLIENT* negotiate(int net, GArray* servers,
> const gchar* tlsdir) {
>               case NBD_OPT_STARTTLS:
>                       if(client->tls_session != NULL) {
>                               send_reply(client, opt, NBD_REP_ERR_INVALID, 
> -1, "Invalid STARTTLS
> request: TLS has already been negotiated!");
> +                             socket_read(client, &len, sizeof(len));
> +                             len = ntohl(len);
> +                             if(len) {
> +                                     socket_read(client, NULL, len);

This should probably send NBD_REP_ERR_INVALID, though (since STARTTLS does not
allow data).

> +                             }
>                               continue;
>                       }
>                       if(tlsdir == NULL) {
>                               send_reply(client, opt, NBD_REP_ERR_POLICY, -1, 
> "TLS not allowed on
> this server");
> +                             socket_read(client, &len, sizeof(len));
> +                             len = ntohl(len);
> +                             if(len) {
> +                                     socket_read(client, NULL, len);

Same here.

> +                             }
> +                             continue;
>                       }
>                       if(handle_starttls(client, opt, servers, cflags, 
> tlsdir) == NULL) {
>                               // can't recover from failed TLS negotiation.
>                               goto hard_close;
>                       }
>               default:
> +                     socket_read(client, &len, sizeof(len));
> +                     len = ntohl(len);
> +                     if(len) {
> +                             socket_read(client, NULL, len);
> +                     }
>                       send_reply(client, opt, NBD_REP_ERR_UNSUP, -1, "The 
> given option is
> unknown to this server implementation");
>                       break;
>               }

Other than that, looks good, but can you send that with git format-patch (or
git send-email)?  Makes applying it (with a proper commit message) easier.
Thanks!

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

Reply via email to