Removal of post-upload-hook

Removal of post-upload-hook

[Arun Raghavan]

[I'm not on the list, so please CC me on replies]

Hello,
I noticed that the post-upload hook had been removed in commit
1456b043fc0f0a395c35d6b5e55b0dad1b6e7acc. The commit message states:

    This hook runs after "git fetch" in the repository the objects are
    fetched from as the user who fetched, and has security implications.

I was wondering if someone could shed some light (or links) on what
security implications this hook has?

Thanks,
-- 
Arun Raghavan
http://arunraghavan.net/
(Ford_Prefect | Gentoo) & (arunsr | GNOME)

Re: Removal of post-upload-hook

[Jeff King]

On Thu, Jan 14, 2010 at 11:31:57PM +0530, Arun Raghavan wrote:

> [I'm not on the list, so please CC me on replies]
> 
> Hello,
> I noticed that the post-upload hook had been removed in commit
> 1456b043fc0f0a395c35d6b5e55b0dad1b6e7acc. The commit message states:
> 
>     This hook runs after "git fetch" in the repository the objects are
>     fetched from as the user who fetched, and has security implications.
> 
> I was wondering if someone could shed some light (or links) on what
> security implications this hook has?

Because receive-pack runs as the user who is pushing, not as the
repository owner. So by convincing you to push to my repository in a
multi-user environment, I convince you to run some arbitrary code of
mine.

-Peff

Re: Removal of post-upload-hook

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> On Thu, Jan 14, 2010 at 11:31:57PM +0530, Arun Raghavan wrote:
> > [I'm not on the list, so please CC me on replies]
> > 
> > Hello,
> > I noticed that the post-upload hook had been removed in commit
> > 1456b043fc0f0a395c35d6b5e55b0dad1b6e7acc. The commit message states:
> > 
> >     This hook runs after "git fetch" in the repository the objects are
> >     fetched from as the user who fetched, and has security implications.
> > 
> > I was wondering if someone could shed some light (or links) on what
> > security implications this hook has?
> 
> Because receive-pack runs as the user who is pushing, not as the
> repository owner. So by convincing you to push to my repository in a
> multi-user environment, I convince you to run some arbitrary code of
> mine.

Uhhh, this was in fetch/upload-pack Peff, not push/receive-pack.

Same issue though.

-- 
Shawn.

Re: Removal of post-upload-hook

[Arun Raghavan]

2010/1/15 Shawn O. Pearce <spearce@spearce.org>:
> Jeff King <peff@peff.net> wrote:
>> On Thu, Jan 14, 2010 at 11:31:57PM +0530, Arun Raghavan wrote:
>> > [I'm not on the list, so please CC me on replies]
>> >
>> > Hello,
>> > I noticed that the post-upload hook had been removed in commit
>> > 1456b043fc0f0a395c35d6b5e55b0dad1b6e7acc. The commit message states:
>> >
>> >     This hook runs after "git fetch" in the repository the objects are
>> >     fetched from as the user who fetched, and has security implications.
>> >
>> > I was wondering if someone could shed some light (or links) on what
>> > security implications this hook has?
>>
>> Because receive-pack runs as the user who is pushing, not as the
>> repository owner. So by convincing you to push to my repository in a
>> multi-user environment, I convince you to run some arbitrary code of
>> mine.
>
> Uhhh, this was in fetch/upload-pack Peff, not push/receive-pack.
>
> Same issue though.

Ah, got it - thank you!
-- 
Arun Raghavan
http://arunraghavan.net/
(Ford_Prefect | Gentoo) & (arunsr | GNOME)

Re: Removal of post-upload-hook

[Jeff King]

On Thu, Jan 14, 2010 at 11:41:07AM -0800, Shawn O. Pearce wrote:

> > Because receive-pack runs as the user who is pushing, not as the
> > repository owner. So by convincing you to push to my repository in a
> > multi-user environment, I convince you to run some arbitrary code of
> > mine.
> 
> Uhhh, this was in fetch/upload-pack Peff, not push/receive-pack.
> 
> Same issue though.

Errr...yeah. Sorry for the confusion. But yes, it's the same mechanism,
except that it is even easier to get people to pull from you (to get
them to push, you first have to get them to write a worthwhile code
contribution. ;) ).

-Peff

Re: Removal of post-upload-hook

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]

On Thu, Jan 14, 2010 at 03:43:05PM -0500, Jeff King wrote:
> On Thu, Jan 14, 2010 at 11:41:07AM -0800, Shawn O. Pearce wrote:
> 
> > > Because receive-pack runs as the user who is pushing, not as the
> > > repository owner. So by convincing you to push to my repository in a
> > > multi-user environment, I convince you to run some arbitrary code of
> > > mine.
> > 
> > Uhhh, this was in fetch/upload-pack Peff, not push/receive-pack.
> > 
> > Same issue though.
> Errr...yeah. Sorry for the confusion. But yes, it's the same mechanism,
> except that it is even easier to get people to pull from you (to get
> them to push, you first have to get them to write a worthwhile code
> contribution. ;) ).
post-update, post-receive, update, pre-receive would all be subject to
this problem as well if:
- the repo was group/world-writable
- the hook is untrusted

post-upload-pack just required group/world-readable and untrusted hook
code.

I'd like to lodge a complaint about the removal of the functionality. I
would have commented on the patch prior to this, but even searching I
didn't see it cross the list.

As a reasonable middle ground between the functionality and complete
removal, can we find a way just to only execute the potentially
dangerous hooks under known safe conditions or when explicitly requested
by the user.

Places where the hooks are safe:
- the hooks are known trusted AND not writable by the user/group.
  (e.g. "chown -R root:root hooks/").
- Systems where the users/groups do not have full shell access, just
  access to run Git itself. Eg gitosis, regular git+ssh:// w/ a
  restricted shell.

Upcoming use case:
For Gentoo's work on migrating to Git, we've been working on a
pre-upload-pack hook and script that can explicitly block the generation
of some packs.
Basically, unless you send a sufficiently recent 'have' line, you are
told to fetch a bundle via HTTP or rsync instead.

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

[-- Attachment #2: Type: application/pgp-signature, Size: 330 bytes --]

Re: Removal of post-upload-hook

[Jeff King]

On Thu, Jan 14, 2010 at 09:06:45PM +0000, Robin H. Johnson wrote:

> As a reasonable middle ground between the functionality and complete
> removal, can we find a way just to only execute the potentially
> dangerous hooks under known safe conditions or when explicitly requested
> by the user.

An alternative to ripping it out that was discussed is to check that
getuid() matches the owner of the hook.

That might be a nice improvement in security for the push hooks, as
well. But it does come at the cost of some inconvenience. How do you set
up hooks in a shared central repo that every user should trigger? You
need some way to say "these hooks really _are_ trusted, run them
anyway", but that mechanism cannot be in the configuration of the repo
itself for obvious reasons. I suppose if the owner is root? But that
leaves no way for non-root users to set up shared access.

Also, there is a similar issue with config. Right now, if I can convince
you to run "git log" in a repo whose config I own, I can make you run
arbitrary commands via textconv (and ditto for "git diff" and external
diff).

> Places where the hooks are safe:
> - the hooks are known trusted AND not writable by the user/group.
>   (e.g. "chown -R root:root hooks/").

This can work, but has drawbacks. See above.

> - Systems where the users/groups do not have full shell access, just
>   access to run Git itself. Eg gitosis, regular git+ssh:// w/ a
>   restricted shell.

Yes, this would work, too, but how do you configure the "it's OK to run
random hooks" flag? Environment?

-Peff

Re: Removal of post-upload-hook

[Arun Raghavan]

2010/1/15 Jeff King <peff@peff.net>:
> On Thu, Jan 14, 2010 at 11:41:07AM -0800, Shawn O. Pearce wrote:
>
>> > Because receive-pack runs as the user who is pushing, not as the
>> > repository owner. So by convincing you to push to my repository in a
>> > multi-user environment, I convince you to run some arbitrary code of
>> > mine.
>>
>> Uhhh, this was in fetch/upload-pack Peff, not push/receive-pack.
>>
>> Same issue though.
>
> Errr...yeah. Sorry for the confusion. But yes, it's the same mechanism,
> except that it is even easier to get people to pull from you (to get
> them to push, you first have to get them to write a worthwhile code
> contribution. ;) ).

:)

