Re: [Nbd] TLS implementation in reference nbd-server

[Wouter Verhelst <w@...112...>]

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