git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Denton Liu <liu.denton@gmail.com>
Date
May 18, 2020, 15:47 UTC
Message-ID
<4b079bcd83ea80b8a0e81b0c1e3d5e083efeb9c6.1589816719.git.liu.denton@gmail.com>
In-Reply-To
<cover.1589816718.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 response end and flush packets 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                               | 10 +++++++++-
 fetch-pack.c                            | 12 ++++++++++++
 remote-curl.c                           |  7 +++++++
 remote.h                                |  3 ++-
 t/t5702-protocol-v2.sh                  | 13 +++++++++++++
 transport.c                             |  3 ++-
 9 files changed, 51 insertions(+), 5 deletions(-)
diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index f48a031dc3..84f8e92b23 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
+	have a response end packet before 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..12b57f5c0a 100644
--- a/connect.c
+++ b/connect.c
@@ -409,7 +409,8 @@ static int process_ref_v2(const char *line, struct ref ***list)
 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 +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"));
+	}
+
 	return list;
 }
 
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;
 
 	if (args->no_dependents) {
 		negotiator = NULL;
@@ -1486,6 +1487,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	}
 
 	while (state != FETCH_DONE) {
+		check_http_delimiter = 0;
+
 		switch (state) {
 		case FETCH_CHECK_LOCAL:
 			sort_ref_list(&ref, ref_compare_name);
@@ -1542,6 +1545,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				/* fallthrough */
 			case NO_COMMON_FOUND:
 				state = FETCH_SEND_REQUEST;
+				check_http_delimiter = 1;
 				break;
 			}
 			break;
@@ -1562,10 +1566,18 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				die(_("git fetch-pack: fetch failed."));
 
 			state = FETCH_DONE;
+			check_http_delimiter = 1;
 			break;
 		case FETCH_DONE:
 			continue;
 		}
+
+		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"));
+		}
 	}
 
 	if (negotiator)
diff --git a/remote-curl.c b/remote-curl.c
index d02cb547e9..8a72b5ee7a 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,11 @@ 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);
+		packet_flush(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 a6002e502f..182978f4be 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
Previous: Denton LiuNext: Jeff King
Message 34 of 62 in “remote-curl: partial fix for a deadlock with stateless rpc”
  1. 0/6 remote-curl: partial fix for a deadlock with stateless rpcDenton Liu, May 13, 2020
  2. 1/6 remote-curl: fix typoDenton Liu, May 13, 2020
  3. 2/6 remote-curl: remove label indentationDenton Liu, May 13, 2020
  4. 3/6 transport: combine common cases with a fallthroughDenton Liu, May 13, 2020
  5. Eric SunshineMay 13, 2020
  6. Denton LiuMay 18, 2020
  7. Eric SunshineMay 18, 2020
  8. 4/6 pkt-line: extern packet_length()Denton Liu, May 13, 2020
  9. Eric SunshineMay 13, 2020
  10. Jeff KingMay 15, 2020
  11. Jeff KingMay 15, 2020
  12. 5/6 remote-curl: error on incomplete packetDenton Liu, May 13, 2020
  13. Jeff KingMay 15, 2020
  14. Denton LiuMay 18, 2020
  15. Jeff KingMay 18, 2020
  16. 6/6 remote-curl: ensure last packet is a flushDenton Liu, May 13, 2020
  17. Denton LiuMay 15, 2020
  18. Jeff KingMay 15, 2020
  19. Junio C HamanoMay 18, 2020
  20. Jeff KingMay 18, 2020
  21. Jeff KingMay 18, 2020
  22. 0/7 remote-curl: fix deadlocks when remote server disconnectsDenton Liu, May 18, 2020
  23. 1/7 remote-curl: fix typoDenton Liu, May 18, 2020
  24. 2/7 remote-curl: remove label indentationDenton Liu, May 18, 2020
  25. Junio C HamanoMay 18, 2020
  26. 4/7 pkt-line: extern packet_length()Denton Liu, May 18, 2020
  27. Jeff KingMay 18, 2020
  28. Eric SunshineMay 18, 2020
  29. Jeff KingMay 18, 2020
  30. Junio C HamanoMay 18, 2020
  31. 3/7 transport: extract common fetch_pack() callDenton Liu, May 18, 2020
  32. Junio C HamanoMay 18, 2020
  33. 6/7 pkt-line: PACKET_READ_RESPONSE_ENDDenton Liu, May 18, 2020
  34. 7/7 stateless-connect: send response end packetDenton Liu, May 18, 2020
  35. Jeff KingMay 18, 2020
  36. Denton LiuMay 18, 2020
  37. Jeff KingMay 18, 2020
  38. 5/7 remote-curl: error on incomplete packetDenton Liu, May 18, 2020
  39. Jeff KingMay 18, 2020
  40. Denton LiuMay 18, 2020
  41. Jeff KingMay 18, 2020
  42. Denton LiuMay 18, 2020
  43. Jeff KingMay 18, 2020
  44. Junio C HamanoMay 18, 2020
  45. Jeff KingMay 19, 2020
  46. Junio C HamanoMay 18, 2020
  47. 0/7 remote-curl: fix deadlocks when remote server disconnectsDenton Liu, May 19, 2020
  48. 1/7 remote-curl: fix typoDenton Liu, May 19, 2020
  49. 2/7 remote-curl: remove label indentationDenton Liu, May 19, 2020
  50. 3/7 transport: extract common fetch_pack() callDenton Liu, May 19, 2020
  51. 4/7 pkt-line: extern packet_length()Denton Liu, May 19, 2020
  52. Eric SunshineMay 19, 2020
  53. 6/7 pkt-line: define PACKET_READ_RESPONSE_ENDDenton Liu, May 19, 2020
  54. 5/7 remote-curl: error on incomplete packetDenton Liu, May 19, 2020
  55. 7/7 stateless-connect: send response end packetDenton Liu, May 19, 2020
  56. Jeff KingMay 19, 2020
  57. Denton LiuMay 19, 2020
  58. 8/7 fixup! pkt-line: extern packet_length()Denton Liu, May 19, 2020
  59. 9/9 fixup! remote-curl: error on incomplete packetDenton Liu, May 22, 2020
  60. Jeff KingMay 22, 2020
  61. Denton LiuMay 22, 2020
  62. Jeff KingMay 22, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.