Another thought - would it be acceptable to have a config option to
enable/disable these types of hooks, so that people who are not
affected by the problem or explicitly don't care can use them? Perhaps
a core.allowInsecureHooks ?

Cheers,
-- 
Arun Raghavan
http://arunraghavan.net/
(Ford_Prefect | Gentoo) & (arunsr | GNOME)

Re: Removal of post-upload-hook

[Ilari Liusvaara]

On Fri, Jan 15, 2010 at 11:42:19AM +0530, Arun Raghavan wrote:
> 
> Another thought - would it be acceptable to have a config option to
> enable/disable these types of hooks, so that people who are not
> affected by the problem or explicitly don't care can use them? Perhaps
> a core.allowInsecureHooks ?

That enable/disable would have to ignore per-repo configuration, which
would make it behave differently from other options. Otherwise attacker
could just flip the setting...

-Ilari

Re: Removal of post-upload-hook

[Arun Raghavan]

2010/1/15 Ilari Liusvaara <ilari.liusvaara@elisanet.fi>:
> On Fri, Jan 15, 2010 at 11:42:19AM +0530, Arun Raghavan wrote:
>>
>> Another thought - would it be acceptable to have a config option to
>> enable/disable these types of hooks, so that people who are not
>> affected by the problem or explicitly don't care can use them? Perhaps
>> a core.allowInsecureHooks ?
>
> That enable/disable would have to ignore per-repo configuration, which
> would make it behave differently from other options. Otherwise attacker
> could just flip the setting...

Alternatively, this could just be a build-time switch.

-- 
Arun Raghavan
http://arunraghavan.net/
(Ford_Prefect | Gentoo) & (arunsr | GNOME)

[PATCH 0/2] upload-pack: pre- and post- hooks

[Arun Raghavan]

Hello!
This patch set reintroduces the post-upload-pack hook and adds a
pre-upload-pack hook. These are now only built if 'ALLOW_INSECURE_HOOKS' is set
at build time. The idea is that only system administrators who need this
functionality and are sure the potential insecurity is not relevant to their
system will enable it.

At some point if the future, if needed, this could also be made a part of the
negotiation between the client and server.

Cheers,
Arun

[PATCH 1/2] upload-pack: Reinstate the post-upload-pack hook

[Arun Raghavan]

This time, we introduce a build-time flag (ALLOW_INSECURE_HOOKS) to make
sure that anybody who wants to use these hooks is adequately warned.
---
 Documentation/git-upload-pack.txt |    2 +
 Documentation/githooks.txt        |   34 +++++++++++++++
 Makefile                          |    8 ++++
 config.mak.in                     |    1 +
 t/Makefile                        |    4 ++
 t/t5501-post-upload-pack.sh       |   69 ++++++++++++++++++++++++++++++
 upload-pack.c                     |   85 ++++++++++++++++++++++++++++++++++++-
 7 files changed, 202 insertions(+), 1 deletions(-)
 create mode 100644 t/t5501-post-upload-pack.sh

