github / privileged-requester

Privileged Requester Action
MIT License
16 stars 1 forks source link

Impersonation issue #166

Closed nobe4 closed 2 weeks ago

nobe4 commented 1 month ago

The current checks uses author name to ensure that the changes were made by the expected user.

https://github.com/github/privileged-requester/blob/fdd9f579ce88b2494b874bb381b966777473c3c3/src/runner.js#L16-L23

It is however really easy to make a commit with a different name, or email address.

git -c user.name='Linus Torvalds' -c user.email='torvalds@linux-foundation.org' commit -m "Wow, this is definitely legit."

This poses a serious risk of impersonation.

nobe4 commented 1 month ago

The GitHub API, provide commit verification information that is parsed in https://github.com/github/privileged-requester/blob/fdd9f579ce88b2494b874bb381b966777473c3c3/src/runner.js#L16

Format:

      "verification": {
        "verified": false,
        "reason": "unsigned",
        "signature": null,
        "payload": null
      }

E.g. gh api /repos/X/Y/pulls/Z/commits | jq '.[].commit.verification':

{
  "payload": null,
  "reason": "unsigned",
  "signature": null,
  "verified": false
},
{
  "payload": "tree xxx\nparent yyy\nparent zzz\nauthor nobe4 <nobe4@users.noreply.github.com>...",
  "reason": "valid",
  "signature": "-----BEGIN PGP SIGNATURE-----\n\nwsFcBA...n-----END PGP SIGNATURE-----\n",
  "verified": true
}

It is possible to add:

    for (const [, commit] of Object.entries(this.pullRequest.listCommits())) {
      let commitAuthor = commit.author.login.toLowerCase();

+     if (!commit.verification.verified) {
+       core.warning("Unexpected unverified commit.");
+       return false.
+     }

      if (commitAuthor !== privileged_requester_username) {
        core.warning(
          `Unexpected commit author found by ${commitAuthor}! Commits should be authored by ${privileged_requester_username} I will not proceed with the privileged reviewer process.`,
        );
        return false;
      }
    }
GrantBirki commented 2 weeks ago

@nobe4 I'll work on implementing this 🚀