{"thread":{"id":"22222","subject":"Removal of post-upload-hook","startedAt":"2010-01-14T18:01:57Z","lastAt":"2010-02-02T06:15:20Z","messageCount":21,"participants":["Arun Raghavan","Jeff King","Shawn O. Pearce","Robin H. Johnson","Ilari Liusvaara","Nicolas Pitre"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"131658","messageId":"6f8b45101001141001q40d8b746v8385bc6ae37a6af4@mail.gmail.com","threadId":"22222","inReplyTo":null,"subject":"Removal of post-upload-hook","fromName":"Arun Raghavan","fromEmail":"ford_prefect@gentoo.org","sentAt":"2010-01-14T18:01:57Z","receivedAt":"2010-01-14T18:01:57Z","isPatch":false,"sender":{"key":"ford_prefect@gentoo.org","avatar":null},"body":"[I'm not on the list, so please CC me on replies]\n\nHello,\nI noticed that the post-upload hook had been removed in commit\n1456b043fc0f0a395c35d6b5e55b0dad1b6e7acc. The commit message states:\n\n This hook runs after \"git fetch\" in the repository the objects are\n fetched from as the user who fetched, and has security implications.\n\nI was wondering if someone could shed some light (or links) on what\nsecurity implications this hook has?\n\nThanks,\n-- \nArun Raghavan\nhttp://arunraghavan.net/\n(Ford_Prefect | Gentoo) & (arunsr | GNOME)\n"},{"id":"131664","messageId":"20100114193607.GB25863@coredump.intra.peff.net","threadId":"22222","inReplyTo":"6f8b45101001141001q40d8b746v8385bc6ae37a6af4@mail.gmail.com","subject":"Re: Removal of post-upload-hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-14T19:36:07Z","receivedAt":"2010-01-14T19:36:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 14, 2010 at 11:31:57PM +0530, Arun Raghavan wrote:\n\n> [I'm not on the list, so please CC me on replies]\n> \n> Hello,\n> I noticed that the post-upload hook had been removed in commit\n> 1456b043fc0f0a395c35d6b5e55b0dad1b6e7acc. The commit message states:\n> \n> This hook runs after \"git fetch\" in the repository the objects are\n> fetched from as the user who fetched, and has security implications.\n> \n> I was wondering if someone could shed some light (or links) on what\n> security implications this hook has?\n\nBecause receive-pack runs as the user who is pushing, not as the\nrepository owner. So by convincing you to push to my repository in a\nmulti-user environment, I convince you to run some arbitrary code of\nmine.\n\n-Peff\n"},{"id":"131666","messageId":"20100114194107.GA20033@spearce.org","threadId":"22222","inReplyTo":"20100114193607.GB25863@coredump.intra.peff.net","subject":"Re: Removal of post-upload-hook","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-01-14T19:41:07Z","receivedAt":"2010-01-14T19:41:07Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Jeff King wrote:\n> On Thu, Jan 14, 2010 at 11:31:57PM +0530, Arun Raghavan wrote:\n> > [I'm not on the list, so please CC me on replies]\n> > \n> > Hello,\n> > I noticed that the post-upload hook had been removed in commit\n> > 1456b043fc0f0a395c35d6b5e55b0dad1b6e7acc. The commit message states:\n> > \n> > This hook runs after \"git fetch\" in the repository the objects are\n> > fetched from as the user who fetched, and has security implications.\n> > \n> > I was wondering if someone could shed some light (or links) on what\n> > security implications this hook has?\n> \n> Because receive-pack runs as the user who is pushing, not as the\n> repository owner. So by convincing you to push to my repository in a\n> multi-user environment, I convince you to run some arbitrary code of\n> mine.\n\nUhhh, this was in fetch/upload-pack Peff, not push/receive-pack.\n\nSame issue though.\n\n-- \nShawn.\n"},{"id":"131669","messageId":"6f8b45101001141152x35c206b4q1591254c35002193@mail.gmail.com","threadId":"22222","inReplyTo":"20100114194107.GA20033@spearce.org","subject":"Re: Removal of post-upload-hook","fromName":"Arun Raghavan","fromEmail":"ford_prefect@gentoo.org","sentAt":"2010-01-14T19:52:18Z","receivedAt":"2010-01-14T19:52:18Z","isPatch":false,"sender":{"key":"ford_prefect@gentoo.org","avatar":null},"body":"2010/1/15 Shawn O. Pearce :\n> Jeff King wrote:\n>> On Thu, Jan 14, 2010 at 11:31:57PM +0530, Arun Raghavan wrote:\n>> > [I'm not on the list, so please CC me on replies]\n>> >\n>> > Hello,\n>> > I noticed that the post-upload hook had been removed in commit\n>> > 1456b043fc0f0a395c35d6b5e55b0dad1b6e7acc. The commit message states:\n>> >\n>> >     This hook runs after \"git fetch\" in the repository the objects are\n>> >     fetched from as the user who fetched, and has security implications.\n>> >\n>> > I was wondering if someone could shed some light (or links) on what\n>> > security implications this hook has?\n>>\n>> Because receive-pack runs as the user who is pushing, not as the\n>> repository owner. So by convincing you to push to my repository in a\n>> multi-user environment, I convince you to run some arbitrary code of\n>> mine.\n>\n> Uhhh, this was in fetch/upload-pack Peff, not push/receive-pack.\n>\n> Same issue though.\n\nAh, got it - thank you!\n-- \nArun Raghavan\nhttp://arunraghavan.net/\n(Ford_Prefect | Gentoo) & (arunsr | GNOME)\n"},{"id":"131673","messageId":"20100114204305.GC26883@coredump.intra.peff.net","threadId":"22222","inReplyTo":"20100114194107.GA20033@spearce.org","subject":"Re: Removal of post-upload-hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-14T20:43:05Z","receivedAt":"2010-01-14T20:43:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 14, 2010 at 11:41:07AM -0800, Shawn O. Pearce wrote:\n\n> > Because receive-pack runs as the user who is pushing, not as the\n> > repository owner. So by convincing you to push to my repository in a\n> > multi-user environment, I convince you to run some arbitrary code of\n> > mine.\n> \n> Uhhh, this was in fetch/upload-pack Peff, not push/receive-pack.\n> \n> Same issue though.\n\nErrr...yeah. Sorry for the confusion. But yes, it's the same mechanism,\nexcept that it is even easier to get people to pull from you (to get\nthem to push, you first have to get them to write a worthwhile code\ncontribution. ;) ).\n\n-Peff\n"},{"id":"131677","messageId":"20100114210645.GE16921@orbis-terrarum.net","threadId":"22222","inReplyTo":"20100114204305.GC26883@coredump.intra.peff.net","subject":"Re: Removal of post-upload-hook","fromName":"Robin H. Johnson","fromEmail":"robbat2@gentoo.org","sentAt":"2010-01-14T21:06:45Z","receivedAt":"2010-01-14T21:06:45Z","isPatch":false,"sender":{"key":"robbat2@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/373898?v=4"},"body":"On Thu, Jan 14, 2010 at 03:43:05PM -0500, Jeff King wrote:\n> On Thu, Jan 14, 2010 at 11:41:07AM -0800, Shawn O. Pearce wrote:\n> \n> > > Because receive-pack runs as the user who is pushing, not as the\n> > > repository owner. So by convincing you to push to my repository in a\n> > > multi-user environment, I convince you to run some arbitrary code of\n> > > mine.\n> > \n> > Uhhh, this was in fetch/upload-pack Peff, not push/receive-pack.\n> > \n> > Same issue though.\n> Errr...yeah. Sorry for the confusion. But yes, it's the same mechanism,\n> except that it is even easier to get people to pull from you (to get\n> them to push, you first have to get them to write a worthwhile code\n> contribution. ;) ).\npost-update, post-receive, update, pre-receive would all be subject to\nthis problem as well if:\n- the repo was group/world-writable\n- the hook is untrusted\n\npost-upload-pack just required group/world-readable and untrusted hook\ncode.\n\nI'd like to lodge a complaint about the removal of the functionality. I\nwould have commented on the patch prior to this, but even searching I\ndidn't see it cross the list.\n\nAs a reasonable middle ground between the functionality and complete\nremoval, can we find a way just to only execute the potentially\ndangerous hooks under known safe conditions or when explicitly requested\nby the user.\n\nPlaces where the hooks are safe:\n- the hooks are known trusted AND not writable by the user/group.\n (e.g. \"chown -R root:root hooks/\").\n- Systems where the users/groups do not have full shell access, just\n access to run Git itself. Eg gitosis, regular git+ssh:// w/ a\n restricted shell.\n\nUpcoming use case:\nFor Gentoo's work on migrating to Git, we've been working on a\npre-upload-pack hook and script that can explicitly block the generation\nof some packs.\nBasically, unless you send a sufficiently recent 'have' line, you are\ntold to fetch a bundle via HTTP or rsync instead.\n\n-- \nRobin Hugh Johnson\nGentoo Linux: Developer, Trustee & Infrastructure Lead\nE-Mail : robbat2@gentoo.org\nGnuPG FP : 11AC BA4F 4778 E3F6 E4ED F38E B27B 944E 3488 4E85\n"},{"id":"131702","messageId":"6f8b45101001142212i4151c625k54b450cd5978f158@mail.gmail.com","threadId":"22222","inReplyTo":"20100114204305.GC26883@coredump.intra.peff.net","subject":"Re: Removal of post-upload-hook","fromName":"Arun Raghavan","fromEmail":"ford_prefect@gentoo.org","sentAt":"2010-01-15T06:12:19Z","receivedAt":"2010-01-15T06:12:19Z","isPatch":false,"sender":{"key":"ford_prefect@gentoo.org","avatar":null},"body":"2010/1/15 Jeff King :\n> On Thu, Jan 14, 2010 at 11:41:07AM -0800, Shawn O. Pearce wrote:\n>\n>> > Because receive-pack runs as the user who is pushing, not as the\n>> > repository owner. So by convincing you to push to my repository in a\n>> > multi-user environment, I convince you to run some arbitrary code of\n>> > mine.\n>>\n>> Uhhh, this was in fetch/upload-pack Peff, not push/receive-pack.\n>>\n>> Same issue though.\n>\n> Errr...yeah. Sorry for the confusion. But yes, it's the same mechanism,\n> except that it is even easier to get people to pull from you (to get\n> them to push, you first have to get them to write a worthwhile code\n> contribution. ;) ).\n\n:)\n\nAnother thought - would it be acceptable to have a config option to\nenable/disable these types of hooks, so that people who are not\naffected by the problem or explicitly don't care can use them? Perhaps\na core.allowInsecureHooks ?\n\nCheers,\n-- \nArun Raghavan\nhttp://arunraghavan.net/\n(Ford_Prefect | Gentoo) & (arunsr | GNOME)\n"},{"id":"131715","messageId":"20100115115212.GA9221@Knoppix","threadId":"22222","inReplyTo":"6f8b45101001142212i4151c625k54b450cd5978f158@mail.gmail.com","subject":"Re: Removal of post-upload-hook","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-01-15T11:52:12Z","receivedAt":"2010-01-15T11:52:12Z","isPatch":false,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Fri, Jan 15, 2010 at 11:42:19AM +0530, Arun Raghavan wrote:\n> \n> Another thought - would it be acceptable to have a config option to\n> enable/disable these types of hooks, so that people who are not\n> affected by the problem or explicitly don't care can use them? Perhaps\n> a core.allowInsecureHooks ?\n\nThat enable/disable would have to ignore per-repo configuration, which\nwould make it behave differently from other options. Otherwise attacker\ncould just flip the setting...\n\n-Ilari\n"},{"id":"131716","messageId":"6f8b45101001150414r2661001ep10819b601953c05b@mail.gmail.com","threadId":"22222","inReplyTo":"20100115115212.GA9221@Knoppix","subject":"Re: Removal of post-upload-hook","fromName":"Arun Raghavan","fromEmail":"ford_prefect@gentoo.org","sentAt":"2010-01-15T12:14:58Z","receivedAt":"2010-01-15T12:14:58Z","isPatch":false,"sender":{"key":"ford_prefect@gentoo.org","avatar":null},"body":"2010/1/15 Ilari Liusvaara :\n> On Fri, Jan 15, 2010 at 11:42:19AM +0530, Arun Raghavan wrote:\n>>\n>> Another thought - would it be acceptable to have a config option to\n>> enable/disable these types of hooks, so that people who are not\n>> affected by the problem or explicitly don't care can use them? Perhaps\n>> a core.allowInsecureHooks ?\n>\n> That enable/disable would have to ignore per-repo configuration, which\n> would make it behave differently from other options. Otherwise attacker\n> could just flip the setting...\n\nAlternatively, this could just be a build-time switch.\n\n-- \nArun Raghavan\nhttp://arunraghavan.net/\n(Ford_Prefect | Gentoo) & (arunsr | GNOME)\n"},{"id":"131724","messageId":"20100115144736.GC621@coredump.intra.peff.net","threadId":"22222","inReplyTo":"20100114210645.GE16921@orbis-terrarum.net","subject":"Re: Removal of post-upload-hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-15T14:47:36Z","receivedAt":"2010-01-15T14:47:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 14, 2010 at 09:06:45PM +0000, Robin H. Johnson wrote:\n\n> As a reasonable middle ground between the functionality and complete\n> removal, can we find a way just to only execute the potentially\n> dangerous hooks under known safe conditions or when explicitly requested\n> by the user.\n\nAn alternative to ripping it out that was discussed is to check that\ngetuid() matches the owner of the hook.\n\nThat might be a nice improvement in security for the push hooks, as\nwell. But it does come at the cost of some inconvenience. How do you set\nup hooks in a shared central repo that every user should trigger? You\nneed some way to say \"these hooks really _are_ trusted, run them\nanyway\", but that mechanism cannot be in the configuration of the repo\nitself for obvious reasons. I suppose if the owner is root? But that\nleaves no way for non-root users to set up shared access.\n\nAlso, there is a similar issue with config. Right now, if I can convince\nyou to run \"git log\" in a repo whose config I own, I can make you run\narbitrary commands via textconv (and ditto for \"git diff\" and external\ndiff).\n\n> Places where the hooks are safe:\n> - the hooks are known trusted AND not writable by the user/group.\n> (e.g. \"chown -R root:root hooks/\").\n\nThis can work, but has drawbacks. See above.\n\n> - Systems where the users/groups do not have full shell access, just\n> access to run Git itself. Eg gitosis, regular git+ssh:// w/ a\n> restricted shell.\n\nYes, this would work, too, but how do you configure the \"it's OK to run\nrandom hooks\" flag? Environment?\n\n-Peff\n"},{"id":"133232","messageId":"1265013127-12589-1-git-send-email-ford_prefect@gentoo.org","threadId":"22222","inReplyTo":"6f8b45101001150414r2661001ep10819b601953c05b@mail.gmail.com","subject":"[PATCH 0/2] upload-pack: pre- and post- hooks","fromName":"Arun Raghavan","fromEmail":"ford_prefect@gentoo.org","sentAt":"2010-02-01T08:32:05Z","receivedAt":"2010-02-01T08:32:05Z","isPatch":true,"sender":{"key":"ford_prefect@gentoo.org","avatar":null},"body":"Hello!\nThis patch set reintroduces the post-upload-pack hook and adds a\npre-upload-pack hook. These are now only built if 'ALLOW_INSECURE_HOOKS' is set\nat build time. The idea is that only system administrators who need this\nfunctionality and are sure the potential insecurity is not relevant to their\nsystem will enable it.\n\nAt some point if the future, if needed, this could also be made a part of the\nnegotiation between the client and server.\n\nCheers,\nArun\n"},{"id":"133233","messageId":"1265013127-12589-2-git-send-email-ford_prefect@gentoo.org","threadId":"22222","inReplyTo":"1265013127-12589-1-git-send-email-ford_prefect@gentoo.org","subject":"[PATCH 1/2] upload-pack: Reinstate the post-upload-pack hook","fromName":"Arun Raghavan","fromEmail":"ford_prefect@gentoo.org","sentAt":"2010-02-01T08:32:06Z","receivedAt":"2010-02-01T08:32:06Z","isPatch":true,"sender":{"key":"ford_prefect@gentoo.org","avatar":null},"body":"This time, we introduce a build-time flag (ALLOW_INSECURE_HOOKS) to make\nsure that anybody who wants to use these hooks is adequately warned.\n---\n Documentation/git-upload-pack.txt | 2 +\n Documentation/githooks.txt | 34 +++++++++++++++\n Makefile | 8 ++++\n config.mak.in | 1 +\n t/Makefile | 4 ++\n t/t5501-post-upload-pack.sh | 69 ++++++++++++++++++++++++++++++\n upload-pack.c | 85 ++++++++++++++++++++++++++++++++++++-\n 7 files changed, 202 insertions(+), 1 deletions(-)\n create mode 100644 t/t5501-post-upload-pack.sh\n\ndiff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt\nindex b8e49dc..63f3b5c 100644\n--- a/Documentation/git-upload-pack.txt\n+++ b/Documentation/git-upload-pack.txt\n@@ -20,6 +20,8 @@ The UI for the protocol is on the 'git-fetch-pack' side, and the\n program pair is meant to be used to pull updates from a remote\n repository. For push operations, see 'git-send-pack'.\n \n+After finishing the operation successfully, `post-upload-pack`\n+hook is called (see linkgit:githooks[5]).\n \n OPTIONS\n -------\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 29eeae7..47bcfd1 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -310,6 +310,40 @@ Both standard output and standard error output are forwarded to\n 'git-send-pack' on the other end, so you can simply `echo` messages\n for the user.\n \n+post-upload-pack\n+----------------\n+\n+Note that this hook is POTENTIALLY INSECURE. It is run as the user who\n+is pulling, so an attacker can make a victim run arbitrary code by\n+convincing him to clone a repository. To enable this hook, git must be\n+compiled with the ALLOW_INSECURE_HOOKS option.\n+\n+After upload-pack successfully finishes its operation, this hook is called\n+for logging purposes.\n+\n+The hook is passed various pieces of information, one per line, from its\n+standard input. Currently the following items can be fed to the hook, but\n+more types of information may be added in the future:\n+\n+want SHA-1::\n+ 40-byte hexadecimal object name the client asked to include in the\n+ resulting pack. Can occur one or more times in the input.\n+\n+have SHA-1::\n+ 40-byte hexadecimal object name the client asked to exclude from\n+ the resulting pack, claiming to have them already. Can occur zero\n+ or more times in the input.\n+\n+time float::\n+ Number of seconds spent for creating the packfile.\n+\n+size decimal::\n+ Size of the resulting packfile in bytes.\n+\n+kind string:\n+ Either \"clone\" (when the client did not give us any \"have\", and asked\n+ for all our refs with \"want\"), or \"fetch\" (otherwise).\n+\n pre-auto-gc\n ~~~~~~~~~~~\n \ndiff --git a/Makefile b/Makefile\nindex 57045de..e29eb33 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -210,6 +210,10 @@ all::\n # Define JSMIN to point to JavaScript minifier that functions as\n # a filter to have gitweb.js minified.\n #\n+# Define ALLOW_INSECURE_HOOKS to enable hooks that have security implications\n+# in some setups (such as pre-/post-upload hooks that run with the user id of\n+# the user who is pulling).\n+#\n # Define DEFAULT_PAGER to a sensible pager command (defaults to \"less\") if\n # you want to use something different. The value will be interpreted by the\n # shell at runtime when it is used.\n@@ -1366,6 +1370,10 @@ ifdef USE_NED_ALLOCATOR\n COMPAT_OBJS += compat/nedmalloc/nedmalloc.o\n endif\n \n+ifdef ALLOW_INSECURE_HOOKS\n+\tBASIC_CFLAGS += -DALLOW_INSECURE_HOOKS\n+endif\n+\n ifeq ($(TCLTK_PATH),)\n NO_TCLTK=NoThanks\n endif\ndiff --git a/config.mak.in b/config.mak.in\nindex 67b12f7..c5bb125 100644\n--- a/config.mak.in\n+++ b/config.mak.in\n@@ -58,3 +58,4 @@ SNPRINTF_RETURNS_BOGUS=@SNPRINTF_RETURNS_BOGUS@\n NO_PTHREADS=@NO_PTHREADS@\n THREADED_DELTA_SEARCH=@THREADED_DELTA_SEARCH@\n PTHREAD_LIBS=@PTHREAD_LIBS@\n+ALLOW_INSECURE_HOOKS=@ALLOW_INSECURE_HOOKS@\ndiff --git a/t/Makefile b/t/Makefile\nindex bd09390..af3c99e 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -16,6 +16,10 @@ SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)\n TSVN = $(wildcard t91[0-9][0-9]-*.sh)\n \n+ifndef ALLOW_INSECURE_HOOKS\n+\tT := $(filter-out t5501-post-upload-pack.sh,$(T))\n+endif\n+\n all: pre-clean\n \t$(MAKE) aggregate-results-and-cleanup\n \ndiff --git a/t/t5501-post-upload-pack.sh b/t/t5501-post-upload-pack.sh\nnew file mode 100644\nindex 0000000..d89fb51\n--- /dev/null\n+++ b/t/t5501-post-upload-pack.sh\n@@ -0,0 +1,69 @@\n+#!/bin/sh\n+\n+test_description='post upload-hook'\n+\n+. ./test-lib.sh\n+\n+LOGFILE=\".git/post-upload-pack-log\"\n+\n+test_expect_success setup '\n+\ttest_commit A &&\n+\ttest_commit B &&\n+\tgit reset --hard A &&\n+\ttest_commit C &&\n+\tgit branch prev B &&\n+\tmkdir -p .git/hooks &&\n+\t{\n+\t\techo \"#!$SHELL_PATH\" &&\n+\t\techo \"cat >post-upload-pack-log\"\n+\t} >\".git/hooks/post-upload-pack\" &&\n+\tchmod +x .git/hooks/post-upload-pack\n+'\n+\n+test_expect_success initial '\n+\trm -fr sub &&\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit fetch --no-tags .. prev\n+\t) &&\n+\twant=$(sed -n \"s/^want //p\" \"$LOGFILE\") &&\n+\ttest \"$want\" = \"$(git rev-parse --verify B)\" &&\n+\t! grep \"^have \" \"$LOGFILE\" &&\n+\tkind=$(sed -n \"s/^kind //p\" \"$LOGFILE\") &&\n+\ttest \"$kind\" = fetch\n+'\n+\n+test_expect_success second '\n+\trm -fr sub &&\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit fetch --no-tags .. prev:refs/remotes/prev &&\n+\t\tgit fetch --no-tags .. master\n+\t) &&\n+\twant=$(sed -n \"s/^want //p\" \"$LOGFILE\") &&\n+\ttest \"$want\" = \"$(git rev-parse --verify C)\" &&\n+\thave=$(sed -n \"s/^have //p\" \"$LOGFILE\") &&\n+\ttest \"$have\" = \"$(git rev-parse --verify B)\" &&\n+\tkind=$(sed -n \"s/^kind //p\" \"$LOGFILE\") &&\n+\ttest \"$kind\" = fetch\n+'\n+\n+test_expect_success all '\n+\trm -fr sub &&\n+\tHERE=$(pwd) &&\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit clone \"file://$HERE/.git\" new\n+\t) &&\n+\tsed -n \"s/^want //p\" \"$LOGFILE\" | sort >actual &&\n+\tgit rev-parse A B C | sort >expect &&\n+\ttest_cmp expect actual &&\n+\t! grep \"^have \" \"$LOGFILE\" &&\n+\tkind=$(sed -n \"s/^kind //p\" \"$LOGFILE\") &&\n+\ttest \"$kind\" = clone\n+'\n+\n+test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex df15181..c992cb4 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -41,6 +41,11 @@ static int use_sideband;\n static int debug_fd;\n static int advertise_refs;\n static int stateless_rpc;\n+#ifdef ALLOW_INSECURE_HOOKS\n+static int allow_insecure_hooks = 1;\n+#else\n+static int allow_insecure_hooks = 0;\n+#endif\n \n static void reset_timeout(void)\n {\n@@ -148,8 +153,69 @@ static int do_rev_list(int fd, void *create_full_pack)\n \treturn 0;\n }\n \n+static int feed_msg_to_hook(int fd, const char *fmt, ...)\n+{\n+\tint cnt;\n+\tchar buf[1024];\n+\tva_list params;\n+\n+\tva_start(params, fmt);\n+\tcnt = vsprintf(buf, fmt, params);\n+\tva_end(params);\n+\treturn write_in_full(fd, buf, cnt) != cnt;\n+}\n+\n+static int feed_obj_to_hook(const char *label, struct object_array *oa, int i, int fd)\n+{\n+\treturn feed_msg_to_hook(fd, \"%s %s\\n\", label,\n+\t\t\t\tsha1_to_hex(oa->objects[i].item->sha1));\n+}\n+\n+static int run_post_upload_pack_hook(size_t total, struct timeval *tv)\n+{\n+\tconst char *argv[2];\n+\tstruct child_process proc;\n+\tint err, i;\n+\n+\targv[0] = \"hooks/post-upload-pack\";\n+\targv[1] = NULL;\n+\n+\tif (access(argv[0], X_OK) < 0)\n+\t\treturn 0;\n+\n+\tif (!allow_insecure_hooks)\n+\t\treturn 1;\n+\n+\tmemset(&proc, 0, sizeof(proc));\n+\tproc.argv = argv;\n+\tproc.in = -1;\n+\tproc.stdout_to_stderr = 1;\n+\terr = start_command(&proc);\n+\tif (err)\n+\t\treturn err;\n+\tfor (i = 0; !err && i < want_obj.nr; i++)\n+\t\terr |= feed_obj_to_hook(\"want\", &want_obj, i, proc.in);\n+\tfor (i = 0; !err && i < have_obj.nr; i++)\n+\t\terr |= feed_obj_to_hook(\"have\", &have_obj, i, proc.in);\n+\tif (!err)\n+\t\terr |= feed_msg_to_hook(proc.in, \"time %ld.%06ld\\n\",\n+\t\t\t\t\t(long)tv->tv_sec, (long)tv->tv_usec);\n+\tif (!err)\n+\t\terr |= feed_msg_to_hook(proc.in, \"size %ld\\n\", (long)total);\n+\tif (!err)\n+\t\terr |= feed_msg_to_hook(proc.in, \"kind %s\\n\",\n+\t\t\t\t\t(nr_our_refs == want_obj.nr && !have_obj.nr)\n+\t\t\t\t\t? \"clone\" : \"fetch\");\n+\tif (close(proc.in))\n+\t\terr = 1;\n+\tif (finish_command(&proc))\n+\t\terr = 1;\n+\treturn err;\n+}\n+\n static void create_pack_file(void)\n {\n+\tstruct timeval start_tv, tv;\n \tstruct async rev_list;\n \tstruct child_process pack_objects;\n \tint create_full_pack = (nr_our_refs == want_obj.nr && !have_obj.nr);\n@@ -158,9 +224,13 @@ static void create_pack_file(void)\n \t\t\"corruption on the remote side.\";\n \tint buffered = -1;\n \tssize_t sz;\n+\tssize_t total_sz;\n \tconst char *argv[10];\n \tint arg = 0;\n \n+\tgettimeofday(&start_tv, NULL);\n+\ttotal_sz = 0;\n+\n \tif (shallow_nr) {\n \t\trev_list.proc = do_rev_list;\n \t\trev_list.data = 0;\n@@ -286,7 +356,7 @@ static void create_pack_file(void)\n \t\t\tsz = xread(pack_objects.out, cp,\n \t\t\t\t sizeof(data) - outsz);\n \t\t\tif (0 < sz)\n-\t\t\t\t;\n+\t\t\t\ttotal_sz += sz;\n \t\t\telse if (sz == 0) {\n \t\t\t\tclose(pack_objects.out);\n \t\t\t\tpack_objects.out = -1;\n@@ -323,6 +393,19 @@ static void create_pack_file(void)\n \t}\n \tif (use_sideband)\n \t\tpacket_flush(1);\n+\n+\tif (allow_insecure_hooks) {\n+\t\tgettimeofday(&tv, NULL);\n+\t\ttv.tv_sec -= start_tv.tv_sec;\n+\t\tif (tv.tv_usec < start_tv.tv_usec) {\n+\t\t\ttv.tv_sec--;\n+\t\t\ttv.tv_usec += 1000000;\n+\t\t}\n+\t\ttv.tv_usec -= start_tv.tv_usec;\n+\t\tif (run_upload_pack_hook(1, total_sz, &tv))\n+\t\t\twarning(\"Running post-upload-hook failed\");\n+\t}\n+\n \treturn;\n \n fail:\n-- \n1.6.6.1\n"},{"id":"133234","messageId":"1265013127-12589-3-git-send-email-ford_prefect@gentoo.org","threadId":"22222","inReplyTo":"1265013127-12589-2-git-send-email-ford_prefect@gentoo.org","subject":"[PATCH 2/2] upload-pack: Add a pre-upload-pack hook","fromName":"Arun Raghavan","fromEmail":"ford_prefect@gentoo.org","sentAt":"2010-02-01T08:32:07Z","receivedAt":"2010-02-01T08:32:07Z","isPatch":true,"sender":{"key":"ford_prefect@gentoo.org","avatar":null},"body":"This hook is run after want/have are communicated and before the actual\nupload operation is begun. It is passed the set of want and have, as\nwell as the type of operation (fetch/clone). The intended use for this\nhook is to reject large uploads (such as very large initial clones).\n---\n Documentation/git-upload-pack.txt | 5 +-\n Documentation/githooks.txt | 37 ++++++++++--\n t/Makefile | 1 +\n t/t5507-pre-upload-pack.sh | 93 +++++++++++++++++++++++++++++++\n templates/hooks--pre-upload-pack.sample | 11 ++++\n upload-pack.c | 20 +++++--\n 6 files changed, 153 insertions(+), 14 deletions(-)\n create mode 100644 t/t5507-pre-upload-pack.sh\n create mode 100644 templates/hooks--pre-upload-pack.sample\n\ndiff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt\nindex 63f3b5c..5c9474d 100644\n--- a/Documentation/git-upload-pack.txt\n+++ b/Documentation/git-upload-pack.txt\n@@ -20,8 +20,11 @@ The UI for the protocol is on the 'git-fetch-pack' side, and the\n program pair is meant to be used to pull updates from a remote\n repository. For push operations, see 'git-send-pack'.\n \n+Before starting the upload operation, `pre-upload-pack`hook may be\n+called (see linkgit:githooks[5]).\n+\n After finishing the operation successfully, `post-upload-pack`\n-hook is called (see linkgit:githooks[5]).\n+hook may be called (see linkgit:githooks[5]).\n \n OPTIONS\n -------\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 47bcfd1..99f8882 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -310,16 +310,18 @@ Both standard output and standard error output are forwarded to\n 'git-send-pack' on the other end, so you can simply `echo` messages\n for the user.\n \n-post-upload-pack\n-----------------\n+pre-upload-pack\n+---------------\n \n-Note that this hook is POTENTIALLY INSECURE. It is run as the user who\n+Note that this hook is POTENTIALLY INSECURE on shared systems where\n+the owner of the repository is not trusted. It is run as the user who\n is pulling, so an attacker can make a victim run arbitrary code by\n-convincing him to clone a repository. To enable this hook, git must be\n-compiled with the ALLOW_INSECURE_HOOKS option.\n+convincing him to clone a repository. To enable this hook, git must\n+be compiled with the ALLOW_INSECURE_HOOKS option.\n \n-After upload-pack successfully finishes its operation, this hook is called\n-for logging purposes.\n+Before the upload-pack is started (but after want/have have been\n+communicated), this hook is be called. It can be used, for example,\n+to deny very large uploads.\n \n The hook is passed various pieces of information, one per line, from its\n standard input. Currently the following items can be fed to the hook, but\n@@ -334,6 +336,27 @@ have SHA-1::\n the resulting pack, claiming to have them already. Can occur zero\n or more times in the input.\n \n+kind string:\n+ Either \"clone\" (when the client did not give us any \"have\", and asked\n+ for all our refs with \"want\"), or \"fetch\" (otherwise).\n+\n+post-upload-pack\n+----------------\n+\n+The same SECURITY CONCERNS as pre-upload-pack apply here.\n+\n+After upload-pack successfully finishes its operation, this hook is called\n+(for example, for logging).\n+\n+want SHA-1::\n+ 40-byte hexadecimal object name the client asked to include in the\n+ resulting pack. Can occur one or more times in the input.\n+\n+have SHA-1::\n+ 40-byte hexadecimal object name the client asked to exclude from\n+ the resulting pack, claiming to have them already. Can occur zero\n+ or more times in the input.\n+\n time float::\n Number of seconds spent for creating the packfile.\n \ndiff --git a/t/Makefile b/t/Makefile\nindex af3c99e..a884e75 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -18,6 +18,7 @@ TSVN = $(wildcard t91[0-9][0-9]-*.sh)\n \n ifndef ALLOW_INSECURE_HOOKS\n \tT := $(filter-out t5501-post-upload-pack.sh,$(T))\n+\tT := $(filter-out t5507-pre-upload-pack.sh,$(T))\n endif\n \n all: pre-clean\ndiff --git a/t/t5507-pre-upload-pack.sh b/t/t5507-pre-upload-pack.sh\nnew file mode 100644\nindex 0000000..d3a7ba7\n--- /dev/null\n+++ b/t/t5507-pre-upload-pack.sh\n@@ -0,0 +1,93 @@\n+#!/bin/sh\n+\n+test_description='pre upload-hook'\n+\n+. ./test-lib.sh\n+\n+LOGFILE=\".git/pre-upload-pack-log\"\n+\n+test_expect_success setup '\n+\ttest_commit A &&\n+\ttest_commit B &&\n+\tgit reset --hard A &&\n+\ttest_commit C &&\n+\tgit branch prev B &&\n+\tmkdir -p .git/hooks &&\n+\t{\n+\t\techo \"#!$SHELL_PATH\" &&\n+\t\techo \"cat >pre-upload-pack-log\"\n+\t} >\".git/hooks/pre-upload-pack\" &&\n+\tchmod +x .git/hooks/pre-upload-pack\n+'\n+\n+test_expect_success initial '\n+\trm -fr sub &&\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit fetch --no-tags .. prev\n+\t) &&\n+\twant=$(sed -n \"s/^want //p\" \"$LOGFILE\") &&\n+\ttest \"$want\" = \"$(git rev-parse --verify B)\" &&\n+\t! grep \"^have \" \"$LOGFILE\" &&\n+\tkind=$(sed -n \"s/^kind //p\" \"$LOGFILE\") &&\n+\ttest \"$kind\" = fetch\n+'\n+\n+test_expect_success second '\n+\trm -fr sub &&\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit fetch --no-tags .. prev:refs/remotes/prev &&\n+\t\tgit fetch --no-tags .. master\n+\t) &&\n+\twant=$(sed -n \"s/^want //p\" \"$LOGFILE\") &&\n+\ttest \"$want\" = \"$(git rev-parse --verify C)\" &&\n+\thave=$(sed -n \"s/^have //p\" \"$LOGFILE\") &&\n+\ttest \"$have\" = \"$(git rev-parse --verify B)\" &&\n+\tkind=$(sed -n \"s/^kind //p\" \"$LOGFILE\") &&\n+\ttest \"$kind\" = fetch\n+'\n+\n+test_expect_success all '\n+\trm -fr sub &&\n+\tHERE=$(pwd) &&\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit clone \"file://$HERE/.git\" new\n+\t) &&\n+\tsed -n \"s/^want //p\" \"$LOGFILE\" | sort >actual &&\n+\tgit rev-parse A B C | sort >expect &&\n+\ttest_cmp expect actual &&\n+\t! grep \"^have \" \"$LOGFILE\" &&\n+\tkind=$(sed -n \"s/^kind //p\" \"$LOGFILE\") &&\n+\ttest \"$kind\" = clone\n+'\n+\n+cat > pre-upload-pack < file &&\n+\t\tgit add file &&\n+\t\tgit commit -m initial\n+\t) &&\n+\ttest_must_fail git clone -n \"file://$(pwd)/src\" dst\n+\n+'\n+\n+test_done\ndiff --git a/templates/hooks--pre-upload-pack.sample b/templates/hooks--pre-upload-pack.sample\nnew file mode 100644\nindex 0000000..7342d23\n--- /dev/null\n+++ b/templates/hooks--pre-upload-pack.sample\n@@ -0,0 +1,11 @@\n+#!/bin/sh\n+\n+# This sample shows how one may reject an upload-pack where the client is\n+# trying to perform an initial clone clone\n+\n+kind=$(awk '/^kind /{print $2; exit}' -)\n+\n+if test \"$kind\" = \"clone\"; then\n+ echo \"Sorry, the clone operation is not allowed\"\n+ exit 1\n+fi\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c992cb4..9c33e63 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -171,14 +171,19 @@ static int feed_obj_to_hook(const char *label, struct object_array *oa, int i, i\n \t\t\t\tsha1_to_hex(oa->objects[i].item->sha1));\n }\n \n-static int run_post_upload_pack_hook(size_t total, struct timeval *tv)\n+static int run_upload_pack_hook(int post, size_t total, struct timeval *tv)\n {\n \tconst char *argv[2];\n \tstruct child_process proc;\n \tint err, i;\n \n-\targv[0] = \"hooks/post-upload-pack\";\n-\targv[1] = NULL;\n+\tif (!post) {\n+\t\targv[0] = \"hooks/pre-upload-pack\";\n+\t\targv[1] = NULL;\n+\t} else {\n+\t\targv[0] = \"hooks/post-upload-pack\";\n+\t\targv[1] = NULL;\n+\t}\n \n \tif (access(argv[0], X_OK) < 0)\n \t\treturn 0;\n@@ -197,10 +202,10 @@ static int run_post_upload_pack_hook(size_t total, struct timeval *tv)\n \t\terr |= feed_obj_to_hook(\"want\", &want_obj, i, proc.in);\n \tfor (i = 0; !err && i < have_obj.nr; i++)\n \t\terr |= feed_obj_to_hook(\"have\", &have_obj, i, proc.in);\n-\tif (!err)\n+\tif (!err && post)\n \t\terr |= feed_msg_to_hook(proc.in, \"time %ld.%06ld\\n\",\n \t\t\t\t\t(long)tv->tv_sec, (long)tv->tv_usec);\n-\tif (!err)\n+\tif (!err && post)\n \t\terr |= feed_msg_to_hook(proc.in, \"size %ld\\n\", (long)total);\n \tif (!err)\n \t\terr |= feed_msg_to_hook(proc.in, \"kind %s\\n\",\n@@ -758,7 +763,10 @@ static void upload_pack(void)\n \treceive_needs();\n \tif (want_obj.nr) {\n \t\tget_common_commits();\n-\t\tcreate_pack_file();\n+\t\tif (run_upload_pack_hook(0, 0, NULL))\n+\t\t\terror(\"pre-upload hook aborted\");\n+\t\telse\n+\t\t\tcreate_pack_file();\n \t}\n }\n \n-- \n1.6.6.1\n"},{"id":"133252","messageId":"20100201152010.GC8916@spearce.org","threadId":"22222","inReplyTo":"1265013127-12589-1-git-send-email-ford_prefect@gentoo.org","subject":"Re: [PATCH 0/2] upload-pack: pre- and post- hooks","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-01T15:20:10Z","receivedAt":"2010-02-01T15:20:10Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Arun Raghavan wrote:\n> This patch set reintroduces the post-upload-pack hook and adds a\n> pre-upload-pack hook. These are now only built if 'ALLOW_INSECURE_HOOKS' is set\n> at build time. The idea is that only system administrators who need this\n> functionality and are sure the potential insecurity is not relevant to their\n> system will enable it.\n\n*sigh*\n\nI guess this is better, having it off by default, but allowing an\nadministrator who needs this feature to build a custom package.\n\nUnfortunately... I'm sure some distro out there is going to think\nthey know how to compile Git better than we do, and enable this by\ndefault, exposing their users to a security hole. Ask the OpenSSL\nproject about how well distros package code... :-\\\n\nI'd like a bit more than just a compile time flag.\n \n> At some point if the future, if needed, this could also be made a part of the\n> negotiation between the client and server.\n\nI'm not sure I follow.\n\nAre you proposing the server advertises that it wants to run hooks,\nand lets the client decide whether or not they should be executed?\n\n-- \nShawn.\n"},{"id":"133256","messageId":"6f8b45101002010750t5541faefv5b4640dfb9949306@mail.gmail.com","threadId":"22222","inReplyTo":"20100201152010.GC8916@spearce.org","subject":"Re: [PATCH 0/2] upload-pack: pre- and post- hooks","fromName":"Arun Raghavan","fromEmail":"ford_prefect@gentoo.org","sentAt":"2010-02-01T15:50:27Z","receivedAt":"2010-02-01T15:50:27Z","isPatch":true,"sender":{"key":"ford_prefect@gentoo.org","avatar":null},"body":"On 1 February 2010 20:50, Shawn O. Pearce wrote:\n> Arun Raghavan wrote:\n>> This patch set reintroduces the post-upload-pack hook and adds a\n>> pre-upload-pack hook. These are now only built if 'ALLOW_INSECURE_HOOKS' is set\n>> at build time. The idea is that only system administrators who need this\n>> functionality and are sure the potential insecurity is not relevant to their\n>> system will enable it.\n>\n> *sigh*\n>\n> I guess this is better, having it off by default, but allowing an\n> administrator who needs this feature to build a custom package.\n>\n> Unfortunately... I'm sure some distro out there is going to think\n> they know how to compile Git better than we do, and enable this by\n> default, exposing their users to a security hole.  Ask the OpenSSL\n> project about how well distros package code...  :-\\\n>\n> I'd like a bit more than just a compile time flag.\n\nI was hoping the all-caps INSECURE in the name would give distributors pause. :)\n\nSuggestions on what else might work?\n\n>> At some point if the future, if needed, this could also be made a part of the\n>> negotiation between the client and server.\n>\n> I'm not sure I follow.\n>\n> Are you proposing the server advertises that it wants to run hooks,\n> and lets the client decide whether or not they should be executed?\n\nSomething like that. I was thinking the client could always advertise\nwhether the it wants to allow the hooks to be executed or not (which\nwould override the default value of the global variable I introduced).\nEither approach would work, though the second is simpler but also\ndumber.\n\nAgain, this might be over-complicating things, which is why I did not\nimplement it. I just wanted to make a note of the fact that this could\nbe done if the need is felt.\n\nCheers,\n-- \nArun Raghavan\nhttp://arunraghavan.net/\n(Ford_Prefect | Gentoo) & (arunsr | GNOME)\n"},{"id":"133257","messageId":"20100201160141.GG8916@spearce.org","threadId":"22222","inReplyTo":"6f8b45101002010750t5541faefv5b4640dfb9949306@mail.gmail.com","subject":"Re: [PATCH 0/2] upload-pack: pre- and post- hooks","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-01T16:01:41Z","receivedAt":"2010-02-01T16:01:41Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Arun Raghavan wrote:\n> On 1 February 2010 20:50, Shawn O. Pearce wrote:\n> > Arun Raghavan wrote:\n> >> This patch set reintroduces the post-upload-pack hook and adds a\n> >> pre-upload-pack hook. These are now only built if 'ALLOW_INSECURE_HOOKS' is set\n> >> at build time. The idea is that only system administrators who need this\n> >> functionality and are sure the potential insecurity is not relevant to their\n> >> system will enable it.\n> >\n> > *sigh*\n> >\n> > I guess this is better, having it off by default, but allowing an\n> > administrator who needs this feature to build a custom package.\n> >\n> > Unfortunately... I'm sure some distro out there is going to think\n> > they know how to compile Git better than we do, and enable this by\n> > default, exposing their users to a security hole. ?Ask the OpenSSL\n> > project about how well distros package code... ?:-\\\n> >\n> > I'd like a bit more than just a compile time flag.\n> \n> I was hoping the all-caps INSECURE in the name would give\n> distributors pause. :)\n> \n> Suggestions on what else might work?\n\nAt one point we were talking about checking the owner of the hook\nscript itself. If it was uid 0 or the current actual user uid,\nthen we run the hook, otherwise we don't.\n\nThat only really works on POSIX platforms, but it does make some\nsense. Root can already screw with you by replacing the binary\nyou are executing, so any hook they own is no more risky than the\ngit-upload-pack you just started.\n\nIf its the actual user uid, then systems like gitosis can still\nmake use of the hook by making the hook owned by the \"git\" user\nthat gitosis is executing all sessions under.\n\nBut mixed user systems, the hook would only run for the user who\ncreated it, and be skipped for everyone else.\n\nI'm not really sure what to do on Win32 here. Everyone is usually\nAdministrator these days which makes the test for \"root\" there\nsomewhat pointless. Maybe its just not enabled on Win32.\n\n\n> >> At some point if the future, if needed, this could also be made a part of the\n> >> negotiation between the client and server.\n> >\n> > I'm not sure I follow.\n> >\n> > Are you proposing the server advertises that it wants to run hooks,\n> > and lets the client decide whether or not they should be executed?\n> \n> Something like that. I was thinking the client could always advertise\n> whether the it wants to allow the hooks to be executed or not (which\n> would override the default value of the global variable I introduced).\n> Either approach would work, though the second is simpler but also\n> dumber.\n> \n> Again, this might be over-complicating things, which is why I did not\n> implement it. I just wanted to make a note of the fact that this could\n> be done if the need is felt.\n\nMy concern with this is, users might disable the hook all of the\ntime, and then servers that actually want the hook (e.g. gentoo's\nuse of the pre-upload-pack to avoid initial clones over git://)\nwould be stuck, just like they are today.\n\nNo, its just not sane to give the user a choice whether or not they\nshould execute something remotely.\n\n-- \nShawn.\n"},{"id":"133262","messageId":"alpine.LFD.2.00.1002011116320.1681@xanadu.home","threadId":"22222","inReplyTo":"20100201152010.GC8916@spearce.org","subject":"Re: [PATCH 0/2] upload-pack: pre- and post- hooks","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-01T16:30:57Z","receivedAt":"2010-02-01T16:30:57Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 1 Feb 2010, Shawn O. Pearce wrote:\n\n> Arun Raghavan wrote:\n> > This patch set reintroduces the post-upload-pack hook and adds a\n> > pre-upload-pack hook. These are now only built if 'ALLOW_INSECURE_HOOKS' is set\n> > at build time. The idea is that only system administrators who need this\n> > functionality and are sure the potential insecurity is not relevant to their\n> > system will enable it.\n> \n> *sigh*\n> \n> I guess this is better, having it off by default, but allowing an\n> administrator who needs this feature to build a custom package.\n> \n> Unfortunately... I'm sure some distro out there is going to think\n> they know how to compile Git better than we do, and enable this by\n> default, exposing their users to a security hole. Ask the OpenSSL\n> project about how well distros package code... :-\\\n> \n> I'd like a bit more than just a compile time flag.\n\nI think such hooks could be allowed only if triggered explicitly by the \nupload-pack caller, such as git-daemon. That's probably the only \nscenario where a useful use case can be justified for them anyway.\n\nAnd of course, to avoid any security problems, the actual hooks must not \nbe provided by the repository owner but provided externally, like from \ngit-daemon, via some upload-pack command line arguments. This way the \nhooks are really controlled by the system administrator managing \ngit-daemon and not by any random git repository owner.\n\nThat should be good enough for all the use cases those hooks were \noriginally designed for.\n\n\nNicolas\n"},{"id":"133263","messageId":"20100201163618.GB9394@spearce.org","threadId":"22222","inReplyTo":"alpine.LFD.2.00.1002011116320.1681@xanadu.home","subject":"Re: [PATCH 0/2] upload-pack: pre- and post- hooks","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-01T16:36:18Z","receivedAt":"2010-02-01T16:36:18Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> On Mon, 1 Feb 2010, Shawn O. Pearce wrote:\n> I think such hooks could be allowed only if triggered explicitly by the \n> upload-pack caller, such as git-daemon. That's probably the only \n> scenario where a useful use case can be justified for them anyway.\n> \n> And of course, to avoid any security problems, the actual hooks must not \n> be provided by the repository owner but provided externally, like from \n> git-daemon, via some upload-pack command line arguments. This way the \n> hooks are really controlled by the system administrator managing \n> git-daemon and not by any random git repository owner.\n> \n> That should be good enough for all the use cases those hooks were \n> originally designed for.\n\nOooh, I like that.\n\nIf the paths to the hooks are passed in on the command line of\ngit-upload-pack, and git-daemon takes those options and passes\nthem through, you're right, we probably get everything we need.\n\nGitosis can still use the hooks if it wants, since it controls\nthe call of git-upload-pack.\n\n-- \nShawn.\n"},{"id":"133320","messageId":"6f8b45101002012150k784b6d78ibffa5092507eee32@mail.gmail.com","threadId":"22222","inReplyTo":"20100201160141.GG8916@spearce.org","subject":"Re: [PATCH 0/2] upload-pack: pre- and post- hooks","fromName":"Arun Raghavan","fromEmail":"ford_prefect@gentoo.org","sentAt":"2010-02-02T05:50:16Z","receivedAt":"2010-02-02T05:50:16Z","isPatch":true,"sender":{"key":"ford_prefect@gentoo.org","avatar":null},"body":"On 1 February 2010 21:31, Shawn O. Pearce wrote:\n> Arun Raghavan wrote:\n>> On 1 February 2010 20:50, Shawn O. Pearce wrote:\n>> > Arun Raghavan wrote:\n[...]\n>> >> At some point if the future, if needed, this could also be made a part of the\n>> >> negotiation between the client and server.\n>> >\n>> > I'm not sure I follow.\n>> >\n>> > Are you proposing the server advertises that it wants to run hooks,\n>> > and lets the client decide whether or not they should be executed?\n>>\n>> Something like that. I was thinking the client could always advertise\n>> whether the it wants to allow the hooks to be executed or not (which\n>> would override the default value of the global variable I introduced).\n>> Either approach would work, though the second is simpler but also\n>> dumber.\n>>\n>> Again, this might be over-complicating things, which is why I did not\n>> implement it. I just wanted to make a note of the fact that this could\n>> be done if the need is felt.\n>\n> My concern with this is, users might disable the hook all of the\n> time, and then servers that actually want the hook (e.g. gentoo's\n> use of the pre-upload-pack to avoid initial clones over git://)\n> would be stuck, just like they are today.\n>\n> No, its just not sane to give the user a choice whether or not they\n> should execute something remotely.\n\nAh, sorry I wasn't clear about this. I've made it so that when\npre-upload-pack fails, the entire operation fails. This makes sense\nbecause pre-upload-pack is meant to check things like \"do we want\nallow the user to get the pack\". For post-upload-pack, failure only\nresults in a warning, since the actual upload is already done and\nthere's not much to do other than log the failure.\n\n-- \nArun Raghavan\nhttp://arunraghavan.net/\n(Ford_Prefect | Gentoo) & (arunsr | GNOME)\n"},{"id":"133321","messageId":"6f8b45101002012152y76bccb65n78235fce170675ef@mail.gmail.com","threadId":"22222","inReplyTo":"20100201163618.GB9394@spearce.org","subject":"Re: [PATCH 0/2] upload-pack: pre- and post- hooks","fromName":"Arun Raghavan","fromEmail":"ford_prefect@gentoo.org","sentAt":"2010-02-02T05:52:18Z","receivedAt":"2010-02-02T05:52:18Z","isPatch":true,"sender":{"key":"ford_prefect@gentoo.org","avatar":null},"body":"On 1 February 2010 22:06, Shawn O. Pearce wrote:\n> Nicolas Pitre wrote:\n>> On Mon, 1 Feb 2010, Shawn O. Pearce wrote:\n>> I think such hooks could be allowed only if triggered explicitly by the\n>> upload-pack caller, such as git-daemon.  That's probably the only\n>> scenario where a useful use case can be justified for them anyway.\n>>\n>> And of course, to avoid any security problems, the actual hooks must not\n>> be provided by the repository owner but provided externally, like from\n>> git-daemon, via some upload-pack command line arguments.  This way the\n>> hooks are really controlled by the system administrator managing\n>> git-daemon and not by any random git repository owner.\n>>\n>> That should be good enough for all the use cases those hooks were\n>> originally designed for.\n>\n> Oooh, I like that.\n>\n> If the paths to the hooks are passed in on the command line of\n> git-upload-pack, and git-daemon takes those options and passes\n> them through, you're right, we probably get everything we need.\n>\n> Gitosis can still use the hooks if it wants, since it controls\n> the call of git-upload-pack.\n\nI can add the uid check before running the hook as well. Is that good\nenough, or would you guys like me to start from scratch with the\ncommand-line argument approach?\n\nCheers,\n-- \nArun Raghavan\nhttp://arunraghavan.net/\n(Ford_Prefect | Gentoo) & (arunsr | GNOME)\n"},{"id":"133322","messageId":"alpine.LFD.2.00.1002020114200.1681@xanadu.home","threadId":"22222","inReplyTo":"6f8b45101002012152y76bccb65n78235fce170675ef@mail.gmail.com","subject":"Re: [PATCH 0/2] upload-pack: pre- and post- hooks","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-02T06:15:20Z","receivedAt":"2010-02-02T06:15:20Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 2 Feb 2010, Arun Raghavan wrote:\n\n> On 1 February 2010 22:06, Shawn O. Pearce wrote:\n> > Nicolas Pitre wrote:\n> >> On Mon, 1 Feb 2010, Shawn O. Pearce wrote:\n> >> I think such hooks could be allowed only if triggered explicitly by the\n> >> upload-pack caller, such as git-daemon.  That's probably the only\n> >> scenario where a useful use case can be justified for them anyway.\n> >>\n> >> And of course, to avoid any security problems, the actual hooks must not\n> >> be provided by the repository owner but provided externally, like from\n> >> git-daemon, via some upload-pack command line arguments.  This way the\n> >> hooks are really controlled by the system administrator managing\n> >> git-daemon and not by any random git repository owner.\n> >>\n> >> That should be good enough for all the use cases those hooks were\n> >> originally designed for.\n> >\n> > Oooh, I like that.\n> >\n> > If the paths to the hooks are passed in on the command line of\n> > git-upload-pack, and git-daemon takes those options and passes\n> > them through, you're right, we probably get everything we need.\n> >\n> > Gitosis can still use the hooks if it wants, since it controls\n> > the call of git-upload-pack.\n> \n> I can add the uid check before running the hook as well. Is that good\n> enough, or would you guys like me to start from scratch with the\n> command-line argument approach?\n\nPlease forget the uid check and go with the command-line argument \napproach. That's the only sane solution.\n\n\nNicolas\n"}]}