diff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt
index b8e49dc..63f3b5c 100644
--- a/Documentation/git-upload-pack.txt
+++ b/Documentation/git-upload-pack.txt
@@ -20,6 +20,8 @@ The UI for the protocol is on the 'git-fetch-pack' side, and the
 program pair is meant to be used to pull updates from a remote
 repository.  For push operations, see 'git-send-pack'.
 
+After finishing the operation successfully, `post-upload-pack`
+hook is called (see linkgit:githooks[5]).
 
 OPTIONS
 -------
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 29eeae7..47bcfd1 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -310,6 +310,40 @@ Both standard output and standard error output are forwarded to
 'git-send-pack' on the other end, so you can simply `echo` messages
 for the user.
 
+post-upload-pack
+----------------
+
+Note that this hook is POTENTIALLY INSECURE. It is run as the user who
+is pulling, so an attacker can make a victim run arbitrary code by
+convincing him to clone a repository. To enable this hook, git must be
+compiled with the ALLOW_INSECURE_HOOKS option.
+
+After upload-pack successfully finishes its operation, this hook is called
+for logging purposes.
+
+The hook is passed various pieces of information, one per line, from its
+standard input.  Currently the following items can be fed to the hook, but
+more types of information may be added in the future:
+
+want SHA-1::
+    40-byte hexadecimal object name the client asked to include in the
+    resulting pack.  Can occur one or more times in the input.
+
+have SHA-1::
+    40-byte hexadecimal object name the client asked to exclude from
+    the resulting pack, claiming to have them already.  Can occur zero
+    or more times in the input.
+
+time float::
+    Number of seconds spent for creating the packfile.
+
+size decimal::
+    Size of the resulting packfile in bytes.
+
+kind string:
+    Either "clone" (when the client did not give us any "have", and asked
+    for all our refs with "want"), or "fetch" (otherwise).
+
 pre-auto-gc
 ~~~~~~~~~~~
 
diff --git a/Makefile b/Makefile
index 57045de..e29eb33 100644
--- a/Makefile
+++ b/Makefile
@@ -210,6 +210,10 @@ all::
 # Define JSMIN to point to JavaScript minifier that functions as
 # a filter to have gitweb.js minified.
 #
+# Define ALLOW_INSECURE_HOOKS to enable hooks that have security implications
+# in some setups (such as pre-/post-upload hooks that run with the user id of
+# the user who is pulling).
+#
 # Define DEFAULT_PAGER to a sensible pager command (defaults to "less") if
 # you want to use something different.  The value will be interpreted by the
 # shell at runtime when it is used.
@@ -1366,6 +1370,10 @@ ifdef USE_NED_ALLOCATOR
        COMPAT_OBJS += compat/nedmalloc/nedmalloc.o
 endif
 
+ifdef ALLOW_INSECURE_HOOKS
+	BASIC_CFLAGS += -DALLOW_INSECURE_HOOKS
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK=NoThanks
 endif
diff --git a/config.mak.in b/config.mak.in
index 67b12f7..c5bb125 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -58,3 +58,4 @@ SNPRINTF_RETURNS_BOGUS=@SNPRINTF_RETURNS_BOGUS@
 NO_PTHREADS=@NO_PTHREADS@
 THREADED_DELTA_SEARCH=@THREADED_DELTA_SEARCH@
 PTHREAD_LIBS=@PTHREAD_LIBS@
+ALLOW_INSECURE_HOOKS=@ALLOW_INSECURE_HOOKS@
diff --git a/t/Makefile b/t/Makefile
index bd09390..af3c99e 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -16,6 +16,10 @@ SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
 TSVN = $(wildcard t91[0-9][0-9]-*.sh)
 
+ifndef ALLOW_INSECURE_HOOKS
+	T := $(filter-out t5501-post-upload-pack.sh,$(T))
+endif
+
 all: pre-clean
 	$(MAKE) aggregate-results-and-cleanup
 
diff --git a/t/t5501-post-upload-pack.sh b/t/t5501-post-upload-pack.sh
new file mode 100644
index 0000000..d89fb51
--- /dev/null
+++ b/t/t5501-post-upload-pack.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='post upload-hook'
+
+. ./test-lib.sh
+
+LOGFILE=".git/post-upload-pack-log"
+
+test_expect_success setup '
+	test_commit A &&
+	test_commit B &&
+	git reset --hard A &&
+	test_commit C &&
+	git branch prev B &&
+	mkdir -p .git/hooks &&
+	{
+		echo "#!$SHELL_PATH" &&
+		echo "cat >post-upload-pack-log"
+	} >".git/hooks/post-upload-pack" &&
+	chmod +x .git/hooks/post-upload-pack
+'
+
+test_expect_success initial '
+	rm -fr sub &&
+	git init sub &&
+	(
+		cd sub &&
+		git fetch --no-tags .. prev
+	) &&
+	want=$(sed -n "s/^want //p" "$LOGFILE") &&
+	test "$want" = "$(git rev-parse --verify B)" &&
+	! grep "^have " "$LOGFILE" &&
+	kind=$(sed -n "s/^kind //p" "$LOGFILE") &&
+	test "$kind" = fetch
+'
+
+test_expect_success second '
+	rm -fr sub &&
+	git init sub &&
+	(
+		cd sub &&
+		git fetch --no-tags .. prev:refs/remotes/prev &&
+		git fetch --no-tags .. master
+	) &&
+	want=$(sed -n "s/^want //p" "$LOGFILE") &&
+	test "$want" = "$(git rev-parse --verify C)" &&
+	have=$(sed -n "s/^have //p" "$LOGFILE") &&
+	test "$have" = "$(git rev-parse --verify B)" &&
+	kind=$(sed -n "s/^kind //p" "$LOGFILE") &&
+	test "$kind" = fetch
+'
+
+test_expect_success all '
+	rm -fr sub &&
+	HERE=$(pwd) &&
+	git init sub &&
+	(
+		cd sub &&
+		git clone "file://$HERE/.git" new
+	) &&
+	sed -n "s/^want //p" "$LOGFILE" | sort >actual &&
+	git rev-parse A B C | sort >expect &&
+	test_cmp expect actual &&
+	! grep "^have " "$LOGFILE" &&
+	kind=$(sed -n "s/^kind //p" "$LOGFILE") &&
+	test "$kind" = clone
+'
+
+test_done
diff --git a/upload-pack.c b/upload-pack.c
index df15181..c992cb4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -41,6 +41,11 @@ static int use_sideband;
 static int debug_fd;
 static int advertise_refs;
 static int stateless_rpc;
