{"thread":{"id":"28943","subject":"[PATCH 0/2] upload-archive security issues","startedAt":"2011-11-15T21:42:00Z","lastAt":"2011-11-16T00:03:28Z","messageCount":7,"participants":["Jeff King","Erik Faye-Lund","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"179532","messageId":"20111115214159.GA20457@sigill.intra.peff.net","threadId":"28943","inReplyTo":null,"subject":"[PATCH 0/2] upload-archive security issues","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-15T21:42:00Z","receivedAt":"2011-11-15T21:42:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[Note to readers who haven't been following the recent thread on\nupload-archive bugs: these security issues are in c09cd77e, which has\nnot actually been released. So this is \"security problems, and we need\nfixes before this ships in 1.7.8\" and not \"OMG your git site is 0wned\"].\n\nLooking at Erik's c09cd77e again, there are some serious security\nproblems, in that we are too lenient with what gets passed to\ngit-archive, which is not hardened to accept random client arguments.\nThat lets a client do all sorts of nasty things like running arbitrary\ncode.\n\nThese patches fix it by making cmd_archive handle the remote-request\nflag better. An alternative would be to pass only known-good options\nthrough upload-archive. That might be more future-proof, but also\ninvolves upload-archive knowing about the innards of write_archive and\nits options. See also the comments in patch 2/2 for another alternative\nfix.\n\n [1/2]: archive: don't allow negation of --remote-request\n [2/2]: archive: limit ourselves during remote requests\n\nAnd yes, I feel like a moron for not noticing these problems during my\ninitial review.\n\n-Peff\n"},{"id":"179533","messageId":"20111115214317.GA20624@sigill.intra.peff.net","threadId":"28943","inReplyTo":"20111115214159.GA20457@sigill.intra.peff.net","subject":"[PATCH 1/2] archive: don't allow negation of --remote-request","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-15T21:43:18Z","receivedAt":"2011-11-15T21:43:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The remote-request flag is a security feature, telling the\nspawned git-archive that certain formats should be turned\noff. We always place it at the front of the command line\nwhen serving a remote request. Of course, this doesn't do us\nany good if the client can simply ask us politely to turn it\noff.\n\nThis bug was introduced in c09cd77 (upload-archive: use\nstart_command instead of fork, 2011-10-24), but hasn't yet\nbeen released.\n\nSigned-off-by: Jeff King \n---\nThe other option would be recognizing and disallowing this when reading\narguments from the remote.\n\n builtin/archive.c | 2 +-\n t/t5000-tar-tree.sh | 12 ++++++++++++\n 2 files changed, 13 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex e405566..fce20a1 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -97,7 +97,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \t\t\t\"path to the remote git-upload-archive command\"),\n \t\t{ OPTION_BOOLEAN, 0, \"remote-request\", &is_remote, NULL,\n \t\t\t\"indicate we are serving a remote request\",\n-\t\t\tPARSE_OPT_NOARG | PARSE_OPT_HIDDEN },\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_HIDDEN | PARSE_OPT_NONEG },\n \t\tOPT_END()\n \t};\n \ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 889842e..723b54e 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -305,6 +305,18 @@ test_expect_success 'only enabled filters are available remotely' '\n \ttest_cmp remote.bar config.bar\n '\n \n+# We have to hand-craft this, since the local \"git archive\" will\n+# eat our \"--no-remote-request\" argument otherwise.\n+test_expect_success 'malicious clients cannot un-remote themselves' '\n+\t{\n+\t\techo \"0021argument --no-remote-request\" &&\n+\t\techo \"001eargument --format=tar.foo\" &&\n+\t\techo \"0012argument HEAD\" &&\n+\t\tprintf \"0000\"\n+\t} >evil-request &&\n+\ttest_must_fail git upload-archive . remote.tar.foo\n+'\n+\n if $GZIP --version >/dev/null 2>&1; then\n \ttest_set_prereq GZIP\n else\n-- \n1.7.7.3.8.g38efa\n"},{"id":"179534","messageId":"20111115214840.GB20624@sigill.intra.peff.net","threadId":"28943","inReplyTo":"20111115214159.GA20457@sigill.intra.peff.net","subject":"[PATCH 2/2] archive: limit ourselves during remote requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-15T21:48:40Z","receivedAt":"2011-11-15T21:48:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Originally, the call-chain of an upload-archive invocation\nwas like:\n\n cmd_archive (on local machine)\n run_remote_archiver (local)\n cmd_upload_archive (on remote machine)\n\trun_upload_archive (remote)\n\t write_archive (remote)\n\nAnd write_archive knew that it could be remotely invoked,\nand didn't trust the arguments given to it when doing\nanything security-critical.\n\nSince c09cd77 (upload-archive: use start_command instead of\nfork, 2011-10-24), we exec a git-archive subprocess, and the\ncall-chain is now:\n\n cmd_archive (local)\n run_remote_archiver (local)\n cmd_upload_archive (remote)\n cmd_archive (in a sub-process)\n write_archive\n\nThe arbitrary arguments we get from the client are passed\nthrough cmd_archive via the command-line of git-archive.\nUnlike write_archive, cmd_archive was never taught not to\ntrust the remote arguments. Among the many horrible things a\nmalicious client could do were:\n\n - accessing another repository as the user running on the\n server, using \"--remote\"\n\n - execute arbitrary code as the user running on the server\n using \"--remote --exec\"\n\n - overwrite arbitrary files using \"--output\"\n\nThis patch causes cmd_archive to respect the remote-request\nflag immediately and chain to write_archive, ignoring any\nother options.\n\nSigned-off-by: Jeff King \n---\nThis is the minimal required to fix it.\n\nI mentioned already the alternative of allowing through only known-good\narguments in upload-pack's prepare_argv. I don't like that because it\nmeans we have to know about every option that write_archive is OK with.\n\nI think a more sensible solution would be a new command, \"git\nupload-archive--remote\", which is just a very stripped-down version of\ngit-archive (i.e., it _only_ calls write_archive). Or it could even be\nspelled \"git upload-archive --remote-request\". But the point is that\ngit-archive never needed to worry about security before. We\nshouldn't be polluting it with security code; we should be bypassing it\ngoing to write_archive directly.\n\nFor the tests, checking each failure mode is perhaps overkill, but I\nwant to be double sure that this doesn't ever regress.\n\n builtin/archive.c | 9 +++++++++\n t/t5000-tar-tree.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex fce20a1..ee2fb54 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -104,6 +104,15 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, local_opts, NULL,\n \t\t\t PARSE_OPT_KEEP_ALL);\n \n+\t/*\n+\t * We want to ignore all other parsed options in the remote case, as\n+\t * they come from an arbitrary client. Therefore we shouldn't do things\n+\t * like write files based on --output, or make new --remote\n+\t * connections.\n+\t */\n+\tif (is_remote)\n+\t\treturn write_archive(argc, argv, prefix, 0, NULL, 1);\n+\n \tif (output)\n \t\tcreate_output_file(output);\n \ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 723b54e..5679c79 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -317,6 +317,45 @@ test_expect_success 'malicious clients cannot un-remote themselves' '\n \ttest_must_fail git upload-archive . remote.tar.foo\n '\n \n+# Again, we have to hand-craft our malicious request. Since parsing\n+# the output to determine that we did indeed get subrepo would be hard,\n+# instead we use an easier test: try to get a branch only in the subrepo,\n+# which must fail if our hack doesn't work.\n+test_expect_success 'malicious clients cannot access repos via --remote' '\n+\tgit init subrepo &&\n+\t(cd subrepo &&\n+\t test_commit subrepo &&\n+\t git branch only-in-subrepo\n+\t) &&\n+\t{\n+\t\techo \"0021argument --remote=../subrepo\"\n+\t\techo \"001dargument only-in-subrepo\" &&\n+\t\tprintf \"0000\"\n+\t} >evil-request &&\n+\ttest_must_fail git upload-archive . evil-output\n+'\n+\n+test_expect_success 'malicious clients cannot exec code via --remote' '\n+\t{\n+\t\techo \"0021argument --remote=../subrepo\"\n+\t\techo \"0026argument --exec=echo foo >hax0red\"\n+\t\techo \"0012argument HEAD\" &&\n+\t\tprintf \"0000\"\n+\t} >evil-request &&\n+\ttest_might_fail git upload-archive . evil-output &&\n+\ttest_path_is_missing .git/hax0red\n+'\n+\n+test_expect_success 'malicious clients cannot trigger --output on server' '\n+\t{\n+\t\techo \"001dargument --output=p0wn3d\"\n+\t\techo \"0012argument HEAD\" &&\n+\t\tprintf \"0000\"\n+\t} >evil-request &&\n+\tgit upload-archive . remote.tar &&\n+\ttest_path_is_missing .git/p0wn3d\n+'\n+\n if $GZIP --version >/dev/null 2>&1; then\n \ttest_set_prereq GZIP\n else\n-- \n1.7.7.3.8.g38efa\n"},{"id":"179536","messageId":"CABPQNSZ71==pQvH9nTQ3rD6c1RJiLmkA+k_9KS=T45cQb+PTLA@mail.gmail.com","threadId":"28943","inReplyTo":"20111115214159.GA20457@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] upload-archive security issues","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-11-15T22:01:09Z","receivedAt":"2011-11-15T22:01:09Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Nov 15, 2011 at 10:42 PM, Jeff King wrote:\n> Looking at Erik's c09cd77e again, there are some serious security\n> problems, in that we are too lenient with what gets passed to\n> git-archive, which is not hardened to accept random client arguments.\n> That lets a client do all sorts of nasty things like running arbitrary\n> code.\n>\n> These patches fix it by making cmd_archive handle the remote-request\n> flag better. An alternative would be to pass only known-good options\n> through upload-archive. That might be more future-proof, but also\n> involves upload-archive knowing about the innards of write_archive and\n> its options.  See also the comments in patch 2/2 for another alternative\n> fix.\n>\n>  [1/2]: archive: don't allow negation of --remote-request\n>  [2/2]: archive: limit ourselves during remote requests\n\nYikes! Perhaps the whole deal of rewriting the code to take explicit\nfile descriptors (and/or dup-bonanza) would have been the better\nchoice after all?\n\nFor the record: I would be fine with c09cd77e simply being reverted\nfor this release, and having a better version applied in the near\nfuture. Windows support for upload-archive is not worth the risk of\nslipping in a remote code execution bug...\n\n>\n> And yes, I feel like a moron for not noticing these problems during my\n> initial review.\n\nNot only did you fail to spot them, you actually wrote that part of the code ;)\n\nhttp://article.gmane.org/gmane.comp.version-control.git/178098\n\n(I don't mean to shift blame over to you, I'm the one who should have\nspent more time thinking about this as this was \"my\" series)\n"},{"id":"179537","messageId":"20111115222320.GA28646@sigill.intra.peff.net","threadId":"28943","inReplyTo":"CABPQNSZ71==pQvH9nTQ3rD6c1RJiLmkA+k_9KS=T45cQb+PTLA@mail.gmail.com","subject":"Re: [PATCH 0/2] upload-archive security issues","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-15T22:23:20Z","receivedAt":"2011-11-15T22:23:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 15, 2011 at 11:01:09PM +0100, Erik Faye-Lund wrote:\n\n> Yikes! Perhaps the whole deal of rewriting the code to take explicit\n> file descriptors (and/or dup-bonanza) would have been the better\n> choice after all?\n\nIt's certainly simpler. This way is not that hard to fix; we just need\nto be more careful about the code path getting from upload-archive into\nwrite_archive.\n\n> For the record: I would be fine with c09cd77e simply being reverted\n> for this release, and having a better version applied in the near\n> future. Windows support for upload-archive is not worth the risk of\n> slipping in a remote code execution bug...\n\nI'd be OK with that, too.\n\n> Not only did you fail to spot them, you actually wrote that part of the code ;)\n> \n> http://article.gmane.org/gmane.comp.version-control.git/178098\n> \n> (I don't mean to shift blame over to you, I'm the one who should have\n> spent more time thinking about this as this was \"my\" series)\n\nHeh. I did say \"something like this\" in that message, which is usually a\nsure sign I haven't actually thought too hard about the code I'm about\nto write. I'll be content to share the blame equally with you. :)\n\n-Peff\n"},{"id":"179543","messageId":"7vehx9t0n0.fsf@alter.siamese.dyndns.org","threadId":"28943","inReplyTo":"20111115222320.GA28646@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] upload-archive security issues","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-15T23:40:51Z","receivedAt":"2011-11-15T23:40:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> For the record: I would be fine with c09cd77e simply being reverted\n>> for this release, and having a better version applied in the near\n>> future. Windows support for upload-archive is not worth the risk of\n>> slipping in a remote code execution bug...\n>\n> I'd be OK with that, too.\n\nOk, that is easier ;-)\n"},{"id":"179544","messageId":"7v7h31szlb.fsf@alter.siamese.dyndns.org","threadId":"28943","inReplyTo":"20111115214840.GB20624@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] archive: limit ourselves during remote requests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-16T00:03:28Z","receivedAt":"2011-11-16T00:03:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... just a very stripped-down version of\n> git-archive (i.e., it _only_ calls write_archive). Or it could even be\n> spelled \"git upload-archive --remote-request\". But the point is that\n> git-archive never needed to worry about security before. We\n> shouldn't be polluting it with security code; we should be bypassing it\n> going to write_archive directly.\n\nQuite sensible, and a good approach to reroll the series, I would think.\n"}]}