On Mon, May 18, 2020 at 11:47:24AM -0400, Denton Liu wrote:
> A separate control packet is chosen because we need to be able to
> differentiate between what the remote server sends and remote-curl's
> control packets. By ensuring in the remote-curl code that a server
> cannot send response end packets, we prevent a malicious server from
> being able to perform a denial of service attack in which they spoof a
> response end packet and cause the described deadlock to happen.
Thanks for thinking about this. I was wondering what mischief a server
might be able to cause by sending such a packet.
> @@ -446,6 +447,13 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
> if (reader->status != PACKET_READ_FLUSH)
> die(_("expected flush after ref listing"));
>
> + if (stateless_rpc) {
> + if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)
> + die(_("expected response end packet after ref listing"));
> + if (packet_reader_read(reader) != PACKET_READ_FLUSH)
> + die(_("expected flush packet after response end"));
> + }
Having two packets here surprised me. We'd have already received the
actual flush from the server (as you can see in the context above).
Wouldn't a single RESPONSE_END be enough to signal the reader?
> diff --git a/fetch-pack.c b/fetch-pack.c
> index f73a2ce6cb..bcbbb7e2fb 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1468,6 +1468,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> struct fetch_negotiator negotiator_alloc;
> struct fetch_negotiator *negotiator;
> int seen_ack = 0;
> + int check_http_delimiter;
This flag was more complicated than I expected, and I'm not sure how we
can easily be certain that all necessary paths are covered.
E.g., in FETCH_PROCESS_ACKS, we'll always be reading a response via
process_acks(). Why do COMMON_FOUND and NO_COMMON_FOUND check for
end-of-response, but READY doesn't? I think the answer is that we'd
continue to read the same response via FETCH_GET_PACK in this instance.
I just wonder if there is a better place to put this logic that would be
more certain to catch every place we'd expect to read to the end of a
response. But I suppose not. We could push it down into process_acks(),
but it would have the same READY logic that's here. I'll admit part of
my complaint is that the existing do_fetch_pack_v2 state-machine switch
is kind of hard to follow, but that's not your fault.
Two things that might help:
- put a comment in READY to indicate why we don't need to check
end-of-response (but do for its sibling cases)
- instead of setting and clearing a variable and then using it to
check at the end of the loop, call check_http_delimiter(&reader,
args->stateless_rpc) at the moment we're done with the packet. The
actual executed code would have the same behavior, I think, but it
might be easier to understand that we're trying to hit the end of
each response.
Also, it probably should be check_stateless_delimiter() or something.
There could be other helpers that support stateless-connect besides
http.
Speaking of which: this is a change to the remote-helper protocol, since
we're now expecting stateless-connect helpers to produce these delim
packets (and failing if they don't). I kind of doubt that anybody but
remote-curl has implemented v2 stateless-connect, but should we be
marking this with an extra capability to be on the safe side?
> + if (args->stateless_rpc && check_http_delimiter) {
> + if (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)
> + die(_("git fetch-pack: expected response end packet"));
> + if (packet_reader_read(&reader) != PACKET_READ_FLUSH)
> + die(_("git fetch-pack: expected flush packet"));
> + }
I wondered also if stateless_rpc might catch cases besides
stateless-connect. But I guess we're in the v2 fetch-pack path, and that
_only_ understands stateless-connect.
-Peff
