[PATCH 0/2] upload-archive security issues

[PATCH 0/2] upload-archive security issues

[Jeff King]

[Note to readers who haven't been following the recent thread on
upload-archive bugs: these security issues are in c09cd77e, which has
not actually been released. So this is "security problems, and we need
fixes before this ships in 1.7.8" and not "OMG your git site is 0wned"].

Looking at Erik's c09cd77e again, there are some serious security
problems, in that we are too lenient with what gets passed to
git-archive, which is not hardened to accept random client arguments.
That lets a client do all sorts of nasty things like running arbitrary
code.

These patches fix it by making cmd_archive handle the remote-request
flag better. An alternative would be to pass only known-good options
through upload-archive. That might be more future-proof, but also
involves upload-archive knowing about the innards of write_archive and
its options.  See also the comments in patch 2/2 for another alternative
fix.

  [1/2]: archive: don't allow negation of --remote-request
  [2/2]: archive: limit ourselves during remote requests

And yes, I feel like a moron for not noticing these problems during my
initial review.

-Peff

[PATCH 1/2] archive: don't allow negation of --remote-request

[Jeff King]

The remote-request flag is a security feature, telling the
spawned git-archive that certain formats should be turned
off. We always place it at the front of the command line
when serving a remote request. Of course, this doesn't do us
any good if the client can simply ask us politely to turn it
off.

This bug was introduced in c09cd77 (upload-archive: use
start_command instead of fork, 2011-10-24), but hasn't yet
been released.

Signed-off-by: Jeff King <peff@peff.net>
---
The other option would be recognizing and disallowing this when reading
arguments from the remote.

 builtin/archive.c   |    2 +-
 t/t5000-tar-tree.sh |   12 ++++++++++++
 2 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index e405566..fce20a1 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -97,7 +97,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 			"path to the remote git-upload-archive command"),
 		{ OPTION_BOOLEAN, 0, "remote-request", &is_remote, NULL,
 			"indicate we are serving a remote request",
-			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
+			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN | PARSE_OPT_NONEG },
 		OPT_END()
 	};
 
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 889842e..723b54e 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -305,6 +305,18 @@ test_expect_success 'only enabled filters are available remotely' '
 	test_cmp remote.bar config.bar
 '
 
+# We have to hand-craft this, since the local "git archive" will
+# eat our "--no-remote-request" argument otherwise.
+test_expect_success 'malicious clients cannot un-remote themselves' '
+	{
+		echo "0021argument --no-remote-request" &&
+		echo "001eargument --format=tar.foo" &&
+		echo "0012argument HEAD" &&
+		printf "0000"
+	} >evil-request &&
+	test_must_fail git upload-archive . <evil-request >remote.tar.foo
+'
+
 if $GZIP --version >/dev/null 2>&1; then
 	test_set_prereq GZIP
 else
-- 
1.7.7.3.8.g38efa

[PATCH 2/2] archive: limit ourselves during remote requests

[Jeff King]

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_archive

The 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(-)

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

Re: [PATCH 2/2] archive: limit ourselves during remote requests

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... 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.

Quite sensible, and a good approach to reroll the series, I would think.

Re: [PATCH 0/2] upload-archive security issues

[Erik Faye-Lund]

On Tue, Nov 15, 2011 at 10:42 PM, Jeff King <peff@peff.net> wrote:
> Looking at Erik's c09cd77e again, there are some serious security
> problems, in that we are too lenient with what gets passed to
> git-archive, which is not hardened to accept random client arguments.
> That lets a client do all sorts of nasty things like running arbitrary
> code.
>
> These patches fix it by making cmd_archive handle the remote-request
> flag better. An alternative would be to pass only known-good options
> through upload-archive. That might be more future-proof, but also
> involves upload-archive knowing about the innards of write_archive and
> its options.  See also the comments in patch 2/2 for another alternative
> fix.
>
>  [1/2]: archive: don't allow negation of --remote-request
>  [2/2]: archive: limit ourselves during remote requests

Yikes! Perhaps the whole deal of rewriting the code to take explicit
file descriptors (and/or dup-bonanza) would have been the better
choice after all?

For the record: I would be fine with c09cd77e simply being reverted
for this release, and having a better version applied in the near
future. Windows support for upload-archive is not worth the risk of
slipping in a remote code execution bug...

>
> And yes, I feel like a moron for not noticing these problems during my
> initial review.

Not only did you fail to spot them, you actually wrote that part of the code ;)

http://article.gmane.org/gmane.comp.version-control.git/178098

(I don't mean to shift blame over to you, I'm the one who should have
spent more time thinking about this as this was "my" series)

Re: [PATCH 0/2] upload-archive security issues

[Jeff King]

On Tue, Nov 15, 2011 at 11:01:09PM +0100, Erik Faye-Lund wrote:

> Yikes! Perhaps the whole deal of rewriting the code to take explicit
> file descriptors (and/or dup-bonanza) would have been the better
> choice after all?

It's certainly simpler. This way is not that hard to fix; we just need
to be more careful about the code path getting from upload-archive into
write_archive.

> For the record: I would be fine with c09cd77e simply being reverted
> for this release, and having a better version applied in the near
> future. Windows support for upload-archive is not worth the risk of
> slipping in a remote code execution bug...

I'd be OK with that, too.

> Not only did you fail to spot them, you actually wrote that part of the code ;)
> 
> http://article.gmane.org/gmane.comp.version-control.git/178098
> 
> (I don't mean to shift blame over to you, I'm the one who should have
> spent more time thinking about this as this was "my" series)

Heh. I did say "something like this" in that message, which is usually a
sure sign I haven't actually thought too hard about the code I'm about
to write. I'll be content to share the blame equally with you. :)

-Peff

Re: [PATCH 0/2] upload-archive security issues

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> For the record: I would be fine with c09cd77e simply being reverted
>> for this release, and having a better version applied in the near
>> future. Windows support for upload-archive is not worth the risk of
>> slipping in a remote code execution bug...
>
> I'd be OK with that, too.

Ok, that is easier ;-)