Fetch/push lets a malicious server steal the targets of "have" lines

Fetch/push lets a malicious server steal the targets of "have" lines

[Matt McCutchen]

I was studying the fetch protocol and I realized that in a scenario in
which a client regularly fetches a set of refs from a server and pushes
them back without careful scrutiny, the server can steal the targets of
unrelated refs from the client repository by fabricating its own refs
to the "have" objects specified by the client during the fetch.  This
is the reverse of attack #1 described in the "SECURITY" section of the
gitnamespaces(7) man page, with the addition that the server doesn't
have to know the object IDs in advance.  Is this supposed to be well-
known?  I've been using git since 2006 and it was a surprise to me.

Hopefully it isn't very common for a user to fetch and push with a
server they don't trust to have all the data in their repository.  I
don't think I have any such cases myself; I have unfinished work that
isn't meant for scrutiny by others, but nothing really damaging if it
were released to the server.  This attack presents no new risks if a
user already runs code fetched from the server in such a way that it
can read the repository.  But there might be some users who just review
embargoed security fixes from multiple sources (or something like that)
without running code themselves, and their security expectations might
be violated.

If my analysis is correct, I'd argue for documenting the issue in a
"SECURITY" section in the git-fetch man page.  Shall I submit a patch?

Thanks for your attention.

Matt

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Junio C Hamano]

Matt McCutchen <matt@mattmccutchen.net> writes:

> I was studying the fetch protocol and I realized that in a scenario in
> which a client regularly fetches a set of refs from a server and pushes
> them back without careful scrutiny, the server can steal the targets of
> unrelated refs from the client repository by fabricating its own refs
> to the "have" objects specified by the client during the fetch.

Let me see if I understood your scenario correctly.

Suppose we start from this history where 'O' are common, your victim
has a 'Y' branch with two commits that are private to it, as well as
a 'X' branch on which it has X1 that it previously obtained from the
server.  On the other hand, the server does not know about Y1 or Y2,
and it added one commit X2 to the branch 'x' the victim is
following:

           victim                server

             Y1---Y2               
            /                      
    ---O---O---X1           ---O---O---X1---X2

Then when victim wants to fetch 'x' from the server, it would say

    have X1, have Y2, have Y1, have O

and gets told to shut up by the server who heard enough.  The
histories on these two parties will then become like this:


           victim                server

             Y1---Y2               
            /                      
    ---O---O---X1---X2      ---O---O---X1---X2

Victim wishes to keep Y1 and Y2 private, but pushes some other
branch (perhaps builds X3 on top of X2 and pushes 'x').  On push
protocol, the server would lie to the victim that it has Y2 without
knowing what they are.

Is that how your attack scenario goes?

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Matt McCutchen]

On Fri, 2016-10-28 at 15:00 -0700, Junio C Hamano wrote:
> Let me see if I understood your scenario correctly.
> 
> Suppose we start from this history where 'O' are common, your victim
> has a 'Y' branch with two commits that are private to it, as well as
> a 'X' branch on which it has X1 that it previously obtained from the
> server.  On the other hand, the server does not know about Y1 or Y2,
> and it added one commit X2 to the branch 'x' the victim is
> following:
> 
>            victim                server
> 
>              Y1---Y2               
>             /                      
>     ---O---O---X1           ---O---O---X1---X2
> 
> Then when victim wants to fetch 'x' from the server, it would say
> 
>     have X1, have Y2, have Y1, have O
> 
> and gets told to shut up by the server who heard enough.  The
> histories on these two parties will then become like this:
> 
> 
>            victim                server
> 
>              Y1---Y2               
>             /                      
>     ---O---O---X1---X2      ---O---O---X1---X2

Then the server generates a commit X3 that lists Y2 as a parent, even
though it doesn't have Y2, and advances 'x' to X3.  The victim fetches
'x':

           victim                  server

             Y1---Y2----                      (Y2)
            /           \                         \ 
    ---O---O---X1---X2---X3   ---O---O---X1---X2---X3

Then the server rolls back 'x' to X2:

           victim                  server

             Y1---Y2----
            /           \
    ---O---O---X1---X2---X3   ---O---O---X1---X2

And the victim pushes:

           victim                  server

             Y1---Y2----               Y1---Y2----
            /           \             /           \
    ---O---O---X1---X2---X3   ---O---O---X1---X2---X3

Now the server has the content of Y2.

If the victim is fetching and pulling a whole "directory" of refs, e.g:

