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. I suspect you may say "but I want to allocate them on the stack". Well, IMHO tough! Also the bit about potentially repurposing bits shouldn't be somewhere where it hits the commit log, so I'd drop the last paragraph. Nits below. > --- > doc/proto.md | 28 +++++++++++++++++++++++++--- > 1 file changed, 25 insertions(+), 3 deletions(-) > > diff --git a/doc/proto.md b/doc/proto.md > index 2956746..0daed51 100644 > --- a/doc/proto.md > +++ b/doc/proto.md > @@ -137,9 +137,18 @@ C: 32 bits, option > C: 32 bits, length of option data (unsigned) > C: any data needed for the chosen option, of length as specified above. > > -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. > +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. 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. > 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 > 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. > > If the value of the option field is `NBD_OPT_EXPORT_NAME` and the server > is willing to allow the export, the server replies with information > @@ -192,6 +201,19 @@ S: 32 bits, length of the reply. This MAY be zero for some replies, in > S: any data as required by the reply (e.g., an export name in the case > of `NBD_REP_SERVER`) > > +Although *length* is a 32-bit field, none of the option replies > +defined in this standard include more than two strings (which are > +capped at 4096 bytes each); See above > 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 > and initiate a hard disconnect (this > +limit applies only to a single option reply, and not to the cumulative > +length of a series of replies to a single option request). ok > 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. > + same as above > The client MUST NOT send any option until it has received a final > reply to any option it has sent (note that some options e.g. > `NBD_OPT_LIST` have multiple replies, and the final reply is > -- > 2.5.5
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail