Re: [PATCH 0/3] protocol v2 and hidden refs

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 11, 2018, 11:45 UTC
Message-ID: <875zw0nv77.fsf@evledraar.gmail.com>
In-Reply-To: <20181211104236.GA6899@sigill.intra.peff.net>

On Tue, Dec 11 2018, Jeff King wrote:

Show 25 quoted lines

> When using the v2 protocol, hidden-ref config is not respected at all:
>
>   $ git config transfer.hiderefs refs/tags/
>   $ git -c protocol.version=0 ls-remote . | grep -c refs/tags
>   0
>   $ git -c protocol.version=2 ls-remote . | grep -c refs/tags
>   1424
>
> The fix in patch 3 is pretty straightforward, but note:
>
>   - I'm a little worried this may happen again with future features. The
>     root cause is that "ls-refs" follows a different code path than the
>     ref advertisement for "upload-pack". So if we add any new config,
>     it needs to go both places (non ref-advertisement config is OK, as
>     the v2 "fetch" command is a lot closer to a v0 upload-pack).
>
>     I think this is just an issue for any future features. I looked for
>     other existing features which might be missing in v2, but couldn't
>     find any.
>
>     I don't know if there's a good solution. I tried running the whole
>     test suite with v2 as the default. It does find this bug, but it has
>     a bunch of other problems (notably fetch-pack won't run as v2, but
>     some other tests I think also depend on v0's reachability rules,
>     which v2 is documented not to enforce).

I think a global test mode for it would be a very good idea.

Show 21 quoted lines

>   - The "serve" command is funky, because it has no concept of whether
>     the "ls-refs" is for fetching or pushing. Is git-serve even a thing
>     that we want to support going forward?  I know part of the original
>     v2 conception was that one would be able to just connect to
>     "git-serve" and do a number of operations. But in practice the v2
>     probing requires saying "I'd like to git-upload-pack, and v2 if you
>     please". So no client ever calls git-serve.
>
>     Is this something we plan to eventually move to? Or can it be
>     considered a funny vestige of the development? In the latter case, I
>     think we should consider removing it.
>
>     I've worked around it here with patch 2, but note that "git serve"
>     would not ever respect uploadpack.hiderefs nor receive.hiderefs
>     (since it has no idea which operation it's doing).
>
> The patches are:
>
>   [1/3]: serve: pass "config context" through to individual commands
>   [2/3]: parse_hide_refs_config: handle NULL section
>   [3/3]: upload-pack: support hidden refs with protocol v2

Does this issue rise to the level of needing a security point-release (which I'm discussing here as the details are already public). The transfer.hideRefs docs have said:

    Even if you hide refs, a client may still be able to steal the
    target objects via the techniques described in the "SECURITY"
    section of the gitnamespaces(7) man page; it’s best to keep private
    data in a separate repository.

So we never promised to hide the objects, but definitely promised to hide the ref names. I don't know if anyone uses this in practice for secret ref names, but if they do they have a data leak if they enable protocol v2.

More importantly, the docs for receive.hideRefs say. "An attempt to update or delete a hidden ref by git push is rejected.". It seems this bit was enforced, i.e. this passes before and after your 3/3, but I have not dug enough to be 100% satisfied with that.

    diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
    index ca69636fd5..20059c3308 100755
    --- a/t/t5512-ls-remote.sh
    +++ b/t/t5512-ls-remote.sh
    @@ -210,6 +210,13 @@ test_expect_success 'protocol v2 supports hiderefs' '
     	! grep refs/tags actual
     '

    +test_expect_success 'protocol v2 respects hiderefs when pushing' '
    +	git init --bare server.git &&
    +	git -C server.git config transfer.hideRefs refs/tags &&
    +	test_must_fail git -c protocol.version=0 push "file://$PWD/server.git" mark &&
    +	test_must_fail git -c protocol.version=2 push "file://$PWD/server.git" mark
    +'
    +
     test_expect_success 'ls-remote --symref' '
     	git fetch origin &&
     	cat >expect <<-EOF &&

If there's some bug where you can bypass this push protection that would be much worse. E.g. GitLab uses "keep-around" refs to track its own internal state, and it would be bad if users could manipulate it.

Show 12 quoted lines

>  builtin/upload-pack.c |  1 +
>  ls-refs.c             | 16 +++++++++++++++-
>  ls-refs.h             |  3 ++-
>  refs.c                |  3 ++-
>  serve.c               |  9 +++++----
>  serve.h               |  7 +++++++
>  t/t5512-ls-remote.sh  |  6 ++++++
>  upload-pack.c         |  4 ++--
>  upload-pack.h         |  4 ++--
>  9 files changed, 42 insertions(+), 11 deletions(-)
>
> -Peff