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

On Fri, Jul 08, 2016 at 03:29:09PM -0700, Stefan Beller wrote:

quoted

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.

Yes. I haven't been following the intermediate discussion and patches,
but I don't see anything wrong with the general design above. I think
you do need to use rp_error() to get the die message to the client for
non-ssh cases, though (that is a problem with other protocol-error
messages, too; I wonder if we should install a custom die handler, or
convert them all to some kind of rp_die()).

-Peff