+#ifdef ALLOW_INSECURE_HOOKS
+static int allow_insecure_hooks = 1;
+#else
+static int allow_insecure_hooks = 0;
+#endif
 
 static void reset_timeout(void)
 {
@@ -148,8 +153,69 @@ static int do_rev_list(int fd, void *create_full_pack)
 	return 0;
 }
 
+static int feed_msg_to_hook(int fd, const char *fmt, ...)
+{
+	int cnt;
+	char buf[1024];
+	va_list params;
+
+	va_start(params, fmt);
+	cnt = vsprintf(buf, fmt, params);
+	va_end(params);
+	return write_in_full(fd, buf, cnt) != cnt;
+}
+
+static int feed_obj_to_hook(const char *label, struct object_array *oa, int i, int fd)
+{
+	return feed_msg_to_hook(fd, "%s %s\n", label,
+				sha1_to_hex(oa->objects[i].item->sha1));
+}
+
+static int run_post_upload_pack_hook(size_t total, struct timeval *tv)
+{
+	const char *argv[2];
+	struct child_process proc;
+	int err, i;
+
+	argv[0] = "hooks/post-upload-pack";
+	argv[1] = NULL;
+
+	if (access(argv[0], X_OK) < 0)
+		return 0;
+
+	if (!allow_insecure_hooks)
+		return 1;
+
+	memset(&proc, 0, sizeof(proc));
+	proc.argv = argv;
+	proc.in = -1;
+	proc.stdout_to_stderr = 1;
+	err = start_command(&proc);
+	if (err)
+		return err;
+	for (i = 0; !err && i < want_obj.nr; i++)
+		err |= feed_obj_to_hook("want", &want_obj, i, proc.in);
+	for (i = 0; !err && i < have_obj.nr; i++)
+		err |= feed_obj_to_hook("have", &have_obj, i, proc.in);
+	if (!err)
+		err |= feed_msg_to_hook(proc.in, "time %ld.%06ld\n",
+					(long)tv->tv_sec, (long)tv->tv_usec);
+	if (!err)
+		err |= feed_msg_to_hook(proc.in, "size %ld\n", (long)total);
+	if (!err)
+		err |= feed_msg_to_hook(proc.in, "kind %s\n",
+					(nr_our_refs == want_obj.nr && !have_obj.nr)
+					? "clone" : "fetch");
+	if (close(proc.in))
+		err = 1;
+	if (finish_command(&proc))
+		err = 1;
+	return err;
+}
+
 static void create_pack_file(void)
 {
+	struct timeval start_tv, tv;
 	struct async rev_list;
 	struct child_process pack_objects;
 	int create_full_pack = (nr_our_refs == want_obj.nr && !have_obj.nr);
@@ -158,9 +224,13 @@ static void create_pack_file(void)
 		"corruption on the remote side.";
 	int buffered = -1;
 	ssize_t sz;
+	ssize_t total_sz;
 	const char *argv[10];
 	int arg = 0;
 
+	gettimeofday(&start_tv, NULL);
+	total_sz = 0;
+
 	if (shallow_nr) {
 		rev_list.proc = do_rev_list;
 		rev_list.data = 0;
@@ -286,7 +356,7 @@ static void create_pack_file(void)
 			sz = xread(pack_objects.out, cp,
 				  sizeof(data) - outsz);
 			if (0 < sz)
-				;
+				total_sz += sz;
 			else if (sz == 0) {
 				close(pack_objects.out);
 				pack_objects.out = -1;
@@ -323,6 +393,19 @@ static void create_pack_file(void)
 	}
 	if (use_sideband)
 		packet_flush(1);
+
+	if (allow_insecure_hooks) {
+		gettimeofday(&tv, NULL);
+		tv.tv_sec -= start_tv.tv_sec;
+		if (tv.tv_usec < start_tv.tv_usec) {
+			tv.tv_sec--;
+			tv.tv_usec += 1000000;
+		}
+		tv.tv_usec -= start_tv.tv_usec;
+		if (run_upload_pack_hook(1, total_sz, &tv))
+			warning("Running post-upload-hook failed");
+	}
+
 	return;
 
  fail:
-- 
1.6.6.1

[PATCH 2/2] upload-pack: Add a pre-upload-pack hook

[Arun Raghavan]

This hook is run after want/have are communicated and before the actual
upload operation is begun. It is passed the set of want and have, as
well as the type of operation (fetch/clone). The intended use for this
hook is to reject large uploads (such as very large initial clones).
---
 Documentation/git-upload-pack.txt       |    5 +-
 Documentation/githooks.txt              |   37 ++++++++++--
 t/Makefile                              |    1 +
 t/t5507-pre-upload-pack.sh              |   93 +++++++++++++++++++++++++++++++
 templates/hooks--pre-upload-pack.sample |   11 ++++
 upload-pack.c                           |   20 +++++--
 6 files changed, 153 insertions(+), 14 deletions(-)
 create mode 100644 t/t5507-pre-upload-pack.sh
 create mode 100644 templates/hooks--pre-upload-pack.sample

diff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt
index 63f3b5c..5c9474d 100644
--- a/Documentation/git-upload-pack.txt
+++ b/Documentation/git-upload-pack.txt
@@ -20,8 +20,11 @@ The UI for the protocol is on the 'git-fetch-pack' side, and the
 program pair is meant to be used to pull updates from a remote
 repository.  For push operations, see 'git-send-pack'.
 
+Before starting the upload operation, `pre-upload-pack`hook may be
+called (see linkgit:githooks[5]).
+
 After finishing the operation successfully, `post-upload-pack`
-hook is called (see linkgit:githooks[5]).
+hook may be called (see linkgit:githooks[5]).
 
 OPTIONS
 -------
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 47bcfd1..99f8882 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -310,16 +310,18 @@ Both standard output and standard error output are forwarded to
 'git-send-pack' on the other end, so you can simply `echo` messages
 for the user.
 
-post-upload-pack
-----------------
+pre-upload-pack
+---------------
 
-Note that this hook is POTENTIALLY INSECURE. It is run as the user who
+Note that this hook is POTENTIALLY INSECURE on shared systems where
+the owner of the repository is not trusted. It is run as the user who
 is pulling, so an attacker can make a victim run arbitrary code by
-convincing him to clone a repository. To enable this hook, git must be
-compiled with the ALLOW_INSECURE_HOOKS option.
+convincing him to clone a repository. To enable this hook, git must
+be compiled with the ALLOW_INSECURE_HOOKS option.
 
-After upload-pack successfully finishes its operation, this hook is called
-for logging purposes.
+Before the upload-pack is started (but after want/have have been
+communicated), this hook is be called. It can be used, for example,
+to deny very large uploads.
 
 The hook is passed various pieces of information, one per line, from its
 standard input.  Currently the following items can be fed to the hook, but
@@ -334,6 +336,27 @@ have SHA-1::
     the resulting pack, claiming to have them already.  Can occur zero
     or more times in the input.
 
+kind string:
+    Either "clone" (when the client did not give us any "have", and asked
+    for all our refs with "want"), or "fetch" (otherwise).
+
+post-upload-pack
+----------------
+
+The same SECURITY CONCERNS as pre-upload-pack apply here.
+
+After upload-pack successfully finishes its operation, this hook is called
+(for example, for logging).
+
+want SHA-1::
+    40-byte hexadecimal object name the client asked to include in the
+    resulting pack.  Can occur one or more times in the input.
+
+have SHA-1::
+    40-byte hexadecimal object name the client asked to exclude from
+    the resulting pack, claiming to have them already.  Can occur zero
+    or more times in the input.
+
 time float::
     Number of seconds spent for creating the packfile.
 
diff --git a/t/Makefile b/t/Makefile
index af3c99e..a884e75 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -18,6 +18,7 @@ TSVN = $(wildcard t91[0-9][0-9]-*.sh)
 
 ifndef ALLOW_INSECURE_HOOKS
 	T := $(filter-out t5501-post-upload-pack.sh,$(T))
+	T := $(filter-out t5507-pre-upload-pack.sh,$(T))
 endif
 
 all: pre-clean
diff --git a/t/t5507-pre-upload-pack.sh b/t/t5507-pre-upload-pack.sh
new file mode 100644
index 0000000..d3a7ba7
--- /dev/null
+++ b/t/t5507-pre-upload-pack.sh
@@ -0,0 +1,93 @@
+#!/bin/sh
+
+test_description='pre upload-hook'
+
+. ./test-lib.sh
+
+LOGFILE=".git/pre-upload-pack-log"
+
+test_expect_success setup '
+	test_commit A &&
+	test_commit B &&
+	git reset --hard A &&
+	test_commit C &&
+	git branch prev B &&
+	mkdir -p .git/hooks &&
+	{
+		echo "#!$SHELL_PATH" &&
+		echo "cat >pre-upload-pack-log"
+	} >".git/hooks/pre-upload-pack" &&
+	chmod +x .git/hooks/pre-upload-pack
+'
+
+test_expect_success initial '
+	rm -fr sub &&
+	git init sub &&
+	(
+		cd sub &&
+		git fetch --no-tags .. prev
+	) &&
+	want=$(sed -n "s/^want //p" "$LOGFILE") &&
+	test "$want" = "$(git rev-parse --verify B)" &&
+	! grep "^have " "$LOGFILE" &&
+	kind=$(sed -n "s/^kind //p" "$LOGFILE") &&
+	test "$kind" = fetch
+'
+
+test_expect_success second '
+	rm -fr sub &&
+	git init sub &&
+	(
+		cd sub &&
+		git fetch --no-tags .. prev:refs/remotes/prev &&
+		git fetch --no-tags .. master
+	) &&
+	want=$(sed -n "s/^want //p" "$LOGFILE") &&
+	test "$want" = "$(git rev-parse --verify C)" &&
+	have=$(sed -n "s/^have //p" "$LOGFILE") &&
+	test "$have" = "$(git rev-parse --verify B)" &&
+	kind=$(sed -n "s/^kind //p" "$LOGFILE") &&
+	test "$kind" = fetch
+'
+
+test_expect_success all '
+	rm -fr sub &&
+	HERE=$(pwd) &&
+	git init sub &&
+	(
+		cd sub &&
+		git clone "file://$HERE/.git" new
+	) &&
+	sed -n "s/^want //p" "$LOGFILE" | sort >actual &&
+	git rev-parse A B C | sort >expect &&
+	test_cmp expect actual &&
+	! grep "^have " "$LOGFILE" &&
+	kind=$(sed -n "s/^kind //p" "$LOGFILE") &&
+	test "$kind" = clone
+'
+
+cat > pre-upload-pack <<EOF
+#!$SHELL_PATH
+kind=\$(awk '/^kind /{print \$2; exit}' -)
+if test "\$kind" = "clone"; then
+  echo "Sorry, no cloning!"
+exit 1; fi
+EOF
+
+test_expect_success 'with failing hook' '
+	rm -fr .git
+	test_create_repo src &&
+	(
+		cd src &&
+		mkdir .git/hooks &&
+		mv ../pre-upload-pack ".git/hooks/pre-upload-pack" &&
+		chmod +x .git/hooks/pre-upload-pack &&
+		echo foo > file &&
+		git add file &&
+		git commit -m initial
+	) &&
+	test_must_fail git clone -n "file://$(pwd)/src" dst
+
+'
+
+test_done
diff --git a/templates/hooks--pre-upload-pack.sample b/templates/hooks--pre-upload-pack.sample
new file mode 100644
index 0000000..7342d23
--- /dev/null
+++ b/templates/hooks--pre-upload-pack.sample
@@ -0,0 +1,11 @@
+#!/bin/sh
+
+# This sample shows how one may reject an upload-pack where the client is
+# trying to perform an initial clone clone
+
+kind=$(awk '/^kind /{print $2; exit}' -)
+
+if test "$kind" = "clone"; then
+  echo "Sorry, the clone operation is not allowed"
+  exit 1
+fi
diff --git a/upload-pack.c b/upload-pack.c
index c992cb4..9c33e63 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -171,14 +171,19 @@ static int feed_obj_to_hook(const char *label, struct object_array *oa, int i, i
 				sha1_to_hex(oa->objects[i].item->sha1));
 }
 
-static int run_post_upload_pack_hook(size_t total, struct timeval *tv)
+static int run_upload_pack_hook(int post, size_t total, struct timeval *tv)
 {
 	const char *argv[2];
 	struct child_process proc;
 	int err, i;
 
-	argv[0] = "hooks/post-upload-pack";
-	argv[1] = NULL;
+	if (!post) {
+		argv[0] = "hooks/pre-upload-pack";
+		argv[1] = NULL;
+	} else {
+		argv[0] = "hooks/post-upload-pack";
+		argv[1] = NULL;
+	}
 
 	if (access(argv[0], X_OK) < 0)
 		return 0;
@@ -197,10 +202,10 @@ static int run_post_upload_pack_hook(size_t total, struct timeval *tv)
 		err |= feed_obj_to_hook("want", &want_obj, i, proc.in);
 	for (i = 0; !err && i < have_obj.nr; i++)
 		err |= feed_obj_to_hook("have", &have_obj, i, proc.in);
-	if (!err)
+	if (!err && post)
 		err |= feed_msg_to_hook(proc.in, "time %ld.%06ld\n",
 					(long)tv->tv_sec, (long)tv->tv_usec);
-	if (!err)
+	if (!err && post)
 		err |= feed_msg_to_hook(proc.in, "size %ld\n", (long)total);
 	if (!err)
 		err |= feed_msg_to_hook(proc.in, "kind %s\n",
@@ -758,7 +763,10 @@ static void upload_pack(void)
 	receive_needs();
 	if (want_obj.nr) {
 		get_common_commits();
-		create_pack_file();
+		if (run_upload_pack_hook(0, 0, NULL))
+			error("pre-upload hook aborted");
+		else
+			create_pack_file();
 	}
 }
 
-- 
1.6.6.1

Re: [PATCH 0/2] upload-pack: pre- and post- hooks

[Shawn O. Pearce]

Arun Raghavan <ford_prefect@gentoo.org> wrote:
> This patch set reintroduces the post-upload-pack hook and adds a
> pre-upload-pack hook. These are now only built if 'ALLOW_INSECURE_HOOKS' is set
> at build time. The idea is that only system administrators who need this
> functionality and are sure the potential insecurity is not relevant to their
> system will enable it.

*sigh*

I guess this is better, having it off by default, but allowing an
administrator who needs this feature to build a custom package.

Unfortunately... I'm sure some distro out there is going to think
they know how to compile Git better than we do, and enable this by
default, exposing their users to a security hole.  Ask the OpenSSL
project about how well distros package code...  :-\

I'd like a bit more than just a compile time flag.
 
> At some point if the future, if needed, this could also be made a part of the
> negotiation between the client and server.

I'm not sure I follow.

Are you proposing the server advertises that it wants to run hooks,
and lets the client decide whether or not they should be executed?

-- 
Shawn.

Re: [PATCH 0/2] upload-pack: pre- and post- hooks

[Arun Raghavan]

On 1 February 2010 20:50, Shawn O. Pearce <spearce@spearce.org> wrote:
> Arun Raghavan <ford_prefect@gentoo.org> wrote:
>> This patch set reintroduces the post-upload-pack hook and adds a
>> pre-upload-pack hook. These are now only built if 'ALLOW_INSECURE_HOOKS' is set
>> at build time. The idea is that only system administrators who need this
>> functionality and are sure the potential insecurity is not relevant to their
>> system will enable it.
>
> *sigh*
>
> I guess this is better, having it off by default, but allowing an
> administrator who needs this feature to build a custom package.
>
> Unfortunately... I'm sure some distro out there is going to think
> they know how to compile Git better than we do, and enable this by
> default, exposing their users to a security hole.  Ask the OpenSSL
> project about how well distros package code...  :-\
>
> I'd like a bit more than just a compile time flag.

I was hoping the all-caps INSECURE in the name would give distributors pause. :)

Suggestions on what else might work?

>> At some point if the future, if needed, this could also be made a part of the
>> negotiation between the client and server.
>
> I'm not sure I follow.
>
> Are you proposing the server advertises that it wants to run hooks,
> and lets the client decide whether or not they should be executed?

Something like that. I was thinking the client could always advertise
whether the it wants to allow the hooks to be executed or not (which
would override the default value of the global variable I introduced).
Either approach would work, though the second is simpler but also
dumber.

Again, this might be over-complicating things, which is why I did not
implement it. I just wanted to make a note of the fact that this could
be done if the need is felt.

Cheers,
-- 
Arun Raghavan
http://arunraghavan.net/
(Ford_Prefect | Gentoo) & (arunsr | GNOME)

Re: [PATCH 0/2] upload-pack: pre- and post- hooks

[Shawn O. Pearce]

Arun Raghavan <ford_prefect@gentoo.org> wrote:
> On 1 February 2010 20:50, Shawn O. Pearce <spearce@spearce.org> wrote:
> > Arun Raghavan <ford_prefect@gentoo.org> wrote:
> >> This patch set reintroduces the post-upload-pack hook and adds a
> >> pre-upload-pack hook. These are now only built if 'ALLOW_INSECURE_HOOKS' is set
> >> at build time. The idea is that only system administrators who need this
> >> functionality and are sure the potential insecurity is not relevant to their
> >> system will enable it.
> >
> > *sigh*
> >
> > I guess this is better, having it off by default, but allowing an
> > administrator who needs this feature to build a custom package.
> >
> > Unfortunately... I'm sure some distro out there is going to think
> > they know how to compile Git better than we do, and enable this by
> > default, exposing their users to a security hole. ?Ask the OpenSSL
> > project about how well distros package code... ?:-\
> >
> > I'd like a bit more than just a compile time flag.
> 
> I was hoping the all-caps INSECURE in the name would give
> distributors pause. :)
> 
> Suggestions on what else might work?

At one point we were talking about checking the owner of the hook
script itself.  If it was uid 0 or the current actual user uid,
then we run the hook, otherwise we don't.

That only really works on POSIX platforms, but it does make some
sense.  Root can already screw with you by replacing the binary
you are executing, so any hook they own is no more risky than the
git-upload-pack you just started.

If its the actual user uid, then systems like gitosis can still
make use of the hook by making the hook owned by the "git" user
that gitosis is executing all sessions under.

But mixed user systems, the hook would only run for the user who
created it, and be skipped for everyone else.

I'm not really sure what to do on Win32 here.  Everyone is usually
Administrator these days which makes the test for "root" there
somewhat pointless.  Maybe its just not enabled on Win32.


> >> At some point if the future, if needed, this could also be made a part of the
> >> negotiation between the client and server.
> >
> > I'm not sure I follow.
> >
> > Are you proposing the server advertises that it wants to run hooks,
> > and lets the client decide whether or not they should be executed?
> 
> Something like that. I was thinking the client could always advertise
> whether the it wants to allow the hooks to be executed or not (which
> would override the default value of the global variable I introduced).
> Either approach would work, though the second is simpler but also
> dumber.
> 
> Again, this might be over-complicating things, which is why I did not
> implement it. I just wanted to make a note of the fact that this could
> be done if the need is felt.

