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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
