Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 23, 2021, 22:58 UTC
Message-ID: <20210623225809.4023571-1-jonathantanmy@google.com>
In-Reply-To: <YNExhalSLWvmky55@camp.crustytoothpaste.net>

Show 6 quoted lines

> > First, unlike brian I don't (I hope I'm fairly summarizing his view
> > here) disagree mostly or entirely with the existence of such a feature
> > at all. I mean, I get the viewpoint that git shouldn't bless what
> > amounts to an active RCE from the remote.
> 
> It's accurate that I'm generally opposed to such a feature.

From emails like [1], I think that you have understood both the pros and cons, and decided that the cons outweigh the pros, which is fair. I'll reply so that others can know what I think about these points.

[1] https://lore.kernel.org/git/YGzrfaSC4xd75j2U@camp.crustytoothpaste.net/

> I feel that
> suggesting people install hooks is likely to lead to social engineering
> attacks,

I think that this vector of social engineering attack already exists, as there are projects that without malice recommend hook installation (so an attacker could masquerade as such). It is true that Git itself printing a message recommending hook installation perhaps could lend a false sense of security, but at least we can control what that message says (as opposed to a project recommending a third-party tool, all with messaging out of our control).

> and it's also likely to lead to bad practices such as the
> expectation that all developers will install hooks or the use of hooks
> instead of CI or other effective controls.

I agree that hooks can be overused or misused, but there are still legitimate uses of it that a project might want to use.

> If we do add this feature (which, as I said, I'm opposed to) and we
> decide to store it in a ref, that ref should not be a normal branch by
> default (it should be a special one-level ref, like refs/stash or such),

Any particular reason not to expose it as a branch (besides following from your general idea that a user should seek out such a feature and not have it presented to them up-front)?

> and the ref name should be configurable.  Not all developers use English
> as their working language and we should respect that.

That makes sense.

> In addition, there should be an advice.* option that allows people to
> turn this off once and for all, and it should be clearly documented.
> Ideally it should be off by default.

I don't think this would be considered "advice" like the other options, but having an option to turn this off once and for all makes sense. Making it off by default would probably mean that projects that use such hooks would recommend cloning with "git -c my-config=1 clone $URL", but perhaps that's OK.

Show 20 quoted lines

> > I think I get why you want to do it that way, I just don't get why, as
> > mostly noted in those earlier rounds why it wouldn't be a better
> > approach / more straightforward / more git-y to:
> > 
> > 1. Work on getting hooks driven by config <this is happening with
> >    Emily's series / my split-out "base" topic>
> > 2. Have a facility to read an in-repo '.gitconfig'; have lots of safety
> >    valves etc. around this, I suggested starting with a whitelist of the
> >    N least dangerous config options, e.g. some diff viewing options, or
> >    a suggested sendemail.to or whatever.
> 
> This also makes me deeply nervous for much of the same reasons.  There
> are situations where e.g. ignoring whitespace can lead to security
> problems in code review (think Python), and in general it's hard to
> reason about all the ways people can do malicious things.  Typically
> adding untrusted config ends poorly (think of all the modeline
> vulnerabilities in Vim).
> 
> I'd definitely want support for this to be off with no prompting by
> default.

To use your example, the model we're proposing is more of only using the modelines from sources we trust - as opposed to ensuring that all possible options set by modelines are benign. Admittedly, the administrator of the source may have difficulty ensuring that bad code doesn't slip through code review, for example, but that is a problem they already deal with (at least for projects with any form of executable code in them, e.g. production code or a build script).