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

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

On Fri, Oct 14, 2016 at 02:23:55PM -0500, Eric Blake wrote:
> On 10/14/2016 01:30 PM, Wouter Verhelst wrote:
> >>>
> >>> 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.
> 
> > 
> >  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);
> 
> Any thoughts about this one? Should we enforce a maximum client option
> length (such as: any client that wants to send us an NBD_OPT_* with more
> than 1M payload is trying to DoS us)?  If so, what is an appropriate
> limit?  We've sort of had the conversation in a past thread about
> maximum limits that a server must tolerate before declaring the client
> to be a waste of time (such as how many error messages can a server send
> before deciding the client isn't going to ask a successful question; how
> much data in NBD_OPT is too much, while still permitting our spec of 64k
> strings as acceptable and assuming we may add a future NBD_OPT that has
> multiple strings, etc).  At one point, I had even proposed a patch to
> force length to be 16 bits (freeing up 16 other bits that could be used
> for future expansion for some other purpose), but as I recall you didn't
> like the idea.

Mmm, right, and I still don't :-)

> Since we are malloc'ing a scratch buffer to hold a client-specified
> length, I do NOT want us to be casually allowing the client to tell us
> to make a 2G allocation.  Maybe when reading off dead length, it's
> better to write a loop that does a loop into a max-size buffer for as
> many loop iterations as needed, rather than allocating a single buffer
> that will just be thrown away; but such complexity doesn't belong on the
> hot-path of normal reads.  Still, even if I cap maximum allocation by
> reading in a loop, there's a question of how much time we allow to
> processing dead reads, vs. cutting our losses and disconnecting the
> client as ill-behaved.

As you've said downthread, there already is a function for that. Still,
I think it is a good idea to answer this question.

How about 4K? That's the maximum length for a string too, right now, and
it conveniently is just the size of a single memory page on most
architectures (i.e., the minimum allocation unit).

> >>  				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).
> > 
> 
> But we've already sent NBD_REP_ERR_INVALID in the line just before we
> detect the invalid length

Ah, yes, I'm an idiot. Didn't see that.

> (note: the guest can't tell whether we replied with that error because
> of invalid length or because of requesting at an invalid point in the
> handshake protocol).

Sure.

> Since I repeated a common construct across multiple lines, I may create
> a helper function to parse off length.  Especially if I want to add
> complexity for handling over-long user-supplied lengths without a huge
> malloc.

Would be helpful. Also, we should probably read the whole header before
starting to process it.

(a packed struct, for instance, which we already damn well do for other
protocol messages...)

> >> +				}
> >>  				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.
> 
> Okay, here we have a case where two errors are both possible at the same
> time: NBD_REP_ERR_POLICY (that the code just sent in the line earlier)
> and NBD_REP_ERR_INVALID (for bad length).  The protocol doesn't state
> any priority scheme when multiple errors are simultaneously possible,
> does it?

Not that I know of...

> I don't see that it makes any particular difference, although it may
> mean that we want to add a sentence to the protocol clarifying that if
> more than one error makes sense, a client MUST NOT rely on the server
> to send a specific error, only that an error will be sent.
> 
> > 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.
> 
> Well yeah, that's the plan, once I can build it to actually test it. :)

Hehe.

> I'm still plugging away at the autoreconf failure on my end.  This was
> just the quick PoC to get the conversation going, pointing out that you
> made a common mistake (since I've already fixed the same bug in both
> qemu and nbdkit); and that it is the same mistake that
> NBD_FLAG_FIXED_NEWSTYLE is supposed to advertise has been fixed.

It's actually worse than that. Before fixed newstyle, the server would
not send anything upon a message it did not understand, and would just
drop the connection. With fixed newstyle, at least it will send
NBD_REP_ERR_UNSUP before misinterpreting length as magic and dropping
the client.

> It is absolutely vital to the handshake protocol that the server read
> off exactly as much as the client sent even when replying with an
> error, in order to be back in sync for reading the next command from
> the client.

Indeed.

-- 
< 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