git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Brandon Williams <bwilliamseng@gmail.com>,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
Date: Mon, 17 Dec 2018 14:57:13 -0500	[thread overview]
Message-ID: <20181217195713.GA10673@sigill.intra.peff.net> (raw)
In-Reply-To: <87o99o1iot.fsf@evledraar.gmail.com>

On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote:

> > An interesting implication of this at GitHub (and possibly other
> > hosters) is that it exposes objects from shared storage via unexpected
> > repos. If I fork torvalds/linux to peff/linux and push up object X, a v0
> > fetch will (by default) refuse to serve it to me. But v2 will happily
> > hand it over, which may confuse people into thinking that the object is
> > (or at least at some point was) in Linus's repository.
> 
> More importantly this bypasses the security guarantee we've had with the
> default of uploadpack.allowAnySHA1InWant=false.

IMHO those security guarantees there are overrated (due to delta
guessing attacks, though things are not quite as bad if the attacker
can't actually push to the repo).

But I agree that people do assume it's the case. I was certainly
surprised by the v2 behavior, and I don't remember that aspect being
discussed.

(I suspect it was mostly because in the stateless world, the "fetch"
operation requires iterating all of the refs again. That's made even
harder by the fact that "ls-refs" takes arguments now, and might have
only advertised a subset of the refs. How can "fetch" know which ones
were advertised?).

>  a) Subject to the "SECURITY" section of git-fetch, i.e. maybe this
>     doesn't even work in the face of a determined attacker.

Yeah, this is the part I was thinking about above.

>  b) Hosting sites like GitHub, GitLab, Gitweb etc. aren't doing
>     reachability checks when you view /commit/<id>. If I delete my topic
>     branch it'll be viewable until the next GC kicks in (which would
>     also be the case with uploadpack.allowAnySHA1InWant=true).

Actually, we don't ever prune unless a user asks us to (and certainly
users do ask us to after accidentally pushing secret stuff). In GitHub's
case, though, we always tell people to consider anything pushed to a
public repo to be non-secret, even if it was exposed for only a few
seconds. My understanding is that there are literally clients watching
the public feeds and sucking down repo data looking for these kinds of
mistakes.

The /commit/<id> thing has been a frequent topic of discussion within
GitHub over the years, and we have looked at actually doing reachability
checks. They're expensive, though bitmaps help.

> So I wonder how much we need to care about this in practice, but for
> some configurations protocol v2 definitely breaks a security promise
> we've been making, now whether that promise was always BS due to "a)"
> above is another matter.
> 
> I'm inclined to say that in the face of that "SECURITY" section we
> should just:
> 
>  * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
>    default. Make saying uploadpack.allowReachableSHA1InWant=false warn
>    with "this won't work, see SECURITY...".
> 
>  * The uploadpack.allowTipSHA1InWant setting will also be turned on by
>    default, and will be much faster, since it'll just degrade to
>    uploadpack.allowReachableSHA1InWant=true and we won't need any
>    reachability check. We'll also warn saying that setting it is
>    useless.

No real argument from me. I have always thought those security
guarantees were BS.

However, I do think that's all orthogonal to the issue I was mentioning,
which is that it looks like repo A wants to serve you object X, even if
that repo never saw the object. That's unique to shared storage schemes,
though.

