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.
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.
>> 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 (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).
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.
>> + }
>> 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? 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. :)
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 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature