Re: [PATCH 0/5] refactor gpg-interface and add gpg verification for clones

[Hans Jerry Illikainen <hji@dyntopia.com>]

On Sun, Jan 05 2020, Junio C Hamano wrote:
> Hans Jerry Illikainen <hji@dyntopia.com> writes:
>
>> And finally, signature verification is added to the clone builtin.  It
>> obeys --(no-)verify-signatures, clone.verifySignatures and
>> gpg.verifySignatures (in decreasing order of significance).
>
> I am not sure what it should mean to verify signature on clone.  I'd
> assume that our threat model and verification policy are consistent
> with what we use for a merge/pull, in that we trust all history
> behind a commit that has a trusted signature, so it is clear that
> you would want the tip commit of the default branch (or if you are
> naming a single branch to clone, then the tip of that branch) to
> carry a trusted signature.

Yes, that's how it's implemented in v0 -- the tip of the branch/tag that
is to be checked out is verified.


> But what about the commits that are reachable from other branches and
> tags that are *not* contained in the branch that is initially checked
> out?

I thought about that and figured that adding signature verification for
the checkout builtin would be the next step after this series.  That
thought should probably have been mentioned in the cover letter for
criticism -- because now I'm not sure if that's a reasonable approach
anymore.


> Later in the proposed log message of 5/5 you allude to risk of
> merely checking out a potentially untrustworthy contents to the
> working tree.  This is far stricter than the usual threat model we
> tend to think about as the developers of source code management
> system, where checking out is perfectly OK but running "make" or its
> equivalent is the first contact between the victim's system with
> malicious contents.

Modern desktop environments perform enough magic that I think it makes
sense to assume that simply having malicious content on disk is enough
for a compromise -- even though no explicit user interaction takes place
with that content.  The NSF bug in GStreamer that made headlines a few
years back is a good example of that [1].  And the numerous AV bugs
published by taviso [2].


> Verifying the tip of the default/sole branch upon cloning before the
> tree of the commit is checked out certainly would cover that single
> case (i.e. the initial checkout after cloning), but I am not sure if
> it is the best way, and I am reasonably certain it is insufficient
> against your threat model.  After such a clone is made, when the
> user checks out another branch obtained from the "origin" remote,
> there is no mechanism that protects the user from the same risk you
> just covered with the new signature verification mechanism upon
> cloning, without validating the tip of that other branch, somewhere
> between the time the clone is made and that other branch gets
> checked out.

I agree.  Again, I should have mentioned my thoughts on adding signature
verification to the checkout builtin in the cover letter.


> It almost makes me suspect that what you are trying to do with the
> "verification upon cloning" may be much better done as a "verify the
> tree for trustworthyness before checking it out to the working tree"
> mechanism, where the trustworthyness of a tree-ish object may be
> defined (and possibly made customizable by the policy of the project
> the user is working on) like so:
>
>  - A tree is trustworthy if it is the tree of a trustworthy commit.
>
>  - A commit is trustworthy if
>
>    . it carries a trusted signature, or
>    . it is pointed by a tag that carries a trusted signature, or
>    . it is reachable from a trustworthy commit.
>
> Now, it is prohibitively expensive to compute the trusttworthiness
> of a tree, defined like the above, when checking it out, UNLESS you
> force each and every commit to carry a trusted signature, which is
> not necessarily practical in the real world.

Even though you mention that it's computationally expensive, I kind of
like this approach.  It seems generalized enough that it doesn't need to
be tied to a single operation like 'clone'.


> Another approach to ensure that any and all checkout would work only
> on a trustworthy tree might be to make sure that tips of *all* the
> refs are trustworthy commits immediately after cloning (instead of
> verifying only the branch you are going to checkout, which is what
> your patch does IIUC).  That way, any subsequent checkout in the
> repository would either be checking out a commit that was
>
>  - originally cloned from the remote, and is reachable from some ref
>    that was validated back when the repository was cloned, or
>
>  - created locally in this repository, which we trust, or
>
>  - fetched from somewhere else, and is reachable from the commit
>    that was validated back when "git pull" validated what was about
>    to be integrated into the history of *this* repository.

Wouldn't that still forgo signature verification when doing something
like:

    $ git fetch
    $ git checkout -b foo origin/branch-with-previously-unseen-commits

unless either fetch or checkout is equipped with signature verification?

Also, in case of a revoked key, this approach would require that tags
signed with that key be deleted on origin.  Maybe that's to be
considered best practice anyway, but distro maintainers might not
appreciate disappearing release tags.

Also, an interesting observation (but probably a "not our problem" as
far as Git is concerned) -- I have noticed that certain git forges might
create branches unexpectedly.  A few of my repos has dependabot/...
branches created by a GitHub bot that is enabled by default:

"""
Automated security updates are opened by Dependabot on behalf of
GitHub. The Dependabot GitHub App is automatically installed on every
repository where automated security updates are enabled.
"""

I can foresee a scenario where validating the tip of every ref on a
fresh clone would result in a DoS for lot of automated CI/CD-type
workflows when bots on GitHub (and other hosts) creates unexpected
branches in the users repos.

    $ git branch -r
      origin/HEAD -> origin/master
      origin/dependabot/pip/requirements/typed-ast-1.3.2
      origin/master


> Hmm?

I appreciate you taking the time to write your thoughts!  Unfortunately
there doesn't seem to be an obvious approach that is significantly
preferable to the alternatives.  I will experiment with the ideas you
mentioned above and see if I can come up with something that makes
sense.

Would you prefer that I break off the series before 5/5 in a new series
to fix the comments you made on the other patches that seem
almost-mergeable?

Thanks!


[1] https://scarybeastsecurity.blogspot.com/2016/11/0day-exploit-compromising-linux-desktop.html
[2] https://bugs.chromium.org/p/project-zero/issues/detail?id=769

-- 
hji