Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options

[Stefan Beller <sbeller@google.com>]

On Fri, Jul 8, 2016 at 3:21 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jul 08, 2016 at 03:17:13PM -0700, Stefan Beller wrote:
>
>> >If people are seeing these in
>> > routine use, then the limits are set too low, and this should happen
>> > roughly as often as a BUG assertion, and IMHO should be treated roughly
>> > the same: don't bother with translation, and don't worry about
>> > optimizing wasted bandwidth for this case. It won't happen enough to
>> > matter.
>>
>> Well the wasted band width is part of the server protection, no?
>
> Not if you stop receiving as soon as you hit the limits. Then of course
> they can send up to the limit each time, but that is not a DoS. That is
> things working as advertised.
>
>> This would favor the idea Jonathan came up with:
>>
>>     server: I advertise push options
>>     client: ok I want to use push options
>>     client: I'll send you 1000 push options with upper bound of 1000M
>>     server: It's a bit too much, eh?
>>     * server quits
>>
>> So this case only occurs for the (malicious?) corner case, where I
>> do not bother a translation.
>
> In the malicious case, the client says "I'll send you 10 push option
> with an upper bound of 1024K", and then sends gigabytes anyway. Either
> way the server has to react to what is sent, not what is promised.

Well that is what the initial patch did via:

+       for (i = 0; i < max_options; i++) {
+               char *line;
+               int len;
+
+               line = packet_read_line(0, &len);
+
+               if (!line)
+                       break;
+
+               if (len > max_size)
+                       die("protocol error: server configuration allows push "
+                           "options of size up to %d bytes", max_size);
+
+               len = strcspn(line, "\n");
+               line[len] = '\0';
+
+               string_list_append(ret, line);
+       }
+       if (i == max_options)
+               die("protocol error: server configuration only allows up "
+                   "to %d push options", max_options);

I assume the die is an effective way to "stop receiving".

Thinking further about what you said, I think the initial selections of
max_size and max_options is sufficient and we only see those bounds in
the malicious case.

This discussion rather makes me wonder if we want to stick to the initial design
as it was easy and not overcomplicating things as we assume the abort case
doesn't occur often.


>
> -Peff