bundle-uri: fix arbitrary file writes via parameter injection

We fetch bundle URIs via `download_https_uri_to_file()`. The logic to
fetch those bundles is not handled in-process, but we instead use a
separate git-remote-https(1) process that performs the fetch for us. The
information about which file should be downloaded and where that file
should be put gets communicated via stdin of that process via a "get"
request. This "get" request has the form "get $uri $file\n\n". As may be
obvious to the reader, this will cause git-remote-https(1) to download
the URI "$uri" and put it into "$file".

The fact that we are using plain spaces and newlines as separators for
the request arguments means that we have to be extra careful with the
respective vaules of these arguments:

  - If "$uri" contained a space we would interpret this as both URI and
    target location.

  - If either "$uri" or "$file" contained a newline we would interpret
    this as a new command.

But we neither quote the arguments such that any characters with special
meaning would be escaped, nor do we verify that none of these special
characters are contained.

If either the URI or file contains a newline character, we are open to
protocol injection attacks. Likewise, if the URI itself contains a
space, then an attacker-controlled URI can lead to partially-controlled
file writes.

Note that the attacker-controlled URIs do not permit completely
arbitrary file writes, but instead allows an attacker to control the
path in which we will write a temporary (e.g., "tmp_uri_XXXXXX")
file.

The result is twofold:

  - By adding a space in "$uri" we can control where exactly a file will
    be written to, including out-of-repository writes. The final
    location is not completely arbitrary, as the injected string will be
    concatenated with the original "$file" path. Furthermore, the name
    of the bundle will be "tmp_uri_XXXXXX", further restricting what an
    adversary would be able to write.

    Also note that is not possible for the URI to contain a newline
    because we end up in `credential_from_url_1()` before we try to
    issue any requests using that URI. As such, it is not possible to
    inject arbitrary commands via the URI.

  - By adding a newline to "$file" we can inject arbitrary commands.
    This gives us full control over where a specific file will be
    written to. Potential attack vectors would be to overwrite hooks,
    but if an adversary were to guess where the user's home directory is
    located they might also easily write e.g. a "~/.profile" file and
    thus cause arbitrary code execution.

    This injection can only become possible when the adversary has full
    control over the target path where a bundle will be downloaded to.
    While this feels unlikely, it is possible to control this path when
    users perform a recursive clone with a ".gitmodules" file that is
    controlled by the adversary.

Luckily though, the use of bundle URIs is not enabled by default in Git
clients (yet): they have to be enabled by setting the `bundle.heuristic`
config key explicitly. As such, the blast radius of this parameter
injection should overall be quite contained.

Fix the issue by rejecting spaces in the URI and newlines in both the
URI and the file. As explained, it shouldn't be required to also
restrict the use of newlines in the URI, as we would eventually die
anyway in `credential_from_url_1()`. But given that we're only one small
step away from arbitrary code execution, let's rather be safe and
restrict newlines in URIs, as well.

Eventually we should probably refactor the way that Git talks with the
git-remote-https(1) subprocess so that it is less fragile. Until then,
these two restrictions should plug the issue.

Reported-by: David Leadbeater <dgl@dgl.cx>
Based-on-patch-by: David Leadbeater <dgl@dgl.cx>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Taylor Blau <me@ttaylorr.com>

diff --git a/bundle-uri.c b/bundle-uri.c
index ca32050a78fd7570adea76db264c5600339caf82..a6a3c1c4b3f05cefedba3709aa545460d3c5bf88 100644 (file)
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -292,6 +292,28 @@ static int download_https_uri_to_file(const char *file, const char *uri)
        struct strbuf line = STRBUF_INIT;
        int found_get = 0;
 
+       /*
+        * The protocol we speak with git-remote-https(1) uses a space to
+        * separate between URI and file, so the URI itself must not contain a
+        * space. If it did, an adversary could change the location where the
+        * downloaded file is being written to.
+        *
+        * Similarly, we use newlines to separate commands from one another.
+        * Consequently, neither the URI nor the file must contain a newline or
+        * otherwise an adversary could inject arbitrary commands.
+        *
+        * TODO: Restricting newlines in the target paths may break valid
+        *       usecases, even if those are a bit more on the esoteric side.
+        *       If this ever becomes a problem we should probably think about
+        *       alternatives. One alternative could be to use NUL-delimited
+        *       requests in git-remote-http(1). Another alternative could be
+        *       to use URL quoting.
+        */
+       if (strpbrk(uri, " \n"))
+               return error("bundle-uri: URI is malformed: '%s'", file);
+       if (strchr(file, '\n'))
+               return error("bundle-uri: filename is malformed: '%s'", file);
+
        strvec_pushl(&cp.args, "git-remote-https", uri, NULL);
        cp.err = -1;
        cp.in = -1;

diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
index 996a08e90c9c2c5b2c37405b1b3082e82d29a7e8..2af523aaa400b1d96466e629b6bc4956904f6919 100755 (executable)
--- a/t/t5558-clone-bundle-uri.sh
+++ b/t/t5558-clone-bundle-uri.sh
@@ -1052,6 +1052,29 @@ test_expect_success 'bundles are downloaded once during fetch --all' '
                trace-mult.txt >bundle-fetches &&
        test_line_count = 1 bundle-fetches
 '
+
+test_expect_success 'bundles with space in URI are rejected' '
+       test_when_finished "rm -rf busted repo" &&
+       mkdir -p "$HOME/busted/ /$HOME/repo/.git/objects/bundles" &&
+       git clone --bundle-uri="$HTTPD_URL/bogus $HOME/busted/" "$HTTPD_URL/smart/fetch.git" repo 2>err &&
+       test_grep "error: bundle-uri: URI is malformed: " err &&
+       find busted -type f >files &&
+       test_must_be_empty files
+'
+
+test_expect_success 'bundles with newline in URI are rejected' '
+       test_when_finished "rm -rf busted repo" &&
+       git clone --bundle-uri="$HTTPD_URL/bogus\nget $HTTPD_URL/bogus $HOME/busted" "$HTTPD_URL/smart/fetch.git" repo 2>err &&
+       test_grep "error: bundle-uri: URI is malformed: " err &&
+       test_path_is_missing "$HOME/busted"
+'
+
+test_expect_success 'bundles with newline in target path are rejected' '
+       git clone --bundle-uri="$HTTPD_URL/bogus" "$HTTPD_URL/smart/fetch.git" "$(printf "escape\nget $HTTPD_URL/bogus .")" 2>err &&
+       test_grep "error: bundle-uri: filename is malformed: " err &&
+       test_path_is_missing escape
+'
+
 # Do not add tests here unless they use the HTTP server, as they will
 # not run unless the HTTP dependencies exist.