From: Patrick Steinhardt' via Git Security Date: Wed, 14 May 2025 06:32:02 +0000 (+0200) Subject: bundle-uri: fix arbitrary file writes via parameter injection X-Git-Tag: v2.43.7~3^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=35cb1bb0b92c132249d932c05bbd860d410e12d4;p=thirdparty%2Fgit.git 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 Based-on-patch-by: David Leadbeater Signed-off-by: Patrick Steinhardt Signed-off-by: Taylor Blau --- diff --git a/bundle-uri.c b/bundle-uri.c index ca32050a78..a6a3c1c4b3 100644 --- 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 996a08e90c..2af523aaa4 100755 --- 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.