Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Denton Liu <liu.denton@gmail.com>
To: Git Mailing List <git@vger.kernel.org>
Cc: Jeff King <peff@peff.net>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Junio C Hamano <gitster@pobox.com>,
	Force Charlie <charlieio@outlook.com>
Subject: [PATCH v3 7/7] stateless-connect: send response end packet
Date: Tue, 19 May 2020 06:54:00 -0400
Message-ID: <c26e160fbc211f1fbb4e231a1c4a4b84fa347cf6.1589885479.git.liu.denton@gmail.com> (raw)
In-Reply-To: <cover.1589885479.git.liu.denton@gmail.com>

Currently, remote-curl acts as a proxy and blindly forwards packets
between an HTTP server and fetch-pack. In the case of a stateless RPC
connection where the connection is terminated before the transaction is
complete, remote-curl will blindly forward the packets before waiting on
more input from fetch-pack. Meanwhile, fetch-pack will read the
transaction and continue reading, expecting more input to continue the
transaction. This results in a deadlock between the two processes.

This can be seen in the following command which does not terminate:

	$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012
	Cloning into 'git'...

whereas the v1 version does terminate as expected:

	$ git -c protocol.version=1 clone https://github.com/git/git.git --shallow-since=20151012
	Cloning into 'git'...
	fatal: the remote end hung up unexpectedly

Instead of blindly forwarding packets, make remote-curl insert a
response end packet after proxying the responses from the remote server
when using stateless_connect(). On the RPC client side, ensure that each
response ends as described.

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.

Reported-by: Force Charlie <charlieio@outlook.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/gitremote-helpers.txt     |  4 +++-
 Documentation/technical/protocol-v2.txt |  2 ++
 builtin/fetch-pack.c                    |  2 +-
 connect.c                               | 16 +++++++++++++++-
 connect.h                               |  4 ++++
 fetch-pack.c                            | 13 +++++++++++++
 remote-curl.c                           |  5 +++++
 remote.h                                |  3 ++-
 t/t5702-protocol-v2.sh                  | 13 +++++++++++++
 transport.c                             |  3 ++-
 10 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index f48a031dc3..93baeeb029 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -405,7 +405,9 @@ Supported if the helper has the "connect" capability.
 	trying to fall back).  After line feed terminating the positive
 	(empty) response, the output of the service starts.  Messages
 	(both request and response) must consist of zero or more
-	PKT-LINEs, terminating in a flush packet. The client must not
+	PKT-LINEs, terminating in a flush packet. Response messages will
+	then have a response end packet after the flush packet to
+	indicate the end of a response.  The client must not
 	expect the server to store any state in between request-response
 	pairs.  After the connection ends, the remote helper exits.
 +
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt
index 7e3766cafb..3996d70891 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -33,6 +33,8 @@ In protocol v2 these special packets will have the following semantics:
 
   * '0000' Flush Packet (flush-pkt) - indicates the end of a message
   * '0001' Delimiter Packet (delim-pkt) - separates sections of a message
+  * '0002' Message Packet (response-end-pkt) - indicates the end of a response
+    for stateless connections
 
 Initial Client Request
 ----------------------
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 4771100072..94b0c89b82 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -224,7 +224,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	version = discover_version(&reader);
 	switch (version) {
 	case protocol_v2:
-		get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);
+		get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL, args.stateless_rpc);
 		break;
 	case protocol_v1:
 	case protocol_v0:
diff --git a/connect.c b/connect.c
index 11c6ec70a0..0df45a1108 100644
--- a/connect.c
+++ b/connect.c
@@ -406,10 +406,21 @@ static int process_ref_v2(const char *line, struct ref ***list)
 	return ret;
 }
 
+void check_stateless_delimiter(int stateless_rpc,
+			      struct packet_reader *reader,
+			      const char *error)
+{
+	if (!stateless_rpc)
+		return; /* not in stateless mode, no delimiter expected */
+	if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)
+		die("%s", error);
+}
+
 struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
 			     struct ref **list, int for_push,
 			     const struct argv_array *ref_prefixes,
-			     const struct string_list *server_options)
+			     const struct string_list *server_options,
+			     int stateless_rpc)
 {
 	int i;
 	*list = NULL;
@@ -446,6 +457,9 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
 	if (reader->status != PACKET_READ_FLUSH)
 		die(_("expected flush after ref listing"));
 
+	check_stateless_delimiter(stateless_rpc, reader,
+				  _("expected response end packet after ref listing"));
+
 	return list;
 }
 
diff --git a/connect.h b/connect.h
index 5f2382e018..235bc66254 100644
--- a/connect.h
+++ b/connect.h
@@ -22,4 +22,8 @@ int server_supports_v2(const char *c, int die_on_error);
 int server_supports_feature(const char *c, const char *feature,
 			    int die_on_error);
 
