[PATCH 2/2] archive: limit ourselves during remote requests
Originally, the call-chain of an upload-archive invocation was like:
cmd_archive (on local machine)
run_remote_archiver (local)
cmd_upload_archive (on remote machine)
run_upload_archive (remote)
write_archive (remote)And write_archive knew that it could be remotely invoked, and didn't trust the arguments given to it when doing anything security-critical.
Since c09cd77 (upload-archive: use start_command instead of fork, 2011-10-24), we exec a git-archive subprocess, and the call-chain is now:
cmd_archive (local)
run_remote_archiver (local)
cmd_upload_archive (remote)
cmd_archive (in a sub-process)
write_archiveThe arbitrary arguments we get from the client are passed through cmd_archive via the command-line of git-archive. Unlike write_archive, cmd_archive was never taught not to trust the remote arguments. Among the many horrible things a malicious client could do were:
- accessing another repository as the user running on the
server, using "--remote" - execute arbitrary code as the user running on the server
using "--remote --exec" - overwrite arbitrary files using "--output"
This patch causes cmd_archive to respect the remote-request flag immediately and chain to write_archive, ignoring any other options.
Signed-off-by: Jeff King <peff@peff.net>
---
This is the minimal required to fix it.
I mentioned already the alternative of allowing through only known-good arguments in upload-pack's prepare_argv. I don't like that because it means we have to know about every option that write_archive is OK with.
I think a more sensible solution would be a new command, "git upload-archive--remote", which is just a very stripped-down version of git-archive (i.e., it _only_ calls write_archive). Or it could even be spelled "git upload-archive --remote-request". But the point is that git-archive never needed to worry about security before. We shouldn't be polluting it with security code; we should be bypassing it going to write_archive directly.
For the tests, checking each failure mode is perhaps overkill, but I want to be double sure that this doesn't ever regress.
builtin/archive.c | 9 +++++++++
t/t5000-tar-tree.sh | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+), 0 deletions(-)
Show changes to 2 files +48 −0
builtin/archive.c, t/t5000-tar-tree.sh
diff --git a/builtin/archive.c b/builtin/archive.c
index fce20a1..ee2fb54 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -104,6 +104,15 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, local_opts, NULL,
PARSE_OPT_KEEP_ALL);
+ /*
+ * We want to ignore all other parsed options in the remote case, as
+ * they come from an arbitrary client. Therefore we shouldn't do things
+ * like write files based on --output, or make new --remote
+ * connections.
+ */
+ if (is_remote)
+ return write_archive(argc, argv, prefix, 0, NULL, 1);
+
if (output)
create_output_file(output);
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 723b54e..5679c79 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -317,6 +317,45 @@ test_expect_success 'malicious clients cannot un-remote themselves' '
test_must_fail git upload-archive . <evil-request >remote.tar.foo
'
+# Again, we have to hand-craft our malicious request. Since parsing
+# the output to determine that we did indeed get subrepo would be hard,
+# instead we use an easier test: try to get a branch only in the subrepo,
+# which must fail if our hack doesn't work.
+test_expect_success 'malicious clients cannot access repos via --remote' '
+ git init subrepo &&
+ (cd subrepo &&
+ test_commit subrepo &&
+ git branch only-in-subrepo
+ ) &&
+ {
+ echo "0021argument --remote=../subrepo"
+ echo "001dargument only-in-subrepo" &&
+ printf "0000"
+ } >evil-request &&
+ test_must_fail git upload-archive . <evil-request >evil-output
+'
+
+test_expect_success 'malicious clients cannot exec code via --remote' '
+ {
+ echo "0021argument --remote=../subrepo"
+ echo "0026argument --exec=echo foo >hax0red"
+ echo "0012argument HEAD" &&
+ printf "0000"
+ } >evil-request &&
+ test_might_fail git upload-archive . <evil-request >evil-output &&
+ test_path_is_missing .git/hax0red
+'
+
+test_expect_success 'malicious clients cannot trigger --output on server' '
+ {
+ echo "001dargument --output=p0wn3d"
+ echo "0012argument HEAD" &&
+ printf "0000"
+ } >evil-request &&
+ git upload-archive . <evil-request >remote.tar &&
+ test_path_is_missing .git/p0wn3d
+'
+
if $GZIP --version >/dev/null 2>&1; then
test_set_prereq GZIP
else
--
1.7.7.3.8.g38efa
Jeff King
