From 51ca7f89f8de97388e4f7997ba07b71dffdb9178 Mon Sep 17 00:00:00 2001 From: Denton Liu Date: Mon, 18 May 2020 11:47:18 -0400 Subject: [PATCH 1/7] remote-curl: fix typo Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano --- remote-curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/remote-curl.c b/remote-curl.c index 1c9aa3d0ab..6844708f38 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -643,7 +643,7 @@ static size_t rpc_out(void *ptr, size_t eltsize, return 0; } /* - * If avail is non-zerp, the line length for the flush still + * If avail is non-zero, the line length for the flush still * hasn't been fully sent. Proceed with sending the line * length. */ From 04cc91abcbeea60f0ef25c041b412480cd2b1afe Mon Sep 17 00:00:00 2001 From: Denton Liu Date: Mon, 18 May 2020 11:47:19 -0400 Subject: [PATCH 2/7] remote-curl: remove label indentation In the codebase, labels are aligned to the leftmost column. Remove the space-indentation from `free_specs:` to conform to this. Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano --- remote-curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/remote-curl.c b/remote-curl.c index 6844708f38..da3e07184a 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -1276,7 +1276,7 @@ static void parse_push(struct strbuf *buf) if (ret) exit(128); /* error already reported */ - free_specs: +free_specs: argv_array_clear(&specs); } From dde72f94bcba8f84f4ea6523b67302df6638c9c0 Mon Sep 17 00:00:00 2001 From: Denton Liu Date: Tue, 19 May 2020 06:53:56 -0400 Subject: [PATCH 3/7] transport: extract common fetch_pack() call In the switch statement, the difference between the `protocol_v2` and `protocol_v{1,0}` arms is a preparatory call to die_if_server_options() in the latter. The fetch_pack() call is identical in both arms. However, since this fetch_pack() call has so many parameters, it is not immediately obvious that the call is identical in both cases. Extract the common fetch_pack() call out of the switch statement so that code duplication is reduced and the logic is more clear for future readers. While we're at it, rewrite the switch statement as an if-else tower for increased clarity. Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano --- transport.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/transport.c b/transport.c index 15f5ba4e8f..431a93caef 100644 --- a/transport.c +++ b/transport.c @@ -369,24 +369,15 @@ static int fetch_refs_via_pack(struct transport *transport, refs_tmp = handshake(transport, 0, NULL, must_list_refs); } - switch (data->version) { - case protocol_v2: - refs = fetch_pack(&args, data->fd, - refs_tmp ? refs_tmp : transport->remote_refs, - to_fetch, nr_heads, &data->shallow, - &transport->pack_lockfile, data->version); - break; - case protocol_v1: - case protocol_v0: - die_if_server_options(transport); - refs = fetch_pack(&args, data->fd, - refs_tmp ? refs_tmp : transport->remote_refs, - to_fetch, nr_heads, &data->shallow, - &transport->pack_lockfile, data->version); - break; - case protocol_unknown_version: + if (data->version == protocol_unknown_version) BUG("unknown protocol version"); - } + else if (data->version <= protocol_v1) + die_if_server_options(transport); + + refs = fetch_pack(&args, data->fd, + refs_tmp ? refs_tmp : transport->remote_refs, + to_fetch, nr_heads, &data->shallow, + &transport->pack_lockfile, data->version); close(data->fd[0]); close(data->fd[1]); From 101736a14c9ad734fe24efed7513189849cd22eb Mon Sep 17 00:00:00 2001 From: Denton Liu Date: Tue, 19 May 2020 06:53:57 -0400 Subject: [PATCH 4/7] pkt-line: extern packet_length() In a future commit, we will be manually processing packets and we will need to access the length header. In order to simplify this, extern packet_length() so that the logic can be reused. Change the function parameter from `const char *linelen` to `const char lenbuf_hex[4]`. Even though these two types behave identically as function parameters, use the array notation to semantically indicate exactly what this function is expecting as an argument. Also, rename it from linelen to lenbuf_hex as the former sounds like it should be an integral type which is misleading. Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano --- pkt-line.c | 6 +++--- pkt-line.h | 9 +++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/pkt-line.c b/pkt-line.c index a0e87b1e81..3beab1dc6b 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -306,10 +306,10 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size, return ret; } -static int packet_length(const char *linelen) +int packet_length(const char lenbuf_hex[4]) { - int val = hex2chr(linelen); - return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2); + int val = hex2chr(lenbuf_hex); + return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2); } enum packet_read_status packet_read_with_status(int fd, char **src_buffer, diff --git a/pkt-line.h b/pkt-line.h index fef3a0d792..a72af9112b 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -74,6 +74,15 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); int packet_read(int fd, char **src_buffer, size_t *src_len, char *buffer, unsigned size, int options); +/* + * Convert a four hex digit packet line length header into its numeric + * representation. + * + * If lenbuf_hex contains non-hex characters, return -1. Otherwise, return the + * numeric value of the length header. + */ +int packet_length(const char lenbuf_hex[4]); + /* * Read a packetized line into a buffer like the 'packet_read()' function but * returns an 'enum packet_read_status' which indicates the status of the read. From 74b082ad34fe2c727c676dac5c33d5e1e5f5ca56 Mon Sep 17 00:00:00 2001 From: Denton Liu Date: Tue, 19 May 2020 06:53:58 -0400 Subject: [PATCH 5/7] remote-curl: error on incomplete 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 with a partially written packet, remote-curl will blindly send the partially written packet before waiting on more input from fetch-pack. Meanwhile, fetch-pack will read the partial packet and continue reading, expecting more input. This results in a deadlock between the two processes. For a stateless connection, inspect packets before sending them and error out if a packet line packet is incomplete. Helped-by: Jeff King Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano --- remote-curl.c | 59 ++++++++++++++++++- t/lib-httpd.sh | 2 + t/lib-httpd/apache.conf | 8 +++ .../incomplete-body-upload-pack-v2-http.sh | 3 + .../incomplete-length-upload-pack-v2-http.sh | 3 + t/t5702-protocol-v2.sh | 34 +++++++++++ 6 files changed, 106 insertions(+), 3 deletions(-) create mode 100644 t/lib-httpd/incomplete-body-upload-pack-v2-http.sh create mode 100644 t/lib-httpd/incomplete-length-upload-pack-v2-http.sh diff --git a/remote-curl.c b/remote-curl.c index da3e07184a..e020140092 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -679,9 +679,53 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp) } #endif +struct check_pktline_state { + char len_buf[4]; + int len_filled; + int remaining; +}; + +static void check_pktline(struct check_pktline_state *state, const char *ptr, size_t size) +{ + while (size) { + if (!state->remaining) { + int digits_remaining = 4 - state->len_filled; + if (digits_remaining > size) + digits_remaining = size; + memcpy(&state->len_buf[state->len_filled], ptr, digits_remaining); + state->len_filled += digits_remaining; + ptr += digits_remaining; + size -= digits_remaining; + + if (state->len_filled == 4) { + 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 < 4) { + state->remaining = 0; + } else { + state->remaining -= 4; + } + state->len_filled = 0; + } + } + + if (state->remaining) { + int remaining = state->remaining; + if (remaining > size) + remaining = size; + ptr += remaining; + size -= remaining; + state->remaining -= remaining; + } + } +} + struct rpc_in_data { struct rpc_state *rpc; struct active_request_slot *slot; + int check_pktline; + struct check_pktline_state pktline_state; }; /* @@ -702,6 +746,8 @@ static size_t rpc_in(char *ptr, size_t eltsize, return size; if (size) data->rpc->any_written = 1; + if (data->check_pktline) + check_pktline(&data->pktline_state, ptr, size); write_or_die(data->rpc->in, ptr, size); return size; } @@ -778,7 +824,7 @@ static curl_off_t xcurl_off_t(size_t len) * If flush_received is true, do not attempt to read any more; just use what's * in rpc->buf. */ -static int post_rpc(struct rpc_state *rpc, int flush_received) +static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_received) { struct active_request_slot *slot; struct curl_slist *headers = http_copy_default_headers(); @@ -920,6 +966,8 @@ retry: curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in); rpc_in_data.rpc = rpc; rpc_in_data.slot = slot; + rpc_in_data.check_pktline = stateless_connect; + memset(&rpc_in_data.pktline_state, 0, sizeof(rpc_in_data.pktline_state)); curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data); curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0); @@ -936,6 +984,11 @@ retry: if (!rpc->any_written) err = -1; + if (rpc_in_data.pktline_state.len_filled) + err = error(_("%d bytes of length header were received"), rpc_in_data.pktline_state.len_filled); + if (rpc_in_data.pktline_state.remaining) + err = error(_("%d bytes of body are still expected"), rpc_in_data.pktline_state.remaining); + curl_slist_free_all(headers); free(gzip_body); return err; @@ -985,7 +1038,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads, break; rpc->pos = 0; rpc->len = n; - err |= post_rpc(rpc, 0); + err |= post_rpc(rpc, 0, 0); } close(client.in); @@ -1342,7 +1395,7 @@ static int stateless_connect(const char *service_name) BUG("The entire rpc->buf should be larger than LARGE_PACKET_MAX"); if (status == PACKET_READ_EOF) break; - if (post_rpc(&rpc, status == PACKET_READ_FLUSH)) + if (post_rpc(&rpc, 1, status == PACKET_READ_FLUSH)) /* We would have an err here */ break; /* Reset the buffer for next request */ diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index 1449ee95e9..d2edfa4c50 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -129,6 +129,8 @@ install_script () { prepare_httpd() { mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH" cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH" + install_script incomplete-length-upload-pack-v2-http.sh + install_script incomplete-body-upload-pack-v2-http.sh install_script broken-smart-http.sh install_script error-smart-http.sh install_script error.sh diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index 994e5290d6..afa91e38b0 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -117,6 +117,8 @@ Alias /auth/dumb/ www/auth/dumb/ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} SetEnv GIT_HTTP_EXPORT_ALL +ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/ +ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/ ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/ ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1 ScriptAlias /broken_smart/ broken-smart-http.sh/ @@ -126,6 +128,12 @@ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1 Options FollowSymlinks + + Options ExecCGI + + + Options ExecCGI + Options ExecCGI diff --git a/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh new file mode 100644 index 0000000000..90e73ef8d5 --- /dev/null +++ b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh @@ -0,0 +1,3 @@ +printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result" +echo +printf "%s%s" "0079" "45" diff --git a/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh new file mode 100644 index 0000000000..dce552e348 --- /dev/null +++ b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh @@ -0,0 +1,3 @@ +printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result" +echo +printf "%s" "00" diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 5039e66dc4..4eb81ba2d4 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -586,6 +586,40 @@ test_expect_success 'clone with http:// using protocol v2' ' ! grep "Send header: Transfer-Encoding: chunked" log ' +test_expect_success 'clone repository with http:// using protocol v2 with incomplete pktline length' ' + test_when_finished "rm -f log" && + + git init "$HTTPD_DOCUMENT_ROOT_PATH/incomplete_length" && + test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/incomplete_length" file && + + test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE_CURL="$(pwd)/log" git -c protocol.version=2 \ + clone "$HTTPD_URL/smart/incomplete_length" incomplete_length_child 2>err && + + # Client requested to use protocol v2 + grep "Git-Protocol: version=2" log && + # Server responded using protocol v2 + grep "git< version 2" log && + # Client reported appropriate failure + test_i18ngrep "bytes of length header were received" err +' + +test_expect_success 'clone repository with http:// using protocol v2 with incomplete pktline body' ' + test_when_finished "rm -f log" && + + git init "$HTTPD_DOCUMENT_ROOT_PATH/incomplete_body" && + test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/incomplete_body" file && + + test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE_CURL="$(pwd)/log" git -c protocol.version=2 \ + clone "$HTTPD_URL/smart/incomplete_body" incomplete_body_child 2>err && + + # Client requested to use protocol v2 + grep "Git-Protocol: version=2" log && + # Server responded using protocol v2 + grep "git< version 2" log && + # Client reported appropriate failure + test_i18ngrep "bytes of body are still expected" err +' + test_expect_success 'clone big repository with http:// using protocol v2' ' test_when_finished "rm -f log" && From 0181b600a6deb66a346dbcbe8300cf9e2467ebbb Mon Sep 17 00:00:00 2001 From: Denton Liu Date: Tue, 19 May 2020 06:53:59 -0400 Subject: [PATCH 6/7] pkt-line: define PACKET_READ_RESPONSE_END In a future commit, we will use PACKET_READ_RESPONSE_END to separate messages proxied by remote-curl. To prepare for this, add the PACKET_READ_RESPONSE_END enum value. In switch statements that need a case added, die() or BUG() when a PACKET_READ_RESPONSE_END is unexpected. Otherwise, mirror how PACKET_READ_DELIM is implemented (especially in cases where packets are being forwarded). Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano --- connect.c | 2 ++ pkt-line.c | 11 +++++++++++ pkt-line.h | 2 ++ remote-curl.c | 2 ++ serve.c | 2 ++ t/helper/test-pkt-line.c | 4 ++++ 6 files changed, 23 insertions(+) diff --git a/connect.c b/connect.c index 23013c6344..11c6ec70a0 100644 --- a/connect.c +++ b/connect.c @@ -127,6 +127,7 @@ enum protocol_version discover_version(struct packet_reader *reader) die_initial_contact(0); case PACKET_READ_FLUSH: case PACKET_READ_DELIM: + case PACKET_READ_RESPONSE_END: version = protocol_v0; break; case PACKET_READ_NORMAL: @@ -310,6 +311,7 @@ struct ref **get_remote_heads(struct packet_reader *reader, state = EXPECTING_DONE; break; case PACKET_READ_DELIM: + case PACKET_READ_RESPONSE_END: die(_("invalid packet")); } diff --git a/pkt-line.c b/pkt-line.c index 3beab1dc6b..8f9bc68ee2 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -99,6 +99,13 @@ void packet_delim(int fd) die_errno(_("unable to write delim packet")); } +void packet_response_end(int fd) +{ + packet_trace("0002", 4, 1); + if (write_in_full(fd, "0002", 4) < 0) + die_errno(_("unable to write stateless separator packet")); +} + int packet_flush_gently(int fd) { packet_trace("0000", 4, 1); @@ -337,6 +344,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer, packet_trace("0001", 4, 0); *pktlen = 0; return PACKET_READ_DELIM; + } else if (len == 2) { + packet_trace("0002", 4, 0); + *pktlen = 0; + return PACKET_READ_RESPONSE_END; } else if (len < 4) { die(_("protocol error: bad line length %d"), len); } diff --git a/pkt-line.h b/pkt-line.h index a72af9112b..5b373fe4cd 100644 --- a/pkt-line.h +++ b/pkt-line.h @@ -22,6 +22,7 @@ */ void packet_flush(int fd); void packet_delim(int fd); +void packet_response_end(int fd); void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3))); void packet_buf_flush(struct strbuf *buf); void packet_buf_delim(struct strbuf *buf); @@ -94,6 +95,7 @@ enum packet_read_status { PACKET_READ_NORMAL, PACKET_READ_FLUSH, PACKET_READ_DELIM, + PACKET_READ_RESPONSE_END, }; enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len, char *buffer, diff --git a/remote-curl.c b/remote-curl.c index e020140092..d02cb547e9 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -601,6 +601,8 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options, case PACKET_READ_FLUSH: memcpy(buf - 4, "0000", 4); break; + case PACKET_READ_RESPONSE_END: + die(_("remote server sent stateless separator")); } } diff --git a/serve.c b/serve.c index 317256c1a4..c046926ba1 100644 --- a/serve.c +++ b/serve.c @@ -217,6 +217,8 @@ static int process_request(void) state = PROCESS_REQUEST_DONE; break; + case PACKET_READ_RESPONSE_END: + BUG("unexpected stateless separator packet"); } } diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c index 12ca698e17..69152958e5 100644 --- a/t/helper/test-pkt-line.c +++ b/t/helper/test-pkt-line.c @@ -46,6 +46,9 @@ static void unpack(void) case PACKET_READ_DELIM: printf("0001\n"); break; + case PACKET_READ_RESPONSE_END: + printf("0002\n"); + break; } } } @@ -75,6 +78,7 @@ static void unpack_sideband(void) case PACKET_READ_FLUSH: return; case PACKET_READ_DELIM: + case PACKET_READ_RESPONSE_END: break; } } From b0df0c16ead4c5512d506dcbbdf31194d992803c Mon Sep 17 00:00:00 2001 From: Denton Liu Date: Tue, 19 May 2020 06:54:00 -0400 Subject: [PATCH 7/7] 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 Helped-by: Jeff King Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano --- 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 @@ out: 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 7eaa19d7c1..d8bbf45ee2 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1451,6 +1451,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, @@ -1535,6 +1542,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: @@ -1542,6 +1553,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; } @@ -1561,6 +1573,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 @@ retry: 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: