Re: [PATCH v2 7/7] stateless-connect: send response end packet

[Jeff King <peff@peff.net>]

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