My concern with this is, users might disable the hook all of the
time, and then servers that actually want the hook (e.g. gentoo's
use of the pre-upload-pack to avoid initial clones over git://)
would be stuck, just like they are today.

No, its just not sane to give the user a choice whether or not they
should execute something remotely.

-- 
Shawn.

Re: [PATCH 0/2] upload-pack: pre- and post- hooks

[Arun Raghavan]

On 1 February 2010 21:31, Shawn O. Pearce <spearce@spearce.org> wrote:
> Arun Raghavan <ford_prefect@gentoo.org> wrote:
>> On 1 February 2010 20:50, Shawn O. Pearce <spearce@spearce.org> wrote:
>> > Arun Raghavan <ford_prefect@gentoo.org> wrote:
[...]
>> >> At some point if the future, if needed, this could also be made a part of the
>> >> negotiation between the client and server.
>> >
>> > I'm not sure I follow.
>> >
>> > Are you proposing the server advertises that it wants to run hooks,
>> > and lets the client decide whether or not they should be executed?
>>
>> Something like that. I was thinking the client could always advertise
>> whether the it wants to allow the hooks to be executed or not (which
>> would override the default value of the global variable I introduced).
>> Either approach would work, though the second is simpler but also
>> dumber.
>>
>> Again, this might be over-complicating things, which is why I did not
>> implement it. I just wanted to make a note of the fact that this could
>> be done if the need is felt.
>
> My concern with this is, users might disable the hook all of the
> time, and then servers that actually want the hook (e.g. gentoo's
> use of the pre-upload-pack to avoid initial clones over git://)
> would be stuck, just like they are today.
>
> No, its just not sane to give the user a choice whether or not they
> should execute something remotely.

Ah, sorry I wasn't clear about this. I've made it so that when
pre-upload-pack fails, the entire operation fails. This makes sense
because pre-upload-pack is meant to check things like "do we want
allow the user to get the pack". For post-upload-pack, failure only
results in a warning, since the actual upload is already done and
there's not much to do other than log the failure.

-- 
Arun Raghavan
http://arunraghavan.net/
(Ford_Prefect | Gentoo) & (arunsr | GNOME)

Re: [PATCH 0/2] upload-pack: pre- and post- hooks

[Nicolas Pitre]

On Mon, 1 Feb 2010, Shawn O. Pearce wrote:

> Arun Raghavan <ford_prefect@gentoo.org> wrote:
> > This patch set reintroduces the post-upload-pack hook and adds a
> > pre-upload-pack hook. These are now only built if 'ALLOW_INSECURE_HOOKS' is set
> > at build time. The idea is that only system administrators who need this
> > functionality and are sure the potential insecurity is not relevant to their
> > system will enable it.
> 
> *sigh*
> 
> I guess this is better, having it off by default, but allowing an
> administrator who needs this feature to build a custom package.
> 
> Unfortunately... I'm sure some distro out there is going to think
> they know how to compile Git better than we do, and enable this by
> default, exposing their users to a security hole.  Ask the OpenSSL
> project about how well distros package code...  :-\
> 
> I'd like a bit more than just a compile time flag.

I think such hooks could be allowed only if triggered explicitly by the 
upload-pack caller, such as git-daemon.  That's probably the only 
scenario where a useful use case can be justified for them anyway.

And of course, to avoid any security problems, the actual hooks must not 
be provided by the repository owner but provided externally, like from 
git-daemon, via some upload-pack command line arguments.  This way the 
hooks are really controlled by the system administrator managing 
git-daemon and not by any random git repository owner.

That should be good enough for all the use cases those hooks were 
originally designed for.


Nicolas

Re: [PATCH 0/2] upload-pack: pre- and post- hooks

[Shawn O. Pearce]

Nicolas Pitre <nico@fluxnic.net> wrote:
> On Mon, 1 Feb 2010, Shawn O. Pearce wrote:
> I think such hooks could be allowed only if triggered explicitly by the 
> upload-pack caller, such as git-daemon.  That's probably the only 
> scenario where a useful use case can be justified for them anyway.
> 
> And of course, to avoid any security problems, the actual hooks must not 
> be provided by the repository owner but provided externally, like from 
> git-daemon, via some upload-pack command line arguments.  This way the 
> hooks are really controlled by the system administrator managing 
> git-daemon and not by any random git repository owner.
> 
> That should be good enough for all the use cases those hooks were 
> originally designed for.

Oooh, I like that.

If the paths to the hooks are passed in on the command line of
git-upload-pack, and git-daemon takes those options and passes
them through, you're right, we probably get everything we need.

Gitosis can still use the hooks if it wants, since it controls
the call of git-upload-pack.

-- 
Shawn.

Re: [PATCH 0/2] upload-pack: pre- and post- hooks

[Arun Raghavan]

On 1 February 2010 22:06, Shawn O. Pearce <spearce@spearce.org> wrote:
> Nicolas Pitre <nico@fluxnic.net> wrote:
>> On Mon, 1 Feb 2010, Shawn O. Pearce wrote:
>> I think such hooks could be allowed only if triggered explicitly by the
>> upload-pack caller, such as git-daemon.  That's probably the only
>> scenario where a useful use case can be justified for them anyway.
>>
>> And of course, to avoid any security problems, the actual hooks must not
>> be provided by the repository owner but provided externally, like from
>> git-daemon, via some upload-pack command line arguments.  This way the
>> hooks are really controlled by the system administrator managing
>> git-daemon and not by any random git repository owner.
>>
>> That should be good enough for all the use cases those hooks were
>> originally designed for.
>
> Oooh, I like that.
>
> If the paths to the hooks are passed in on the command line of
> git-upload-pack, and git-daemon takes those options and passes
> them through, you're right, we probably get everything we need.
>
> Gitosis can still use the hooks if it wants, since it controls
> the call of git-upload-pack.

I can add the uid check before running the hook as well. Is that good
enough, or would you guys like me to start from scratch with the
command-line argument approach?

Cheers,
-- 
Arun Raghavan
http://arunraghavan.net/
(Ford_Prefect | Gentoo) & (arunsr | GNOME)

Re: [PATCH 0/2] upload-pack: pre- and post- hooks

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1531 bytes --]

On Tue, 2 Feb 2010, Arun Raghavan wrote:

> On 1 February 2010 22:06, Shawn O. Pearce <spearce@spearce.org> wrote:
> > Nicolas Pitre <nico@fluxnic.net> wrote:
> >> On Mon, 1 Feb 2010, Shawn O. Pearce wrote:
> >> I think such hooks could be allowed only if triggered explicitly by the
> >> upload-pack caller, such as git-daemon.  That's probably the only
> >> scenario where a useful use case can be justified for them anyway.
> >>
> >> And of course, to avoid any security problems, the actual hooks must not
> >> be provided by the repository owner but provided externally, like from
> >> git-daemon, via some upload-pack command line arguments.  This way the
> >> hooks are really controlled by the system administrator managing
> >> git-daemon and not by any random git repository owner.
> >>
> >> That should be good enough for all the use cases those hooks were
> >> originally designed for.
> >
> > Oooh, I like that.
> >
> > If the paths to the hooks are passed in on the command line of
> > git-upload-pack, and git-daemon takes those options and passes
> > them through, you're right, we probably get everything we need.
> >
> > Gitosis can still use the hooks if it wants, since it controls
> > the call of git-upload-pack.
> 
> I can add the uid check before running the hook as well. Is that good
> enough, or would you guys like me to start from scratch with the
> command-line argument approach?

Please forget the uid check and go with the command-line argument 
approach.  That's the only sane solution.


Nicolas