fetch: refs/heads/*:refs/remotes/server1/*
push: refs/heads/for-server1/*:refs/heads/*

then instead of generating a merge commit, the server can just generate
another ref 'xx' pointing to Y2, assuming it can entice the victim to
set up a corresponding local branch refs/heads/for-server1/xx and push
it back.  Or if the victim is for some reason just mirroring back and
forth:

fetch: refs/heads/*:refs/heads/for-server1/*
push: refs/heads/for-
server1/*:refs/heads/*

then it doesn't have to set up a local branch as separate step.

Matt

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Junio C Hamano]

Matt McCutchen <matt@mattmccutchen.net> writes:

> Then the server generates a commit X3 that lists Y2 as a parent, even
> though it doesn't have Y2, and advances 'x' to X3.  The victim fetches
> 'x':
>
>            victim                  server
>
>              Y1---Y2----                      (Y2)
>             /           \                         \ 
>     ---O---O---X1---X2---X3   ---O---O---X1---X2---X3
>
> Then the server rolls back 'x' to X2:
>
>            victim                  server
>
>              Y1---Y2----
>             /           \
>     ---O---O---X1---X2---X3   ---O---O---X1---X2

Ah, I see.  My immediate reaction is that you can do worse things in
the reverse direction compared to this, but your scenario does sound
bad already.

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Matt McCutchen]

On Fri, 2016-10-28 at 18:11 -0700, Junio C Hamano wrote:
> Ah, I see.  My immediate reaction is that you can do worse things in
> the reverse direction compared to this, but your scenario does sound
> bad already.

Are you saying that clients connecting to untrusted servers already
face worse risks that people should know about, so there is no point in
documenting this one?  I guess I don't know about the other risks aside
from accepting a corrupt object, which should be preventable by
enabling fetch.fsckObjects.  It seems we need either a statement that
connecting to untrusted servers is officially unsupported or a
description of the specific risks.

Matt

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Jeff King]

On Fri, Oct 28, 2016 at 11:33:49PM -0400, Matt McCutchen wrote:

> On Fri, 2016-10-28 at 18:11 -0700, Junio C Hamano wrote:
> > Ah, I see.  My immediate reaction is that you can do worse things in
> > the reverse direction compared to this, but your scenario does sound
> > bad already.
> 
> Are you saying that clients connecting to untrusted servers already
> face worse risks that people should know about, so there is no point in
> documenting this one?  I guess I don't know about the other risks aside
> from accepting a corrupt object, which should be preventable by
> enabling fetch.fsckObjects.  It seems we need either a statement that
> connecting to untrusted servers is officially unsupported or a
> description of the specific risks.

I'm not sure I understand how connecting to a remote server to fetch is
a big problem. The server may learn about the existence of particular
sha1s in your repository, but cannot get their content.

It's the subsequent push that is a problem.

In the scenarios you've described, I'm mostly inclined to say that the
problem is not git or the protocol itself, but rather lax refspecs.
You mentioned earlier:

  the server can just generate another ref 'xx' pointing to Y2, assuming
  it can entice the victim to set up a corresponding local branch
  refs/heads/for-server1/xx and push it back.  Or if the victim is for
  some reason just mirroring back and forth:

This sounds a lot like "I told git to push a bunch of things without
checking if they were really secret, and it turned out to push some
secret things". IOW I think the problem is not that the server may lie
about what it has, but that the user was not careful about what they
pushed. I dunno. I do not mind making a note in the documentation
explaining the implications of a server lying, but the scenarios seem
pretty contrived to me.

A much more interesting one, IMHO, is a server whose receive-pack lies
about which objects it has (possibly ones it found out about earlier via
fetch), which provokes the client to generate deltas against objects the
server doesn't have (and thereby leaking information about the base
objects).

That is a problem no matter how careful your refspecs are. I suspect it
would be a hard attack to pull off in practice, just because it's going
to depend heavily on the content of the specific objects, what kinds of
deltas you can convince the other side to generate, etc. That might
merit a mention in the git-push documentation.

-Peff

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Matt McCutchen]

On Sat, 2016-10-29 at 09:39 -0400, Jeff King wrote:
> I'm not sure I understand how connecting to a remote server to fetch is
> a big problem. The server may learn about the existence of particular
> sha1s in your repository, but cannot get their content.
> 
> It's the subsequent push that is a problem.
> 
> In the scenarios you've described, I'm mostly inclined to say that the
> problem is not git or the protocol itself, but rather lax refspecs.
> You mentioned earlier:
> 
>   the server can just generate another ref 'xx' pointing to Y2, assuming
>   it can entice the victim to set up a corresponding local branch
>   refs/heads/for-server1/xx and push it back.  Or if the victim is for
>   some reason just mirroring back and forth:
> 
> This sounds a lot like "I told git to push a bunch of things without
> checking if they were really secret, and it turned out to push some
> secret things". IOW I think the problem is not that the server may lie
> about what it has, but that the user was not careful about what they
> pushed. I dunno. I do not mind making a note in the documentation
> explaining the implications of a server lying, but the scenarios seem
> pretty contrived to me.

Let's focus on the first scenario.  There the user is just pulling and
pushing a master branch.  Are you saying that each time the user pulls,
they need to look over all the commits they pulled before pushing them
back?  I think that's unrealistic, for example, on a busy project with
centralized code review or if the user is publishing a project-specific 
modified version of an upstream library.  The natural user expectation
is that anything pulled from a public repository is public.

But let's see what Junio says in the other subthread.

> A much more interesting one, IMHO, is a server whose receive-pack lies
> about which objects it has (possibly ones it found out about earlier via
> fetch), which provokes the client to generate deltas against objects the
> server doesn't have (and thereby leaking information about the base
> objects).
> 
> That is a problem no matter how careful your refspecs are. I suspect it
> would be a hard attack to pull off in practice, just because it's going
> to depend heavily on the content of the specific objects, what kinds of
> deltas you can convince the other side to generate, etc. That might
> merit a mention in the git-push documentation.

Sure, if I end up doing a patch, I'll include this.

Matt

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Jeff King]

On Sat, Oct 29, 2016 at 12:08:31PM -0400, Matt McCutchen wrote:

> Let's focus on the first scenario.  There the user is just pulling and
> pushing a master branch.  Are you saying that each time the user pulls,
> they need to look over all the commits they pulled before pushing them
> back?  I think that's unrealistic, for example, on a busy project with
> centralized code review or if the user is publishing a project-specific 
> modified version of an upstream library.  The natural user expectation
> is that anything pulled from a public repository is public.

No, I'm saying if you are running "git push foo master", then you should
expect the contents of "master" to go to "foo". That _could_ have
security implications if you come up with a sequence of events where
secret things made it to "master". But it seems to me that "foo
previously lied to you about what it has" is not the weak link in that
chain. It is not thinking about what secret things are hitting the
master that you are pushing, no matter how they got there.

I agree there is a potential workflow (that you have laid out) where
such lying can cause an innocent-looking sequence of events to disclose
the secret commits. And again, I don't mind a note in the documentation
mentioning that. I just have trouble believing it's a common one in
practice.

The reason I brought up the delta thing, even though it's a much harder
attack to execute, is that it comes up in much more common workflows,
like simply fetching from a private security-sensitive repo into your
"main" public repo (which is an example you brought up, and something I
know that I have personally done in the past for git.git).

-Peff

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... It is not thinking about what secret things are hitting the
> master that you are pushing, no matter how they got there.
>
> I agree there is a potential workflow (that you have laid out) where
> such lying can cause an innocent-looking sequence of events to disclose
> the secret commits. And again, I don't mind a note in the documentation
> mentioning that. I just have trouble believing it's a common one in
> practice.

I'd say I agree with the above.  I am not sure how easy people
employing common workflows can be tricked into the scenario Matt
presented, either, but I do not think it would hurt to warn people
that they need to be careful not to pull from or push to an
untrustworthy place or push things you are not sure that are clean.

> The reason I brought up the delta thing, even though it's a much harder
> attack to execute, is that it comes up in much more common workflows,
> like simply fetching from a private security-sensitive repo into your
> "main" public repo (which is an example you brought up, and something I
> know that I have personally done in the past for git.git).

Yup.

[PATCH] fetch/push: document that private data can be leaked

[Matt McCutchen]

A malicious server may be able to use the fetch and push protocols to
steal data from a user's repository that the user did not intend to
share, via attacks similar to those described in the gitnamespaces(7)
man page. Mention this in the git-fetch(1), git-pull(1), and git-push(1)
man pages and recommend using separate repositories for private data and
interaction with untrusted servers.

Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---

And here's a proposed patch.  Based on the maint branch, ac84098.

 Documentation/fetch-push-security.txt | 9 +++++++++
 Documentation/git-fetch.txt           | 2 ++
 Documentation/git-pull.txt            | 2 ++
 Documentation/git-push.txt            | 2 ++
 4 files changed, 15 insertions(+)
 create mode 100644 Documentation/fetch-push-security.txt

diff --git a/Documentation/fetch-push-security.txt b/Documentation/fetch-push-security.txt
new file mode 100644
index 0000000..00944ed
--- /dev/null
+++ b/Documentation/fetch-push-security.txt
@@ -0,0 +1,9 @@
+SECURITY
+--------
+The fetch and push protocols are not designed to prevent a malicious
+server from stealing data from your repository that you did not intend to
+share. The possible attacks are similar to the ones described in the
+"SECURITY" section of linkgit:gitnamespaces[7]. If you have private data
+that you need to protect from the server, keep it in a separate
+repository.
+
diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index 9e42169..a461b4b 100644
--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -192,6 +192,8 @@ The first command fetches the `maint` branch from the repository at
 objects will eventually be removed by git's built-in housekeeping (see
 linkgit:git-gc[1]).
 
+include::fetch-push-security.txt[]
+
 BUGS
 ----
 Using --recurse-submodules can only fetch new commits in already checked
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index d033b25..0af2de9 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -237,6 +237,8 @@ If you tried a pull which resulted in complex conflicts and
 would want to start over, you can recover with 'git reset'.
 
 
+include::fetch-push-security.txt[]
+
 BUGS
 ----
 Using --recurse-submodules can only fetch new commits in already checked
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 47b77e6..5ebef9e 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -559,6 +559,8 @@ Commits A and B would no longer belong to a branch with a symbolic name,
 and so would be unreachable.  As such, these commits would be removed by
 a `git gc` command on the origin repository.
 
+include::fetch-push-security.txt[]
+
 GIT
 ---
 Part of the linkgit:git[1] suite
-- 
2.7.4

Re: [PATCH] fetch/push: document that private data can be leaked

[Junio C Hamano]

Matt McCutchen <matt@mattmccutchen.net> writes:

>  Documentation/fetch-push-security.txt | 9 +++++++++

A new (consolidated) piece like this that can be included in
multiple places is a good idea.  I wonder if the original
description in "namespaces" thing can be moved here and then
"namespaces" page can be made to also borrow from this?

>  Documentation/git-fetch.txt           | 2 ++
>  Documentation/git-pull.txt            | 2 ++
>  Documentation/git-push.txt            | 2 ++
>  4 files changed, 15 insertions(+)
>  create mode 100644 Documentation/fetch-push-security.txt
>
> diff --git a/Documentation/fetch-push-security.txt b/Documentation/fetch-push-security.txt
> new file mode 100644
> index 0000000..00944ed
> --- /dev/null
> +++ b/Documentation/fetch-push-security.txt
> @@ -0,0 +1,9 @@
> +SECURITY
> +--------
> +The fetch and push protocols are not designed to prevent a malicious
> +server from stealing data from your repository that you did not intend to
> +share. The possible attacks are similar to the ones described in the
> +"SECURITY" section of linkgit:gitnamespaces[7]. If you have private data
> +that you need to protect from the server, keep it in a separate
> +repository.

Yup, and then "do not push to untrustworthy place without checking
what you are pushing", too?

> diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt

These three look sensible.

Re: [PATCH] fetch/push: document that private data can be leaked

[Matt McCutchen]

On Sun, 2016-11-13 at 18:57 -0800, Junio C Hamano wrote:
> Matt McCutchen <matt@mattmccutchen.net> writes:
> 
> > 
> >  Documentation/fetch-push-security.txt | 9 +++++++++
> 
> A new (consolidated) piece like this that can be included in
> multiple places is a good idea.  I wonder if the original
> description in "namespaces" thing can be moved here and then
> "namespaces" page can be made to also borrow from this?

I gave this a try.  New patch coming.

> > --- /dev/null
> > +++ b/Documentation/fetch-push-security.txt
> > @@ -0,0 +1,9 @@
> > +SECURITY
> > +--------
> > +The fetch and push protocols are not designed to prevent a
> > malicious
> > +server from stealing data from your repository that you did not
> > intend to
> > +share. The possible attacks are similar to the ones described in
> > the
> > +"SECURITY" section of linkgit:gitnamespaces[7]. If you have
> > private data
> > +that you need to protect from the server, keep it in a separate
> > +repository.
> 
> Yup, and then "do not push to untrustworthy place without checking
> what you are pushing", too?

If there is no private data in the repository, then there is no need
for the user to check what they are pushing.  As I've indicated before,
IMO manually checking each push would not be a workable security
measure in the long term anyway.

Matt

[PATCH] doc: mention transfer data leaks in more places

[Matt McCutchen]

The "SECURITY" section of the gitnamespaces(7) man page described two
ways for a client to steal data from a server that wasn't intended to be
shared. Similar attacks can be performed by a server on a client, so
adapt the section to cover both directions and add it to the
git-fetch(1), git-pull(1), and git-push(1) man pages. Also add
references to this section from the documentation of server
configuration options that attempt to control data leakage but may not
be fully effective.

Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---
 Documentation/config.txt              | 17 ++++++++++++++---
 Documentation/git-fetch.txt           |  2 ++
 Documentation/git-pull.txt            |  2 ++
 Documentation/git-push.txt            |  2 ++
 Documentation/gitnamespaces.txt       | 20 +-------------------
 Documentation/transfer-data-leaks.txt | 30 ++++++++++++++++++++++++++++++
 6 files changed, 51 insertions(+), 22 deletions(-)
 create mode 100644 Documentation/transfer-data-leaks.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 21fdddf..fc2cf83 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2898,6 +2898,11 @@ is omitted from the advertisements but `refs/heads/master` and
 `refs/namespaces/bar/refs/heads/master` are still advertised as so-called
 "have" lines. In order to match refs before stripping, add a `^` in front of
 the ref name. If you combine `!` and `^`, `!` must be specified first.
++
+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
+linkgit:gitnamespaces[7] man page; it's best to keep private data in a
+separate repository.
 
 transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
@@ -2907,7 +2912,7 @@ transfer.unpackLimit::
 uploadarchive.allowUnreachable::
 	If true, allow clients to use `git archive --remote` to request
 	any tree, whether reachable from the ref tips or not. See the
-	discussion in the `SECURITY` section of
+	discussion in the "SECURITY" section of
 	linkgit:git-upload-archive[1] for more details. Defaults to
 	`false`.
 
@@ -2921,13 +2926,19 @@ uploadpack.allowTipSHA1InWant::
 	When `uploadpack.hideRefs` is in effect, allow `upload-pack`
 	to accept a fetch request that asks for an object at the tip
 	of a hidden ref (by default, such a request is rejected).
-	see also `uploadpack.hideRefs`.
+	See also `uploadpack.hideRefs`.  Even if this is false, a client
+	may be able to steal objects via the techniques described in the
+	"SECURITY" section of the linkgit:gitnamespaces[7] man page; it's
+	best to keep private data in a separate repository.
 
 uploadpack.allowReachableSHA1InWant::
 	Allow `upload-pack` to accept a fetch request that asks for an
 	object that is reachable from any ref tip. However, note that
 	calculating object reachability is computationally expensive.
-	Defaults to `false`.
+	Defaults to `false`.  Even if this is false, a client may be able
+	to steal objects via the techniques described in the "SECURITY"
+	section of the linkgit:gitnamespaces[7] man page; it's best to
+	keep private data in a separate repository.
 
 uploadpack.keepAlive::
 	When `upload-pack` has started `pack-objects`, there may be a
diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index 9e42169..b153aef 100644
--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -192,6 +192,8 @@ The first command fetches the `maint` branch from the repository at
 objects will eventually be removed by git's built-in housekeeping (see
 linkgit:git-gc[1]).
 
+include::transfer-data-leaks.txt[]
+
 BUGS
 ----
 Using --recurse-submodules can only fetch new commits in already checked
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index d033b25..4470e4b 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -237,6 +237,8 @@ If you tried a pull which resulted in complex conflicts and
 would want to start over, you can recover with 'git reset'.
 
 
+include::transfer-data-leaks.txt[]
+
 BUGS
 ----
 Using --recurse-submodules can only fetch new commits in already checked
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 47b77e6..8eefabd 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -559,6 +559,8 @@ Commits A and B would no longer belong to a branch with a symbolic name,
 and so would be unreachable.  As such, these commits would be removed by
 a `git gc` command on the origin repository.
 
+include::transfer-data-leaks.txt[]
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Documentation/gitnamespaces.txt b/Documentation/gitnamespaces.txt
index 7685e36..b614969 100644
--- a/Documentation/gitnamespaces.txt
+++ b/Documentation/gitnamespaces.txt
@@ -61,22 +61,4 @@ For a simple local test, you can use linkgit:git-remote-ext[1]:
 git clone ext::'git --namespace=foo %s /tmp/prefixed.git'
 ----------
 
-SECURITY
---------
-
-Anyone with access to any namespace within a repository can potentially
-access objects from any other namespace stored in the same repository.
-You can't directly say "give me object ABCD" if you don't have a ref to
-it, but you can do some other sneaky things like:
-
-. Claiming to push ABCD, at which point the server will optimize out the
-  need for you to actually send it. Now you have a ref to ABCD and can
-  fetch it (claiming not to have it, of course).
-
-. Requesting other refs, claiming that you have ABCD, at which point the
-  server may generate deltas against ABCD.
-
-None of this causes a problem if you only host public repositories, or
-if everyone who may read one namespace may also read everything in every
-other namespace (for instance, if everyone in an organization has read
-permission to every repository).
+include::transfer-data-leaks.txt[]
diff --git a/Documentation/transfer-data-leaks.txt b/Documentation/transfer-data-leaks.txt
new file mode 100644
index 0000000..914bacc
--- /dev/null
+++ b/Documentation/transfer-data-leaks.txt
@@ -0,0 +1,30 @@
+SECURITY
+--------
+The fetch and push protocols are not designed to prevent one side from
+stealing data from the other repository that was not intended to be
+shared. If you have private data that you need to protect from a malicious
+peer, your best option is to store it in another repository. This applies
+to both clients and servers. In particular, namespaces on a server are not
+effective for read access control; you should only grant read access to a
+namespace to clients that you would trust with read access to the entire
+repository.
+
+The known attack vectors are as follows:
+
+. The victim sends "have" lines advertising the IDs of objects it has that
+  are not explicitly intended to be shared but can be used to optimize the
+  transfer if the peer also has them. The attacker chooses an object ID X
+  to steal and sends a ref to X, but isn't required to send the content of
+  X because the victim already has it. Now the victim believes that the
+  attacker has X, and it sends the content of X back to the attacker
+  later. (This attack is most straightforward for a client to perform on a
+  server, by creating a ref to X in the namespace the client has access
+  to and then fetching it. The most likely way for a server to perform it
+  on a client is to "merge" X into a public branch and hope that the user
+  does additional work on this branch and pushes it back to the server
+  without noticing the merge.)
+
+. As in #1, the attacker chooses an object ID X to steal. The victim sends
+  an object Y that the attacker already has, and the attacker falsely
+  claims to have X and not Y, so the victim sends Y as a delta against X.
+  The delta reveals regions of X that are similar to Y to the attacker.
-- 
2.7.4

Re: [PATCH] doc: mention transfer data leaks in more places

[Junio C Hamano]

Matt McCutchen <matt@mattmccutchen.net> writes:

> The "SECURITY" section of the gitnamespaces(7) man page described two
> ways for a client to steal data from a server that wasn't intended to be
> shared. Similar attacks can be performed by a server on a client, so
> adapt the section to cover both directions and add it to the
> git-fetch(1), git-pull(1), and git-push(1) man pages. Also add
> references to this section from the documentation of server
> configuration options that attempt to control data leakage but may not
> be fully effective.

This round looks OK.  Will queue.  Thanks.

Re: [PATCH] fetch/push: document that private data can be leaked

[Junio C Hamano]

Matt McCutchen <matt@mattmccutchen.net> writes:

>> Yup, and then "do not push to untrustworthy place without checking
>> what you are pushing", too?
>
> If there is no private data in the repository, then there is no need
> for the user to check what they are pushing. As I've indicated before,
> IMO manually checking each push would not be a workable security
> measure in the long term anyway.

Then what is?  Don't answer; this is a rhetorical question.

The answer is "do not push to untrustworthy place", if you are
unable to check what you are pushing.

Re: [PATCH] fetch/push: document that private data can be leaked

[Jeff King]

On Mon, Nov 14, 2016 at 11:00:04AM -0800, Junio C Hamano wrote:

> Matt McCutchen <matt@mattmccutchen.net> writes:
> 
> >> Yup, and then "do not push to untrustworthy place without checking
> >> what you are pushing", too?
> >
> > If there is no private data in the repository, then there is no need
> > for the user to check what they are pushing. As I've indicated before,
> > IMO manually checking each push would not be a workable security
> > measure in the long term anyway.
> 
> Then what is?  Don't answer; this is a rhetorical question.
> 
> The answer is "do not push to untrustworthy place", if you are
> unable to check what you are pushing.

I think "check what you are pushing" only covers one case (attacker lies
to you during a fetch, and you accidentally push that back, thinking
they already have it).

But consider the other case mentioned: the attacker lies to you while
pushing and _says_ they have X, then deduces information from the delta
you generate. The only advice there is "do not push to an untrusted
place from a repository containing private objects".

So I think the in-between answer is "it is OK to push to an
untrustworthy place, but do not do it from a repo that may contain
secret contents".

-Peff

Re: [PATCH] fetch/push: document that private data can be leaked

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I think the in-between answer is "it is OK to push to an
> untrustworthy place, but do not do it from a repo that may contain
> secret contents".

Yes, that sounds like a sensible piece of advice to give to the
readers.

Re: [PATCH] fetch/push: document that private data can be leaked

[Matt McCutchen]

On Mon, 2016-11-14 at 11:00 -0800, Junio C Hamano wrote:
> Matt McCutchen <matt@mattmccutchen.net> writes:
> 
> > 
> > > 
> > > Yup, and then "do not push to untrustworthy place without
> > > checking
> > > what you are pushing", too?
> > 
> > If there is no private data in the repository, then there is no
> > need
> > for the user to check what they are pushing. As I've indicated
> > before,
> > IMO manually checking each push would not be a workable security
> > measure in the long term anyway.
> 
> Then what is?

Don't put private data in the same repository, then the whole issue
becomes moot.  Am I missing something?

Matt

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Matt McCutchen]

On Fri, 2016-10-28 at 22:31 -0700, Junio C Hamano wrote:
> Not sending to the list, where mails from Gmail/phone is known to get
> rejected.

[I guess I can go ahead and quote this to the list.]

> No. I'm saying that the scenario you gave is bad and people should be
> taught not to connect to untrustworthy sites.

To clarify, are you saying:

(1) don't connect to an untrusted server ever (e.g., we don't promise
that the server can't execute arbitrary code on the client), or

(2) don't connect to an untrusted server if the client repository has
data that needs to be kept secret from the server?

The fetch/push attack relates only to #2.  If #1, what are the other
risks you are thinking of?

Matt

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Junio C Hamano]

Matt McCutchen <matt@mattmccutchen.net> writes:

> On Fri, 2016-10-28 at 22:31 -0700, Junio C Hamano wrote:
>> Not sending to the list, where mails from Gmail/phone is known to get
>> rejected.
>
> [I guess I can go ahead and quote this to the list.]
>
>> No. I'm saying that the scenario you gave is bad and people should be
>> taught not to connect to untrustworthy sites.
>
> To clarify, are you saying:
>
> (1) don't connect to an untrusted server ever (e.g., we don't promise
> that the server can't execute arbitrary code on the client), or
>
> (2) don't connect to an untrusted server if the client repository has
> data that needs to be kept secret from the server?

You sneaked "arbitrary code execution" into the discussion but I do
not know where it came from.  In any case, "don't pull from or push
to untrustworthy place" would be a common sense advice that would
make sense in any scenario ;-)

Just for future reference, when you have ideas/issues that might
have possible security ramifications, I'd prefer to see it first
discussed on a private list we created for that exact purpose, until
we can assess the impact (if any).  Right now MaintNotes says this:

    If you think you found a security-sensitive issue and want to disclose
    it to us without announcing it to wider public, please contact us at
    our security mailing list <git-security@googlegroups.com>.  This is
    a closed list that is limited to people who need to know early about
    vulnerabilities, including:

      - people triaging and fixing reported vulnerabilities
      - people operating major git hosting sites with many users
      - people packaging and distributing git to large numbers of people

    where these issues are discussed without risk of the information
    leaking out before we're ready to make public announcements.

We may want to tweak the description from "disclose it to us" to
"have a discussion on it with us" (the former makes it sound as if
the topic has to be a definite problem, the latter can include an
idle speculation that may not be realistic attack vector).

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Matt McCutchen]

On Sun, 2016-10-30 at 01:03 -0700, Junio C Hamano wrote:
> Matt McCutchen <matt@mattmccutchen.net> writes:
> 
> > 
> > On Fri, 2016-10-28 at 22:31 -0700, Junio C Hamano wrote:
> > > 
> > > Not sending to the list, where mails from Gmail/phone is known to
> > > get
> > > rejected.
> > 
> > [I guess I can go ahead and quote this to the list.]
> > 
> > > 
> > > No. I'm saying that the scenario you gave is bad and people
> > > should be
> > > taught not to connect to untrustworthy sites.
> > 
> > To clarify, are you saying:
> > 
> > (1) don't connect to an untrusted server ever (e.g., we don't
> > promise
> > that the server can't execute arbitrary code on the client), or
> > 
> > (2) don't connect to an untrusted server if the client repository
> > has
> > data that needs to be kept secret from the server?
> 
> You sneaked "arbitrary code execution" into the discussion but I do
> not know where it came from.  In any case, "don't pull from or push
> to untrustworthy place" would be a common sense advice that would
> make sense in any scenario ;-)

A blanket statement like that without explanation is not very helpful
to users who do find themselves needing to pull from or push to a
server they don't absolutely trust.  The only "definitely safe" option
it leaves them is to run the entire thing in a sandbox.  A statement of
the nature of the risk is much more helpful: users can determine that
they don't care about the risk, or if it does, what the easiest
workaround is.

The new risk we discovered in this thread is of leakage of private data
from the local repository.  To avoid that risk, it's sufficient for
users to move private data to a separate repository, so that's the
advice I propose to give.  Are you aware of issues with fetch/push with
potential impact beyond leakage of private data, which would make my
proposed text insufficient?  I was giving "arbitrary code execution" as
an example of what the impact of such an issue could be.

> Just for future reference, when you have ideas/issues that might
> have possible security ramifications, I'd prefer to see it first
> discussed on a private list we created for that exact purpose, until
> we can assess the impact (if any).  Right now MaintNotes says this:
> 
>     If you think you found a security-sensitive issue and want to
> disclose
>     it to us without announcing it to wider public, please contact us
> at
>     our security mailing list <git-security@googlegroups.com>.  This
> is
>     a closed list that is limited to people who need to know early
> about
>     vulnerabilities, including:
> 
>       - people triaging and fixing reported vulnerabilities
>       - people operating major git hosting sites with many users
>       - people packaging and distributing git to large numbers of
> people
> 
>     where these issues are discussed without risk of the information
>     leaking out before we're ready to make public announcements.
> 
> We may want to tweak the description from "disclose it to us" to
> "have a discussion on it with us" (the former makes it sound as if
> the topic has to be a definite problem, the latter can include an
> idle speculation that may not be realistic attack vector).

OK.  I'll admit that I didn't even look for a policy on reporting of
security issues because I believed the issue had low enough impact that
a report to a dedicated security contact point would be unwelcome.
 Maybe that was reckless.  The new text sounds good, if you put it in a
place where people like me would see it. :/

Matt

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Jon Loeliger]

So, like, Junio C Hamano said:
> Matt McCutchen <matt@mattmccutchen.net> writes:
> 
> > Then the server generates a commit X3 that lists Y2 as a parent, even
> > though it doesn't have Y2, and advances 'x' to X3.  The victim fetches
> > 'x':
> >
> >            victim                  server
> >
> >              Y1---Y2----                      (Y2)
> >             /           \                         \ 
> >     ---O---O---X1---X2---X3   ---O---O---X1---X2---X3
> >
> > Then the server rolls back 'x' to X2:
> >
> >            victim                  server
> >
> >              Y1---Y2----
> >             /           \
> >     ---O---O---X1---X2---X3   ---O---O---X1---X2
> 
> Ah, I see.  My immediate reaction is that you can do worse things in
> the reverse direction compared to this, but your scenario does sound
> bad already.

Is there an existing protocol provision, or an extension to
the protocol that would allow a distrustful client to say to
the server, "Really, you have Y2?  Prove it."  And expect the
server to respond with a SHA1 sequence back to a common SHA
(in this case the left-most O).  If so, a user could designate
some branch (Y) as "sensitive".  Or, a whole repo could be
so designated and the client then effectivey treats the server
as a semi-hostile witness.

Dunno.

jdl

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Junio C Hamano]

Jon Loeliger <jdl@jdl.com> writes:

> Is there an existing protocol provision, or an extension to
> the protocol that would allow a distrustful client to say to
> the server, "Really, you have Y2?  Prove it."

There is not, but I do not think it would be an effective solution.

The issue is not the lack of protocol support, but how to determine
that the other side needs such a proof for Y2 but not for other
commits.  How does your side know what makes Y2 special and why does
yout side think they should not have Y2?

Once you know how to determine Y2 is special, that knowledge can be
used to abort the "push" before even starting.  When you are pushing
back the 'master' and that 'master' reaches Y2, which must be kept
secret, you shouldn't be pushing that 'master' to them, whether they
claim to have Y2 or not.

I think the above is just a different way to say what Peff just said
(paraphrasing, do not push what is secret).

Re: Fetch/push lets a malicious server steal the targets of "have" lines

[Matt McCutchen]

On Sun, 2016-10-30 at 01:16 -0700, Junio C Hamano wrote:
> Jon Loeliger <jdl@jdl.com> writes:
> 
> > 
> > Is there an existing protocol provision, or an extension to
> > the protocol that would allow a distrustful client to say to
> > the server, "Really, you have Y2?  Prove it."
> 
> There is not, but I do not think it would be an effective solution.
> 
> The issue is not the lack of protocol support, but how to determine
> that the other side needs such a proof for Y2 but not for other
> commits.  How does your side know what makes Y2 special and why does
> yout side think they should not have Y2?
> 
> Once you know how to determine Y2 is special, that knowledge can be
> used to abort the "push" before even starting.  When you are pushing
> back the 'master' and that 'master' reaches Y2, which must be kept
> secret, you shouldn't be pushing that 'master' to them, whether they
> claim to have Y2 or not.

FWIW, I can imagine a protocol that would prove possession for all
objects, which would completely fix the problem.  Each object would
have a "private" hash computed recursively over the object graph, just
like the ordinary object hash, but with a different seed.  The object
database would be extended to cache the private hash of every object.
 Then, during a fetch or push, when the two sides identify a matching
object, the side that would otherwise have had to send the object sends
the private hash.  Support for storing multiple hashes per object might
also be useful in some way for the migration to a stronger hash
function than SHA-1.

The next best solution, which doesn't require a protocol change but
requires a little user intervention, is to have a configuration option
per remote for a set of refs whose reachable objects are known to be
safe to send to the server.  This set presumably includes the remote's
own remote-tracking refs.  During fetch and push, the client looks for
matches only among these "safe" objects, effectively emulating a
repository containing only the safe objects.  A fetch may update the
remote-tracking refs to point to unsafe objects that were already in
the local repository, effectively making them safe, but only after the
server sends the content of these objects (and the client validates the
hashes!).

Unfortunately, I'm not signing up to implement either solution. :(

Matt