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

[Jeff King <peff@peff.net>]

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

> > 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