Re: [PATCH v2] hooks: propose project configured hooks

["brian m. carlson" <sandals@crustytoothpaste.net>]

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

On 2021-04-05 at 22:45:10, Albert Cui wrote:
> Right, this entire proposal is trying to get to a "Git-blessed" solution,
> and I should make the need clearer. A few reasons for standardizing
> this come to mind:
> 
> 1. Many existing "standard" solutions. A multitude of existing solutions for
> this use case speaks to the fact that a basic config script is not sufficient.
> I mentioned Husky above, but here are a few more; basically each
> popular programming language environment has a solution for this.
> 
> https://github.com/sds/overcommit - Ruby
> https://github.com/pre-commit/pre-commit - Python
> https://github.com/Arkweid/lefthook - Go
> https://github.com/shibapm/Komondor - Swift
> https://github.com/typicode/husky - Node
> 
> These solutions all handle the installation and updating of hooks. A
> "configure-hooks.sh" script doesn't handle hook updates, unless you go through
> the trouble yourself of implementing and maintaining that.

I think part of the problem is that an automated process to update hooks
is generally a security vulnerability, since it means that untrusted
remote code will automatically run on your computer.

I want to be clear that I understand the desire for this feature, even
though it's not a feature I would personally use, and the fact that
there are many approaches means that clearly there are many people that
do want this functionality.  I have in the past shared hooks with others
and we have mutually benefitted enormously from that fact.  My concerns
here are solely about the security aspects of this feature.

> 3. Improving security. As you mentioned, hooks are difficult to get
> right from a security
> perspective, and standardizing on a single implementation allows us to
> give developers
> a well-vetted solution with a better security model than what exists
> today. For example,
> we're proposing making it very clear to users whenever there's a hook
> update. This isn't
> something that existing solutions do.

I don't think this materially improves security.  All of these options
have the same security problems, and that's inherent in the solution.
What we're doing here is basically giving people a built-in feature that
is the equivalent of piping curl to bash and blessing it as secure when
it's not.

> I'll also say in general, the Git project is much more likely to get
> security right than smaller
> projects, where oftentimes even popular projects end up unmaintained.

I agree that Git tries to be careful about security.  It is for these
reasons that I think Derrick and I have provided you the feedback we
have here.

> Agreed. We already did a security review internally at Google. The main
> feedback was:
> 
> * We need an explicit opt-in opposed to setting hooks up automatically,
> e.g. a command line flag like --accept-hooks at minimum. This is primarily
> to distinguish people who are just cloning a repository to browse the code
> from people who are developing.
> 
> * The average user doesn't have the ability to review hooks in general
> (security is hard and obscuration is easy), and if the user has
> already opted into
> this feature because they are engaged in development, it's very likely
> that they're
> already running build scripts, so the additional attack vector here doesn't seem
> like a big issue.

I think you've hit the nail on the head here, but drawn a mistaken
conclusion.  The average user doesn't have the ability to review hooks
in general and therefore cannot make an informed decision about whether
to enable them, so the behavior we need to have is not to lead them to
doing things which are risky from a security perspective.

If my goal is to just build a product and not to run its tests, which I
do with a decent number of projects, then I can audit a Go or Rust
project trivially and determine if it executes arbitrary code or not
during the build process and if so, inspect it and gain confidence in
it.  In fact, there are many projects which don't execute build scripts
during the process, and therefore which are completely safe.  This hook
design changes that calculus dramatically.

I also want to point out that people clone repositories for a variety of
reasons.  At GitHub, every team has its own repository with
documentation.  Literally every employee at the company, regardless of
role, interacts with a Git repository, even people who do normally
nontechnical tasks such as our in-house lawyers and our event planners.
Many of these people are nontechnical, and almost none of these
repositories has any software development involvement.  There are also
numerous people elsewhere who may work on projects such as books or
other non-software in repositories who are nontechnical.  Under the
current model, the biggest problem these people face is accidentally
corrupting their local repository and losing data.  With a design that
prompts them to install hooks, they face the possibility of arbitrary
code execution.

The reason I proposed the FAQ we have in our documentation is because I
answer a decent number of questions on Stack Overflow, in addition to
questions that involve users that I get pulled into at work.
Overwhelmingly, the vast majority of users, even developers, are not
completely comfortable with Git and are unsure about how to use it
effectively (cf. https://ohshitgit.com/).  If we propose to a user that
they should do something like enable hooks by adding a prompt, many
users will automatically say "yes" because (a) they don't understand and
they trust that Git is prompting them to do something beneficial and (b)
because they don't know or care and just want to get on with their
lives.  As a result, we're exposing people to giant social engineering
attacks on behalf of potentially unscrupulous repository maintainers.

This is made worse by the fact that we will prompt users even when
cloning a repo that they have no intention of performing development on
means that we will have users who are misled here where otherwise
nothing would happen.

There is a huge problem with social engineering attacks and phishing on
the Internet today and I'm concerned that this is going in exactly the
wrong direction.

I would want to see a comprehensive security analysis feature taking
into consideration social engineering attacks, the skill level and
comfort with Git of the majority of Git users, and the fact that people
clone repositories for many reasons other than software development.
It's easy to look at this from the perspective of the typical employee
at a major tech company and assume that users are generally security
conscious, comfortable with Git, and primarily engaged in software
development on the projects they clone, but I'm not sure any of those
cases are generally true, and anyway there are many counterexamples in
the real world whose use cases we need to take into account.

I continue to have serious reservations about this series and approach,
and I'm not sure that any proposal we can adopt here will address the
security concerns.  To be frank, I don't think this proposal should move
forward in its current state or otherwise, since I think the security
problems are inherent in this approach and fundamentally can't be fixed.

This is, as should be obvious from my email address, my personal
opinion, despite my reference to my employer above.  Unless otherwise
stated, I don't speak for my employer and they don't speak for me.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]