8

On github, any person (let's call them Alice) can fork a project, make modifications to it, and then submit those changes to the project owner(Bob) as a pull request, and the idea is that Bob reviews the submitted code to make sure it is up to spec, and contains no viruses*.

If Bob wants changes made before accepting the pull request (such as code style), they can make comments asking Alice to make those changes, and Alice will most likely make those changes, and commit them to Alice's fork, at which point the commits automatically appear as part of the pull request.

Now, what if there is a malicious person (Malice) that creates a fork with beneficial changes and submits a pull request, and Malice knows exactly when Bob is going to look at that pull request, and that Bob is going to take approximately 5 minutes to look over the new code. Would it then be possible for Malice to make a malicious commit in between Bob requesting the page to view the pull request and Bob pressing the "Accept" button?

*Ideally, this is what happens. We assume Bob is both benevolent and intelligent, and does this properly to his best ability, but is unaware of this particular possible vulnerability.

  • Ummm yes. But Bob is both benevolent and intelligent so clearly he checked that the file hash had not changed before Accepting a pull request. – AstroDan Mar 1 '16 at 18:46
  • @AstroDan How does he do that? Refresh the page? If he does, there's still a delay inbetween seeing the pull request and accepting it. Perhaps in that case the delay is small enough that malice would have to be able to view Bob's network traffic to know when he refreshed the page (https helps here, but still doesn't completely eliminate the possibility), or Malice would have to get lucky. I don't like luck-based security :( – Shelvacu Mar 1 '16 at 18:51
  • If my (very rusty) remembrance of how git works is still correct the commit is accepted via the hash of the input. When you accept you are saying I accept the commit of hash n. if n does not match the current hash the it will fail. – AstroDan Mar 1 '16 at 18:54
  • 2
    @AstroDan I'm talking about Github, not git. I doubt anything similar exists in git. But it is possible that the request sent to github for a pull request includes a telling of what commits are currently being shown on a page, in which case github would not be vulnerable. I will test this when I get home if there are no answers by then. – Shelvacu Mar 1 '16 at 18:56
  • I don't think github will automatically take those changes. You have to update your pull request – d1str0 Mar 1 '16 at 19:51
2

No, Github is not vulnerable.

I have tested this with a very helpful friend of mine to make things go quicker. When this was attempted, github updated the list of commits live, and if I clicked the "Accept" button before the list was updated, github throws up this message:

MERGE ATTEMPT FAILED: Head branch was modified. Review and try the merge again

  • 2
    As I said in the comments of my answer, this test does not prove that there is no race condition. This test shows that GitHub makes a test before merging the pull request. But what if, Malice's commit is received in the mean time? The delay might be very very short, but this is a race condition. And you said you don't like lucky-based security. – Antoine Pinsard Mar 2 '16 at 5:56
0

Pull requests are not specific to GitHub. It is part of git.

Accepting a pull request is just merging a revision into master. So I would strongly hope that if you tell GitHub to execute git merge deadbeef, he would actually execute git merge deadbeef and not git merge HEAD.

  • 2
    You hope? I can't accept a hope. I couldn't even accept a hope and a prayer. – Shelvacu Mar 1 '16 at 21:43
  • Maybe "I trust" would be more accurate. I don't have access to GitHub 's source code so I can't do much more. – Antoine Pinsard Mar 1 '16 at 21:46
  • 1
    If you can't accept a hope, I'm afraid you should directly ask GitHub. I don't think anyone here has access to the source code, as it is proprietary. – Antoine Pinsard Mar 1 '16 at 22:03
  • 1
    One could test it by being both Malice and Bob, which I plan to do once I get home. – Shelvacu Mar 1 '16 at 23:19
  • Not sure the problem @shelvacu is really about the 'pull requests', which as you said aren't specific to github. Rather the he seems interested in the how exactly the 'web based merge feature' of github' behaves. Anyway, it sure seems like it would be pretty easy to build a test. Create, a spare account, fork your a test project on a main account, send the PR, then make changes. See if the PR is linked to a specific commit, or just a specific branch. If it is linked to a specific commit, it doesn't seem like it should be possible to change and have the same commit hash. – Zoredache Mar 2 '16 at 0:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.