9

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.

Improve this question
8
  • 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, 2016 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, 2016 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, 2016 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, 2016 at 18:56
  • I don't think github will automatically take those changes. You have to update your pull request
    – d1str0
    Mar 1, 2016 at 19:51

3 Answers 3

Reset to default
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

Improve this answer
1
  • 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, 2016 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.

Improve this answer
7
  • 2
    You hope? I can't accept a hope. I couldn't even accept a hope and a prayer.
    – Shelvacu
    Mar 1, 2016 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, 2016 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, 2016 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, 2016 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, 2016 at 0:18
0

The GitHub merge pull request API does not appear to be vulnerable. As documented here, the sha argument to the pull request will ensure that the pull request hasn't had new commits added beyond what the invoker of the merge expects:

SHA that pull request head must match to allow merge.

Improve this answer

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .