Re: Fetch/push lets a malicious server steal the targets of "have" lines
On Sun, 2016-10-30 at 01:16 -0700, Junio C Hamano wrote: > Jon Loeligerwrites: > > > > > 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
On Sun, 2016-10-30 at 01:03 -0700, Junio C Hamano wrote: > Matt McCutchenwrites: > > > > > 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
Jon Loeligerwrites: > 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 McCutchenwrites: > 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
Jeff Kingwrites: > ... 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
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
So, like, Junio C Hamano said: > Matt McCutchenwrites: > > > 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
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
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
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
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
Matt McCutchenwrites: > 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
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
Matt McCutchenwrites: > 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?