Skip to content

Commit b0df0c1

Browse files
Denton-Lgitster
Denton-L
authored and
gitster
committed
stateless-connect: send response end packet
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent 0181b60 commit b0df0c1

File tree

10 files changed

+60
-5
lines changed

10 files changed

+60
-5
lines changed

‎Documentation/gitremote-helpers.txt‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -405,7 +405,9 @@ Supported if the helper has the "connect" capability.
405405
trying to fall back). After line feed terminating the positive
406406
(empty) response, the output of the service starts. Messages
407407
(both request and response) must consist of zero or more
408-
PKT-LINEs, terminating in a flush packet. The client must not
408+
PKT-LINEs, terminating in a flush packet. Response messages will
409+
then have a response end packet after the flush packet to
410+
indicate the end of a response. The client must not
409411
expect the server to store any state in between request-response
410412
pairs. After the connection ends, the remote helper exits.
411413
+

‎Documentation/technical/protocol-v2.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ In protocol v2 these special packets will have the following semantics:
3333

3434
* '0000' Flush Packet (flush-pkt) - indicates the end of a message
3535
* '0001' Delimiter Packet (delim-pkt) - separates sections of a message
36+
* '0002' Message Packet (response-end-pkt) - indicates the end of a response
37+
for stateless connections
3638

3739
Initial Client Request
3840
----------------------

‎builtin/fetch-pack.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
224224
version = discover_version(&reader);
225225
switch (version) {
226226
case protocol_v2:
227-
get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);
227+
get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL, args.stateless_rpc);
228228
break;
229229
case protocol_v1:
230230
case protocol_v0:

‎connect.c‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -406,10 +406,21 @@ static int process_ref_v2(const char *line, struct ref ***list)
406406
return ret;
407407
}
408408

409+
void check_stateless_delimiter(int stateless_rpc,
410+
struct packet_reader *reader,
411+
const char *error)
412+
{
413+
if (!stateless_rpc)
414+
return; /* not in stateless mode, no delimiter expected */
415+
if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)
416+
die("%s", error);
417+
}
418+
409419
struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
410420
struct ref **list, int for_push,
411421
const struct argv_array *ref_prefixes,
412-
const struct string_list *server_options)
422+
const struct string_list *server_options,
423+
int stateless_rpc)
413424
{
414425
int i;
415426
*list = NULL;
@@ -446,6 +457,9 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
446457
if (reader->status != PACKET_READ_FLUSH)
447458
die(_("expected flush after ref listing"));
448459

460+
check_stateless_delimiter(stateless_rpc, reader,
461+
_("expected response end packet after ref listing"));
462+
449463
return list;
450464
}
451465

‎connect.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,4 +22,8 @@ int server_supports_v2(const char *c, int die_on_error);
2222
int server_supports_feature(const char *c, const char *feature,
2323
int die_on_error);
2424

25+
void check_stateless_delimiter(int stateless_rpc,
26+
struct packet_reader *reader,
27+
const char *error);
28+
2529
#endif

‎fetch-pack.c‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1451,6 +1451,13 @@ enum fetch_state {
14511451
FETCH_DONE,
14521452
};
14531453

1454+
static void do_check_stateless_delimiter(const struct fetch_pack_args *args,
1455+
struct packet_reader *reader)
1456+
{
1457+
check_stateless_delimiter(args->stateless_rpc, reader,
1458+
_("git fetch-pack: expected response end packet"));
1459+
}
1460+
14541461
static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
14551462
int fd[2],
14561463
const struct ref *orig_ref,
@@ -1535,13 +1542,18 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
15351542
/* Process ACKs/NAKs */
15361543
switch (process_acks(negotiator, &reader, &common)) {
15371544
case READY:
1545+
/*
1546+
* Don't check for response delimiter; get_pack() will
1547+
* read the rest of this response.
1548+
*/
15381549
state = FETCH_GET_PACK;
15391550
break;
15401551
case COMMON_FOUND:
15411552
in_vain = 0;
15421553
seen_ack = 1;
15431554
/* fallthrough */
15441555
case NO_COMMON_FOUND:
1556+
do_check_stateless_delimiter(args, &reader);
15451557
state = FETCH_SEND_REQUEST;
15461558
break;
15471559
}
@@ -1561,6 +1573,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
15611573
process_section_header(&reader, "packfile", 0);
15621574
if (get_pack(args, fd, pack_lockfile, sought, nr_sought))
15631575
die(_("git fetch-pack: fetch failed."));
1576+
do_check_stateless_delimiter(args, &reader);
15641577

15651578
state = FETCH_DONE;
15661579
break;

‎remote-curl.c‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -703,6 +703,8 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si
703703
state->remaining = packet_length(state->len_buf);
704704
if (state->remaining < 0) {
705705
die(_("remote-curl: bad line length character: %.4s"), state->len_buf);
706+
} else if (state->remaining == 2) {
707+
die(_("remote-curl: unexpected response end packet"));
706708
} else if (state->remaining < 4) {
707709
state->remaining = 0;
708710
} else {
@@ -991,6 +993,9 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
991993
if (rpc_in_data.pktline_state.remaining)
992994
err = error(_("%d bytes of body are still expected"), rpc_in_data.pktline_state.remaining);
993995

996+
if (stateless_connect)
997+
packet_response_end(rpc->in);
998+
994999
curl_slist_free_all(headers);
9951000
free(gzip_body);
9961001
return err;

‎remote.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,8 @@ struct ref **get_remote_heads(struct packet_reader *reader,
179179
struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
180180
struct ref **list, int for_push,
181181
const struct argv_array *ref_prefixes,
182-
const struct string_list *server_options);
182+
const struct string_list *server_options,
183+
int stateless_rpc);
183184

184185
int resolve_remote_symref(struct ref *ref, struct ref *list);
185186

‎t/t5702-protocol-v2.sh‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,6 +620,19 @@ test_expect_success 'clone repository with http:// using protocol v2 with incomp
620620
test_i18ngrep "bytes of body are still expected" err
621621
'
622622

623+
test_expect_success 'clone with http:// using protocol v2 and invalid parameters' '
624+
test_when_finished "rm -f log" &&
625+
626+
test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE_CURL="$(pwd)/log" \
627+
git -c protocol.version=2 \
628+
clone --shallow-since=20151012 "$HTTPD_URL/smart/http_parent" http_child_invalid &&
629+
630+
# Client requested to use protocol v2
631+
grep "Git-Protocol: version=2" log &&
632+
# Server responded using protocol v2
633+
grep "git< version 2" log
634+
'
635+
623636
test_expect_success 'clone big repository with http:// using protocol v2' '
624637
test_when_finished "rm -f log" &&
625638

‎transport.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
297297
if (must_list_refs)
298298
get_remote_refs(data->fd[1], &reader, &refs, for_push,
299299
ref_prefixes,
300-
transport->server_options);
300+
transport->server_options,
301+
transport->stateless_rpc);
301302
break;
302303
case protocol_v1:
303304
case protocol_v0:

0 commit comments

Comments
 (0)