scorecard icon indicating copy to clipboard operation
scorecard copied to clipboard
verified publisher icon ossf

BUG: code review bypass via commits to local branches

Open laurentsimon opened this issue 3 years ago • 8 comments

Steps:

  1. Dependabot publishes a change.
  2. Maintainer edits that change to do whatever they want
  3. Maintainer approves it
  4. Maintainer merges it.

Let's try to verify that the approver != from all commits in the PR. In the config, branch protection is not enabled thru *, I think.

Is there a dependabot config option for dependabot to use a remote fork's branch instead of local branch on the project?

Thanks @loosebazooka for sharing!

laurentsimon avatar Mar 25 '22 16:03 laurentsimon

another bypass I found:

  1. Create a PR
  2. Click merge

The PR is considered reviewed.

Scorecard has this code https://github.com/ossf/scorecard/blob/main/checks/evaluation/code_review.go#L138 that accepts these because the committer is always "github". graphql has a field to know who clicked "auto-merge" this PR.

laurentsimon avatar Mar 25 '22 19:03 laurentsimon

Steps:

  1. Dependabot publishes a change.
  2. Maintainer edits that change to do whatever they want
  3. Maintainer approves it
  4. Maintainer merges it.

Let's try to verify that the approver != from all commits in the PR. In the config, branch protection is not enabled thru *, I think.

Is there a dependabot config option for dependabot to use a remote fork's branch instead of local branch on the project?

Thanks @loosebazooka for sharing!

For this, can we do something like - expand the PullRequest struct to also contain commits (today we only care about merge commit)? That should provide us the info to find these types of bypasses.

azeemshaikh38 avatar Mar 30 '22 15:03 azeemshaikh38

For this, can we do something like - expand the PullRequest struct to also contain commits (today we only care about merge commit)?

yep that's also what I had in mind. Seems doable; it may increase rate limiting usage, but we'll see.

laurentsimon avatar Mar 30 '22 16:03 laurentsimon

Yeah, good thing is we have tests that will tell us by how much the API usage will go up. Would be good to get this fixed soon. Do you want to take this on or keep this open for someone else?

azeemshaikh38 avatar Mar 30 '22 16:03 azeemshaikh38

If nobody else wants it I'll take it, but if someone is interested I'm happy for them to take it.

laurentsimon avatar Mar 30 '22 17:03 laurentsimon

If nobody else wants it I'll take it, but if someone is interested I'm happy for them to take it.

I can take this on to reduce some of your load.

azeemshaikh38 avatar Mar 30 '22 19:03 azeemshaikh38

SG, it's a fun (and important) one!

laurentsimon avatar Mar 30 '22 20:03 laurentsimon

Just an update - I'm working on this in 2 parts. (1) expose PR commit data through CodeReviewRawData struct. (2) figure out how to evaluate this data to find malicious activity while not causing false positives.

(1) is easy and I'm working on a PR for it. I'm still unsure about (2) and need to look into it more. Ideas are welcome.

azeemshaikh38 avatar May 23 '22 19:05 azeemshaikh38