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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Feb 1, 2010, 16:01 UTC
Message-ID: <20100201160141.GG8916@spearce.org>
In-Reply-To: <6f8b45101002010750t5541faefv5b4640dfb9949306@mail.gmail.com>

Arun Raghavan <ford_prefect@gentoo.org> wrote:

Show 24 quoted lines

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

Show 17 quoted lines

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