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  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

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  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 .  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

[Junio C Hamano]

Jon Loeliger  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

[Junio C Hamano]

Matt McCutchen  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 .  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

[Junio C Hamano]

Jeff King  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.

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

[Jon Loeliger]

So, like, Junio C Hamano said:
> Matt McCutchen  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

[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

[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

[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 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

[Junio C Hamano]

Matt McCutchen  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 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:
> 
>    victimserver
> 
>  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:
> 
> 
>    victimserver
> 
>  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  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:

   victimserver

 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:


   victimserver

 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?

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