On 05/11/2016 02:20 PM, Alex Bligh wrote: > > On 11 May 2016, at 20:08, Eric Blake <eblake@...696...> wrote: > >> Calling out a MUCH smaller limit of 16k during handshake phase in >> contrast to transmission phase, and acknowledging that we have bytes in >> transmission phase that will always be 0 without some other extension to >> state otherwise, seems worthwhile. > > OK, point taken. I'm convinced in principle of this one. > > However, I think 16k is too small if the string limit stays at 4k. That's > four whole strings. If we are going to support 4k strings, we should > support 4k strings. I thing it would be pretty hard to effectively > DoS any server with (e.g.) 1MB. I know the current limit (NBD_REP_SERVER) > is 8192 bytes plus loose change, but I don't think we should be in a > position where we need to change this every time a new option comes up. At what point do we think a limit is reasonable that no option or option reply would ever need to exceed a given size? Would 0xffff be reasonable as the upper limit (15 max-sized strings plus other data including lengths of those strings)? That way, we still manage to effectively reserve ourselves 2 bytes in case we ever finding ourselves wanting to use them for other purposes. Also, you can pack more than 15 strings, as long as at least one of them is not the maximum length. And remember that the limit is only on single messages; it's still fairly easy to break things up into multiple messages: if you need to send an array of strings to the server, do it via back-to-back NBD_OPT_ calls (one call per array entry) rather than a single NBD_OPT call (with the entire array). And we already do just that for replies (NBD_REP_SERVER and NBD_REP_INFO are both arrays of information broken up into smaller replies). > > I suspect you may say "but I want to allocate them on the stack". Yes, that is also a nice benefit for a reasonably small size, but not the driving factor (4096 is already borderline too large for stack allocation), and even if you can stack-allocate one string, by the time you have a struct containing multiple strings, malloc'd string pointers make more sense than reserving multiple 4096-byte arrays within the struct. > Well, IMHO tough! Stack vs. heap allocation is not the end of the world. > > Also the bit about potentially repurposing bits shouldn't be somewhere > where it hits the commit log, so I'd drop the last paragraph. I thought it made a stronger case for WHY we are explicitly choosing a value less than 0x10000 as the cutoff, because it leaves us room for future expansion. I don't mind if we trim the explanation a bit, but do think we should at least capture somewhere (whether in the protocol itself or in the commit message) our design decision, and particularly the realization that we MUST NOT define those extra bits without first negotiating a global/client flag to make it obvious those bits now mean something other than length. >> +The presence of the option length in every option allows the server to >> +skip any options presented by the client that it does not understand. >> +Although *length* is a 32-bit field, none of the options defined in >> +this standard include more than one string (which is capped at 4096 >> +bytes); and extensions should likewise limit the amount of data the >> +client can send for a single valid option. > > This is the problem above. Let's say someone introduces NBD_OPT_FOO > which contains 5 strings. Even 4 strings is too much for a 16k message - you also have to reserve space for the lengths of those strings (so a string at 4096 bytes actually occupies 4098 bytes of the message if you use a minimum 16-bit string length field; and with NBD_OPT_GO, we just introduced a 32-bit string length field even after I pointed out that 16 bits would have been sufficient, on the grounds that it more closely resembles NBD_OPT_EXPORT_NAME). > They now (a) have to modify this sentence > in the standard, and (b) will have servers which think them sending > the option is a DoS attack, rather than skipping the appropriate > length. We should be basing the maximum length not on the maximum > current length, but on the maximum likely length ever. Or we could say that the designer of NBD_OPT_FOO should instead invent NBD_OPT_FOO1 and NBD_OPT_FOO2, and break the options across two different requests, so that we stay within bounds. But I can be persuaded that a maximum of 3 strings does feel rather tight from what we might EVER want to do, and that a limit of 0xffff bytes (15 strings + other stuff) allows quite a bit more flexibility while still staying within a 2-byte length. > >> Therefore, the server MAY >> +treat any option request with *length* larger than 16,384 as a denial >> +of service attack, and initiate a hard disconnect. > > and we should pick a larger value there Anyone else have an opinion on a reasonable maximum option size to enforce? > >> If needed, a >> +future version of the standard might introduce a global flag and >> +client response flag that would allow the client and server to replace >> +the 32-bit length field with a pair of 16-bit flags and 16-bit length >> +fields. > > and then we can drop this bit. Indeed I think talking about what future > versions might do is inappropriate anyway. We already talk about what future versions might do if we run out of global flags, so it's not unprecedented, but I can agree that removing it from the spec and putting it only in the commit message may be a reasonable compromise. >> and extensions should likewise limit the >> +amount of data the server can send for a single option reply. >> +Therefore, the client MAY treat any option reply with *length* larger >> +than 16,384 as a corrupt server, > > I think simply as a DoS attack In my mind, a DoS is an attack where someone can tie up a long-running process to the exclusion of others. A client can DoS a server (make the server do so much work it can't accept other clients), but how does a server DoS a client (the client isn't trying to accept other connections)? I considered using the same wording in both places, until I realized that DoS doesn't make as much sense from server back to client. But if you're okay with the term in both directions, then I don't mind using it for consistency. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature