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

From: Jeff King <peff@peff.net>
Date: May 18, 2020, 16:43 UTC
Message-ID: <20200518164308.GC42240@coredump.intra.peff.net>
In-Reply-To: <4b079bcd83ea80b8a0e81b0c1e3d5e083efeb9c6.1589816719.git.liu.denton@gmail.com>

On Mon, May 18, 2020 at 11:47:24AM -0400, Denton Liu wrote:

Show 6 quoted lines

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

Show 10 quoted lines

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

Show 9 quoted lines

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

Show 6 quoted lines

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