Re: Removal of post-upload-hook

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.

Re: Removal of post-upload-hook

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

Re: Removal of post-upload-hook

> Jeff King <[hidden email]> 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.

Re: Removal of post-upload-hook

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

Re: Removal of post-upload-hook

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

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.

Re: Removal of post-upload-hook

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

Re: Removal of post-upload-hook

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

Re: Removal of post-upload-hook

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

Re: Removal of post-upload-hook

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

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

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.

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

+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

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

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

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

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

Arun Raghavan <[hidden email]> 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?

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

> Arun Raghavan <[hidden email]> 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.

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

> On 1 February 2010 20:50, Shawn O. Pearce <[hidden email]> wrote:
> > Arun Raghavan <[hidden email]> 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.

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

> Arun Raghavan <[hidden email]> 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.

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

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

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

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

> Nicolas Pitre <[hidden email]> 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?