IMHO this is also a user education issue (it's a question of trust: refs
are the source of trust, and objects don't need to be trusted because
they're immutable). But it's pretty subtle for people who don't know the
inner workings of Git.

-Peff

  parent reply	other threads:[~2018-12-17 19:57 UTC|newest]

Thread overview: 73+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-11 10:42 [PATCH 0/3] protocol v2 and hidden refs Jeff King
2018-12-11 10:43 ` [PATCH 1/3] serve: pass "config context" through to individual commands Jeff King
2018-12-14  2:09   ` Junio C Hamano
2018-12-14  8:20     ` Jeff King
2018-12-15  0:31       ` Junio C Hamano
2018-12-16 10:25         ` Jeff King
2018-12-16 11:12           ` Junio C Hamano
2018-12-18 12:47             ` Jeff King
2018-12-14  8:36   ` Jonathan Nieder
2018-12-14  8:55     ` Jeff King
2018-12-14  9:28       ` Jonathan Nieder
2018-12-14  9:55         ` Jeff King
2018-12-11 10:43 ` [PATCH 2/3] parse_hide_refs_config: handle NULL section Jeff King
2018-12-14  2:11   ` Junio C Hamano
2018-12-11 10:44 ` [PATCH 3/3] upload-pack: support hidden refs with protocol v2 Jeff King
2018-12-11 11:45 ` [PATCH 0/3] protocol v2 and hidden refs Ævar Arnfjörð Bjarmason
2018-12-11 13:55   ` Jeff King
2018-12-11 21:21     ` [PATCH 0/3] Add a GIT_TEST_PROTOCOL_VERSION=X test mode Ævar Arnfjörð Bjarmason
2018-12-11 21:24       ` Ævar Arnfjörð Bjarmason
2018-12-11 21:21     ` [PATCH 1/3] tests: add a special setup where for protocol.version Ævar Arnfjörð Bjarmason
2018-12-12  0:27       ` [PATCH 0/3] Some fixes and improvements Jonathan Tan
2018-12-12  0:27         ` [PATCH 1/3] squash this into your patch Jonathan Tan
2018-12-12  0:27         ` [PATCH 2/3] builtin/fetch-pack: support protocol version 2 Jonathan Tan
2018-12-12  0:27         ` [PATCH 3/3] also squash this into your patch Jonathan Tan
2018-12-13  2:49         ` [PATCH 0/3] Some fixes and improvements Junio C Hamano
2018-12-13 15:58           ` [PATCH v2 0/8] protocol v2 fixes Ævar Arnfjörð Bjarmason
2018-12-17 22:40             ` [PATCH v3 0/4] " Ævar Arnfjörð Bjarmason
2018-12-18 12:48               ` Jeff King
2018-12-17 22:40             ` [PATCH v3 1/4] serve: pass "config context" through to individual commands Ævar Arnfjörð Bjarmason
2018-12-17 22:40             ` [PATCH v3 2/4] parse_hide_refs_config: handle NULL section Ævar Arnfjörð Bjarmason
2018-12-17 22:40             ` [PATCH v3 3/4] upload-pack: support hidden refs with protocol v2 Ævar Arnfjörð Bjarmason
2018-12-17 22:40             ` [PATCH v3 4/4] fetch-pack: support protocol version 2 Ævar Arnfjörð Bjarmason
2019-01-08 19:45               ` Junio C Hamano
2019-01-08 20:38                 ` Jonathan Tan
2019-01-08 21:14                   ` Jeff King
2018-12-13 15:58           ` [PATCH v2 1/8] serve: pass "config context" through to individual commands Ævar Arnfjörð Bjarmason
2018-12-13 15:58           ` [PATCH v2 2/8] parse_hide_refs_config: handle NULL section Ævar Arnfjörð Bjarmason
2018-12-13 15:58           ` [PATCH v2 3/8] upload-pack: support hidden refs with protocol v2 Ævar Arnfjörð Bjarmason
2018-12-13 15:58           ` [PATCH v2 4/8] tests: add a check for unportable env --unset Ævar Arnfjörð Bjarmason
2018-12-13 15:58           ` [PATCH v2 5/8] tests: add a special setup where for protocol.version Ævar Arnfjörð Bjarmason
2018-12-13 19:48             ` Jonathan Tan
2018-12-13 15:58           ` [PATCH v2 6/8] tests: mark & fix tests broken under GIT_TEST_PROTOCOL_VERSION=1 Ævar Arnfjörð Bjarmason
2018-12-13 15:58           ` [PATCH v2 7/8] builtin/fetch-pack: support protocol version 2 Ævar Arnfjörð Bjarmason
2018-12-14 10:17             ` Jeff King
2018-12-13 15:58           ` [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2 Ævar Arnfjörð Bjarmason
2018-12-13 16:08             ` Ævar Arnfjörð Bjarmason
2018-12-14  2:18               ` Junio C Hamano
2018-12-14 10:12               ` Jeff King
2018-12-14 10:55                 ` Ævar Arnfjörð Bjarmason
2018-12-14 11:08                   ` Ævar Arnfjörð Bjarmason
2018-12-17 19:59                     ` Jeff King
2018-12-17 19:57                   ` Jeff King [this message]
2018-12-17 22:16                     ` [PATCH] upload-pack: turn on uploadpack.allowAnySHA1InWant=true Ævar Arnfjörð Bjarmason
2018-12-17 22:34                       ` David Turner
2018-12-17 22:57                         ` Ævar Arnfjörð Bjarmason
2018-12-17 23:07                           ` David Turner
2018-12-17 23:14                     ` [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2 Jonathan Nieder
2018-12-17 23:36                       ` Ævar Arnfjörð Bjarmason
2018-12-18  0:02                         ` Jonathan Nieder
2018-12-18  9:28                           ` Ævar Arnfjörð Bjarmason
2018-12-18 12:41                             ` Jeff King
2018-12-18 12:36                       ` Jeff King
2018-12-18 13:10                         ` Ævar Arnfjörð Bjarmason
2018-12-26 22:14                           ` Junio C Hamano
2018-12-27 11:26                             ` Ævar Arnfjörð Bjarmason
2018-12-27 17:10                               ` Jonathan Nieder
2018-12-11 21:21     ` [PATCH 2/3] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=1 Ævar Arnfjörð Bjarmason
2018-12-11 21:21     ` [PATCH 3/3] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2 Ævar Arnfjörð Bjarmason
2018-12-13 19:53 ` [PATCH 0/3] protocol v2 and hidden refs Jonathan Tan
2018-12-14  8:35   ` Jeff King
2018-12-15 19:53     ` Ævar Arnfjörð Bjarmason
2018-12-16 10:40       ` Jeff King
2018-12-16 11:47         ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181217195713.GA10673@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=bwilliamseng@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).