search

ndavison / circleci-logs

CircleCI log and security configuration automations
22 stars 4 forks source link

As many targets enforce checks upon performing forked pulled requests, isn't this method of forcing secrets unreliable? #3

Open bbsecretstester opened 4 years ago

bbsecretstester commented 4 years ago

I was curious to know your thoughts on many targets using checks such as contribution agreements, pull request reviews prior to accepting, etc. I've found that even if a CircleCI repo is in fact vulnerable for exfiltration, I have yet to find a vulnerable target allow any user to perform a pull request prior to the target reviewing the request or performing checks on it.

thoughts?

ndavison commented 4 years ago

Hi @bbsecretstester - yep these can be mitigations to an extent. Part of the script is it will try and detect whether the CircleCI build started closer to the merge time rather than PR creation time, which may indicate it is not exploitable (i.e. the job ran when it was merged so unlikely a project would do that for a malicious PR):

https://github.com/ndavison/circleci-logs/blob/master/circleci-vulnerable-config.py#L115-L130

I'd say that just having to sign a contribution agreement isn't necessarily a strong mitigation, as depending on the project that can be a trivial step before exploitation. I've also seen cases where this is not a requirement for the CircleCI build to kick off, just for the PR to be merged down the line. And further I've seen cases where a Github bot will kill that CircleCI job before it can run, and then re-run it later based on a reviewer's approval.

So I'd say a lot of scouting is needed in some cases. The key things to check (when it appears the job was given access to secrets) is whether the CircleCI job ran when the PR was created, and whether the CircleCI job was run by the same user who created the PR.

ndavison commented 4 years ago

I'll also add that the main reason I added the -v (verbose) and -a (check all builds) options to the script was to help investigate cases where the vulnerable status is complex. It could be a PR's latest CircleCI build was inline with the merge, but an earlier build associated with the PR did run on PR creation and also had secrets passed in. In this case more output and checking all builds (associated with the PRs the script collected) can help determine this. Also the -o (open PRs only) option is handy to try and eliminate the possibility the build occurred on merge (as it only checks still open PRs).