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. And I would know,
because I had to fix the same bug in both qemu-nbd and in nbd-kit; it
matters when talking with a client that wants to use NBD_OPT_GO.
Something like the following (untested) patch is needed (Untested
because I can't get past autoreconf at the moment; see other thread...):
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);
+ }
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);
+ }
+ 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;
}
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature