Allow plugins to run for pull requests

[x1ddos]

As a starting use case: it is often convenient to "stage" a change introduced by a PR for manual inspection. For instance, a staging version of a website could be published at a unique URL, in addition to regular build process:

Currently, plugins are not being run, unconditionally. To make the behavior backward compatible, the when filter would default to when: { event: push } if none specified.

[gtaylor]

This may or may not be relevant to this particular discussion, but I really like how the Kubernetes team is handling this with their Jenkins setup:

Here is the issue I pulled this from. I really dig this as an extra option on top of whatever Drone core protections we might add.

[bradrydzewski]

I propose we should only execute plugins if the yaml is signed. Otherwise there are a couple possible attack vectors that could be exploited. Right now these vectors are prevented by excluding plugins from executing during pull requests.

First example might be the ssh or rsync plugin where a malicious pull requests modifies the yaml to execute the plugin step, but changes the commands to do something very bad, such as removing directories of even altering the authorized_keys file to give themselves access to the server:

deploy:
  ssh:
    addr: 1.2.3.4
-   commands: [ ./restart.sh ]
+   commands: [ sudo rm -rf /root/ ]
+   when: { event: pull_request }

Second example might be to try to exploit something like the git or pypi plugin. Let's say a malicious pull request add a post-commit hook to the .git directory (or alters the setup.py for pypi). The hook will run inside the plugin, and will be able to query the /proc filesystem to see what data the plugin was invoked with. The git plugin is invoked with a payload that includes your ssh private key, for example.

+publish:
+  git:
+    push: git@github.com:foo/bar.git

I believe signing the yaml is the best way to prevent these sort of attacks. If a developer chooses to enable (and sign the yaml) to execute the pypi step when testing a pull request they may make themselves vulnerable. So how do we feel about "giving people enough rope to hang themselves with" (as the phrase goes)?

cc @benschumacher

[x1ddos]

Run plugins in PRs only when yaml is signed sgtm.

[x1ddos]

Just a note: all the attack vectors @bradrydzewski mentions generally come from environments where an arbitrary command can be injected bypassing yaml signature verification.

@gtaylor k8s has a load of shell scripts executed during testing (which brings you to the previous paragraph). but yeah, it would be a nice another feature for drone, although much more complex than this.

[x1ddos]

What I meant by first paragraph in #1476 (comment) is @bradrydzewski's first example can be exploited even if a yaml is signed: it is sufficient to insert sudo rm -rf /root/ in ./restart.sh w/o modifying the yaml file.

Users just need to know this is dangerous. Maybe some plugins could be manually marked as safe as an additional measure.

[bradrydzewski]

... first example can be exploited even if a yaml is signed: it is sufficient to insert sudo rm -rf /root/ in ./restart.sh w/o modifying the yaml file.

yes, we need to warn people to use their better judgement, probably with gigantic warning messages in the documentation. Since people like to skip / skim the documentation they might miss the warnings. We could possibly add something to the yaml forcing them to acknowledge that it warrants additional consideration (something like unsafe: true or safe_mode: off?)

Users just need to know this is dangerous. Maybe some plugins could be manually marked as safe as an additional measure.

agreed, there are a few ideas we are kicking around such as the plugin requesting permissions to sensitive data. The pypi plugin could be exploited to expose the ssh key or netrc file, even though it doesn't need this sensitive data to do its job.

Plugins could request sensitive data (sort of like android apps) by storing that meta-data in the Dockerfile. Drone could inspect the image prior to starting the plugin:

LABEL permissions=PRIVATE_KEY,NETRC

This still isn't full proof since the git plugins (which would be subject to evil scripts in .git/hooks) still need the ssh key, however, we could harden the git plugins to remove the .git/hooks directory at runtime.

This of course still means that we need to identify all the possible exploits in each plugin requesting secrets, which is subject to human error.

[x1ddos]

Maybe a PLUGIN_SAFE could be introduced as a first iteration, similar to what PLUGIN_FILTER does, which would include only a subset of official plugins by default.

[benschumacher]

There was a comment made in drone-dev on the topic highlighted in #1476 (comment). One potential mechanism to address this potential attack is to (optionally) allow the signing mechanism used on .drone.yml to be extended to other artifacts.

I'm not proposing a syntax, but if a change to an artifacts such that it doesn't match an expected signature, then it could fail the build early. This could already be implemented by a user by building these tests directly into .drone.yml. As I type this, I could imagine writing this logic directly in a custom before plugin based on #1443. Obviously this still requires users to adopt these security tools, but that's always the case.

I'm a fan of opt-in for unsafe behaviors, so I like the idea of having a configuration callout for unsafe: true, but is the YAML the correct location for that?

[cloudnautique]

Noodling this a bit, there are varying degrees of trust already baked into Drone. With a "Trusted" repo setting you are already acknowledging some degree of "allow me to be vulnerable". With that setting a user can enable things like privileged mode on a container, which from a security stand point is game over.

In the context of a trusted build, with conditionals on plugins, you have some granularity on when to run things particularly with event types, tags and branches.

Would it be reasonable that if you have a signed yml, trusted build setting enabled, and conditionals for event: pull_request that it be honored.

I also really like the suggestion of @gtaylor based on whitelisting users. Or org members default to some behavior vs non. For public projects, this could go a long way into defining how/where secrets are allowed.

[bradrydzewski]

@cloudnautique I think as long as the yaml is signed and event: [ pull_request ] is explicitly configured for each plugin we should have enough assurance to proceed.

The attack vector we need to prevent is altering the yaml to run a deployment step that shouldn't otherwise be executed. If the yaml is signed this attack vector cannot be exploited.

The exception to the above rule are plugins that don't require secrets to execute such as the ssh and rsync plugin. This is because drone provides a per-repository key pair that is used automatically. This has me thinking that drone should stop generating this key pair, and these plugins should instead source the key pair from the .drone.sec file.

[sl1pm4t]

What was the outcome of this thread?
Is this functionality available in the current 0.4 builds? Or do we need to wait for 0.5 / 0.6 ?
Thanks

[bradrydzewski]

@sl1pm4t this is fixed in 0.5. It required breaking changes and therefore won't get backported to 0.4

[anthonycorletti]

Wanted to ask if this should be closed or not as this is the only issue holding the v0.5.0 milestone back. Stoked for the 0.5 official release 🎉5️⃣

[bradrydzewski]

closing since this is implemented in 0.5

there are some agent <> server communication issues blocking an official 0.5 tag, but since many people are already using 0.5 and taking advantage of this feature, I think we can close :)