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>

diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index f48a031dc346a307aa33aebc62686709a18673f6..93baeeb0295824ca90bf3ba81635e3d0b4470eb1 100644 (file)
--- 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 7e3766cafb39ccc2f37ec59f0dcd728afb58e587..3996d7089162981447cf805d07eaadc59d05d19d 100644 (file)
--- 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 47711000725b4fb7dc504c62cc2d4ac384f7d46b..94b0c89b8241f65e53d9c80a24de330cf35935ac 100644 (file)
--- 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 11c6ec70a0c5718e11bd265b29bda75a0d13d672..0df45a110888e53e4d6f3f86bafc76abcd096297 100644 (file)
--- 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 5f2382e01868042757901a419b5e5f34ad8bb279..235bc66254d4a0b8760cd828339fccee7fac1bd0 100644 (file)
--- 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 7eaa19d7c17abeb4d2975d112a79762507a81009..d8bbf45ee27a732469287f99768321640f9d992a 100644 (file)
--- 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 d02cb547e97ee5fef3a1b1af3280ec40bc656d45..75532a8baea8f20eb2c4aaa0f82c287a0dbaf617 100644 (file)
--- 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 11d8719b587767bf4cdf0b5b1eccb9572da2658e..5cc26c1b3b3e1f1e2c97c24088f866de6840eab0 100644 (file)
--- 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 4eb81ba2d49abedb3c2af2e1081c77693f69e9c9..8da65e60deea33a61c65829f4ae79f290e7d2c25 100755 (executable)
--- 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 431a93caef710f754307dbf2d69a1d8c3a9e0ae4..7d50c502adfb6c24be91c8b07260c61a0add5340 100644 (file)
--- 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: