(Un)signed commits: how we found a (non) security bug in GitHub
When you build an entire software around someone else API, you tend to know everything about it. We made Mergify on top of GitHub API, and it's hard to describe how well we understand its API. From its fabulous and beloved features to its most horrible defects, we've been experiencing most of it.
We thought we knew it all until we discovered a very intriguing… behavior.
It's all about Branch Protections
Over the last years, the usage of the branch protection feature grew broadly among GitHub users. Almost every organization out there uses it to prevent people from clicking too soon on the green merge button.
Obviously, our response to this requirement is to rely on Mergify, which provides a finer-grained, more robust and automatic approach to merge. Nevertheless, Mergify works on repositories with branch protection enabled — and this is how we discovered this bug.
One of the branch protection features that an administrator can enable is Require signed commits. If you enable this checkbox, "contributors and bots can only push commits that have been signed and verified to the branch" as the documentation states.
What we discovered is that this protection does not work like many people think it does.
If you create a pull request targeting a protected branch where signed commits are required, GitHub protects the branch and disallows the pull request to be merged if it contains any unsigned commits. This protection can be testified by the error message you see near the merge button.
You'd think this pull request is therefore un-mergeable. The button is grey, disabled, and the big red cross indicates you can't do anything with this pull request until the author signs its commits.
Actually, it is mergeable, see for yourself:
Did you hack GitHub?
This is what we thought at the beginning, so we went away and reached GitHub via their security program. We provided a demo and a way to reproduce this issue, thinking that this could not be the intended behavior. Having a way to circumvent the protection advertised by the user interface seemed like a no-brainer to us.
After opening a security report on GitHub's security platform, they reached their product team, which took a few days, before getting back to us. They finally provided this answer:
The merge commit strategy is disallowed because it's attempting to merge in truly unsigned commits, while the squash and merge condenses the commits into a single commit signed with GitHub's key and is allowed. We don't currently treat GitHub-signed commits as somehow "less" verified than ones signed by a registered user-owned GPG key, but is something we may explore in the future, as well as ensuring this behavior is consistent across the Web and API.
That answer makes sense, technically. By squashing the unsigned commits together, GitHub transforms unsigned commits into a signed commit. Is that what repositories administrators expect? We still don't know for sure.
Anyhow, the inconsistency between the user interface and the API might lead users toward thinking they are protected against merging unsigned commits while they are not. The user interface provides a false sense of security.
GitHub branch protection feature name can somewhat be misleading in that sense. It is easy for a user to believe that every pull request must have signed commits, whereas it only means the branch will get signed commits in it, whatever their source is (properly signed commits or a squash merge done via the API).
That does not mean anyone could insert untrusted code in a repository. Authors of pull requests are still authenticated through GitHub, which makes sure the source can be trusted — signed commits or not. What this protection settings ensure is that every commit in your repository is signed, which makes the cloned code auditable in that regard.
If you want your pull request to contains signed commits before merging and avoid this loophole, there's a simple way to fix it: disable squash merge.
Unfortunately, disabling squash merges can only be done on an entire repository and not on a per-branch basis — contrary to branch protections. However, that might still be a viable approach for users concerned about commit signatures and authenticity and wanting more safety.