+void check_stateless_delimiter(int stateless_rpc,
+			       struct packet_reader *reader,
+			       const char *error);
+
 #endif
diff --git a/fetch-pack.c b/fetch-pack.c
index f73a2ce6cb..f096442d4d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1450,6 +1450,13 @@ enum fetch_state {
 	FETCH_DONE,
 };
 
+static void do_check_stateless_delimiter(const struct fetch_pack_args *args,
+					 struct packet_reader *reader)
+{
+	check_stateless_delimiter(args->stateless_rpc, reader,
+				  _("git fetch-pack: expected response end packet"));
+}
+
 static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				    int fd[2],
 				    const struct ref *orig_ref,
@@ -1534,6 +1541,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 			/* Process ACKs/NAKs */
 			switch (process_acks(negotiator, &reader, &common)) {
 			case READY:
+				/*
+				 * Don't check for response delimiter; get_pack() will
+				 * read the rest of this response.
+				 */
 				state = FETCH_GET_PACK;
 				break;
 			case COMMON_FOUND:
@@ -1541,6 +1552,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				seen_ack = 1;
 				/* fallthrough */
 			case NO_COMMON_FOUND:
+				do_check_stateless_delimiter(args, &reader);
 				state = FETCH_SEND_REQUEST;
 				break;
 			}
@@ -1560,6 +1572,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 			process_section_header(&reader, "packfile", 0);
 			if (get_pack(args, fd, pack_lockfile, sought, nr_sought))
 				die(_("git fetch-pack: fetch failed."));
+			do_check_stateless_delimiter(args, &reader);
 
 			state = FETCH_DONE;
 			break;
diff --git a/remote-curl.c b/remote-curl.c
index d02cb547e9..75532a8bae 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -703,6 +703,8 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si
 				state->remaining = packet_length(state->len_buf);
 				if (state->remaining < 0) {
 					die(_("remote-curl: bad line length character: %.4s"), state->len_buf);
+				} else if (state->remaining == 2) {
+					die(_("remote-curl: unexpected response end packet"));
 				} else if (state->remaining < 4) {
 					state->remaining = 0;
 				} else {
@@ -991,6 +993,9 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
 	if (rpc_in_data.pktline_state.remaining)
 		err = error(_("%d bytes of body are still expected"), rpc_in_data.pktline_state.remaining);
 
+	if (stateless_connect)
+		packet_response_end(rpc->in);
+
 	curl_slist_free_all(headers);
 	free(gzip_body);
 	return err;
diff --git a/remote.h b/remote.h
index 11d8719b58..5cc26c1b3b 100644
--- a/remote.h
+++ b/remote.h
@@ -179,7 +179,8 @@ struct ref **get_remote_heads(struct packet_reader *reader,
 struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
 			     struct ref **list, int for_push,
 			     const struct argv_array *ref_prefixes,
-			     const struct string_list *server_options);
+			     const struct string_list *server_options,
+			     int stateless_rpc);
 
 int resolve_remote_symref(struct ref *ref, struct ref *list);
 
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 4eb81ba2d4..8da65e60de 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -620,6 +620,19 @@ test_expect_success 'clone repository with http:// using protocol v2 with incomp
 	test_i18ngrep "bytes of body are still expected" err
 '
 
+test_expect_success 'clone with http:// using protocol v2 and invalid parameters' '
+	test_when_finished "rm -f log" &&
+
+	test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE_CURL="$(pwd)/log" \
+		git -c protocol.version=2 \
+		clone --shallow-since=20151012 "$HTTPD_URL/smart/http_parent" http_child_invalid &&
+
+	# Client requested to use protocol v2
+	grep "Git-Protocol: version=2" log &&
+	# Server responded using protocol v2
+	grep "git< version 2" log
+'
+
 test_expect_success 'clone big repository with http:// using protocol v2' '
 	test_when_finished "rm -f log" &&
 
diff --git a/transport.c b/transport.c
index 431a93caef..7d50c502ad 100644
--- a/transport.c
+++ b/transport.c
@@ -297,7 +297,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
 		if (must_list_refs)
 			get_remote_refs(data->fd[1], &reader, &refs, for_push,
 					ref_prefixes,
-					transport->server_options);
+					transport->server_options,
+					transport->stateless_rpc);
 		break;
 	case protocol_v1:
 	case protocol_v0:
-- 
2.26.2.706.g87896c9627


  parent reply index

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13 18:04 [PATCH 0/6] remote-curl: partial fix for a deadlock with stateless rpc Denton Liu
2020-05-13 18:04 ` [PATCH 1/6] remote-curl: fix typo Denton Liu
2020-05-13 18:04 ` [PATCH 2/6] remote-curl: remove label indentation Denton Liu
2020-05-13 18:04 ` [PATCH 3/6] transport: combine common cases with a fallthrough Denton Liu
2020-05-13 23:14   ` Eric Sunshine
2020-05-18  9:18     ` Denton Liu
2020-05-18 17:43       ` Eric Sunshine
2020-05-13 18:04 ` [PATCH 4/6] pkt-line: extern packet_length() Denton Liu
2020-05-13 23:23   ` Eric Sunshine
2020-05-15 20:56   ` Jeff King
2020-05-15 20:57     ` Jeff King
2020-05-13 18:04 ` [PATCH 5/6] remote-curl: error on incomplete packet Denton Liu
2020-05-15 21:38   ` Jeff King
2020-05-18  9:08     ` Denton Liu
2020-05-18 15:49       ` Jeff King
2020-05-13 18:04 ` [PATCH 6/6] remote-curl: ensure last packet is a flush Denton Liu
2020-05-15 21:02   ` Denton Liu
2020-05-15 21:41     ` Jeff King
2020-05-18 16:34       ` Junio C Hamano
2020-05-18 16:52         ` Jeff King
2020-05-18 21:00           ` Jeff King
2020-05-18 15:47 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Denton Liu
2020-05-18 15:47   ` [PATCH v2 1/7] remote-curl: fix typo Denton Liu
2020-05-18 15:47   ` [PATCH v2 2/7] remote-curl: remove label indentation Denton Liu
2020-05-18 18:37     ` Junio C Hamano
2020-05-18 15:47   ` [PATCH v2 3/7] transport: extract common fetch_pack() call Denton Liu
2020-05-18 18:40     ` Junio C Hamano
2020-05-18 15:47   ` [PATCH v2 4/7] pkt-line: extern packet_length() Denton Liu
2020-05-18 16:04     ` Jeff King
2020-05-18 17:50       ` Eric Sunshine
2020-05-18 20:08         ` Jeff King
2020-05-18 18:44       ` Junio C Hamano
2020-05-18 15:47   ` [PATCH v2 5/7] remote-curl: error on incomplete packet Denton Liu
2020-05-18 16:22     ` Jeff King
2020-05-18 16:51       ` Denton Liu
2020-05-18 15:47   ` [PATCH v2 6/7] pkt-line: PACKET_READ_RESPONSE_END Denton Liu
2020-05-18 15:47   ` [PATCH v2 7/7] stateless-connect: send response end packet Denton Liu
2020-05-18 16:43     ` Jeff King
2020-05-18 17:12       ` Denton Liu
2020-05-18 17:26         ` Jeff King
2020-05-18 16:50   ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Jeff King
2020-05-18 17:36     ` Denton Liu
2020-05-18 20:58       ` Jeff King
2020-05-18 22:52         ` Junio C Hamano
2020-05-19  2:38           ` Jeff King
2020-05-18 19:36     ` Junio C Hamano
2020-05-19 10:53   ` [PATCH v3 " Denton Liu
2020-05-19 10:53     ` [PATCH v3 1/7] remote-curl: fix typo Denton Liu
2020-05-19 10:53     ` [PATCH v3 2/7] remote-curl: remove label indentation Denton Liu
2020-05-19 10:53     ` [PATCH v3 3/7] transport: extract common fetch_pack() call Denton Liu
2020-05-19 10:53     ` [PATCH v3 4/7] pkt-line: extern packet_length() Denton Liu
2020-05-19 16:23       ` Eric Sunshine
2020-05-19 10:53     ` [PATCH v3 5/7] remote-curl: error on incomplete packet Denton Liu
2020-05-19 10:53     ` [PATCH v3 6/7] pkt-line: define PACKET_READ_RESPONSE_END Denton Liu
2020-05-19 10:54     ` Denton Liu [this message]
2020-05-19 18:40     ` [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects Jeff King
2020-05-19 21:14       ` Denton Liu
2020-05-19 20:51     ` [PATCH v3 8/7] fixup! pkt-line: extern packet_length() Denton Liu
2020-05-22 13:33     ` [PATCH v3 9/9] fixup! remote-curl: error on incomplete packet Denton Liu
2020-05-22 15:54       ` Jeff King
2020-05-22 16:05         ` Denton Liu
2020-05-22 16:31           ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c26e160fbc211f1fbb4e231a1c4a4b84fa347cf6.1589885479.git.liu.denton@gmail.com \
    --to=liu.denton@gmail.com \
    --cc=charlieio@outlook.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git