Re: [PATCH v3 0/8] refs: add symref support to 'git-update-ref'

From: Jeff King <peff@peff.net>
Date: Apr 23, 2024, 22:03 UTC
Message-ID: <20240423220308.GC1172807@coredump.intra.peff.net>
In-Reply-To: <20240423212818.574123-1-knayak@gitlab.com>

On Tue, Apr 23, 2024 at 11:28:10PM +0200, Karthik Nayak wrote:

> Changes from v2:
> 1. We no longer have separate commands for symrefs, instead the regular
> commands learn to parse 'ref:<target>' as symref targets. This reduces
> the code in this series. Thanks Patrick for the suggestion.

Hmm. I can see how this makes things a lot simpler, but it introduces an ambiguity, since you can pass full ref expressions to "update-ref" (like "ref:foo" to find the "foo" entry in ref^{tree}). I see that you only kick in the symref "ref:" handling if the regular oid lookup failed, so there's no backwards-compatibility issue (anything that used to work will still take precedence, and the new code only runs when the old code would have reported an error).

But I wonder if it would let somebody cause mischief in a repository they can push to, but which may get updates from other sources. For example, imagine a forge like GitLab runs the equivalent of:

  echo "create refs/whatever ref:refs/heads/main" |
  git update-ref --stdin

as part of some system process. Now if I push up "refs/heads/ref" that contains the path "refs/heads/main" in its tree, that will take precedence, causing the system process to do something it did not expect.

I think you'd have to pile on a lot of assumptions to get any kind of security problem. Something like:

 1. The system has a hidden ref namespace like refs/gitlab that normal
    remote push/fetch users are not allowed to read/write to.

 2. The system tries to make a symlink within that namespace. Say,
    "refs/gitlab/metadata/HEAD" to point to
    "refs/gitlab/metadata/branches/main" or something.

 3. The user pushes up "refs/heads/ref" with a tree that contains
    "refs/gitlab/metadata/branches/main". Now when (2) happens, the
    hidden ref points to user-controlled data.

That's pretty convoluted. But we can avoid it entirely if there's no ambiguity in the protocol at all.

-Peff