Escape incoming data

[ppitonak]

Expected Behavior

Incoming data parsed by trigger binding should be escaped so that it's not possible to execute injection attack.

In my example below, I expect to see Hello Pavol && echo I was here on $(date +%F) and I can do whatever I want!

Actual Behavior

$ tkn tr logs --last -f
[greet] Hello Pavol
[greet] I was here on 2020-07-10 and I can do whatever I want!

Steps to Reproduce the Problem

1. deploy the following reproducer

   ---
   apiVersion: tekton.dev/v1beta1
   kind: Task
   metadata:
   name: hello 
   spec:
   params:
   - name: NAME
     default: World 
   steps:
   - name: greet
     image: fedora:32 
     script: |
       #!/bin/bash
       echo Hello $(params.NAME)!
   ---
   apiVersion: triggers.tekton.dev/v1alpha1
   kind: TriggerBinding
   metadata:
   name: hello 
   spec:
   params:
   - name: NAME 
   value: $(body.name)
   ---
   apiVersion: triggers.tekton.dev/v1alpha1
   kind: TriggerTemplate
   metadata:
   name: hello
   spec:
   params:
   - name: NAME
   resourcetemplates:
   - apiVersion: tekton.dev/v1beta1
   kind: TaskRun
   metadata:
     generateName: hello-tr-
   spec:
     taskRef:
       name: hello
     params:
     - name: NAME
       value: $(params.NAME)
   ---
   apiVersion: triggers.tekton.dev/v1alpha1
   kind: EventListener
   metadata:
   name: hello
   spec:
   serviceAccountName: pipeline
   triggers:
   - bindings:
   - name: hello
   template:
     name: hello

2. expose the event listerner, e.g. oc expose svc el-hello
3. make a request... `curl -X POST -i 'http://el-hello.mycluster.com/' --data '{"name":"Pavol && echo I was here on $(date +%F) and I can do whatever I want"}'

Additional Info

There is basically the same problem when not using triggers at all but only plain taskrun/pipelinerun but IMHO in that case the risk of misuse/attack is quite limited.

[dibyom]

Thanks for the detailed report. Like you already mentioned, technically the issue is with Pipelines though using Triggers increases the risk.

One idea would be to escape anything in $() from event bodies. Another would be for Pipelines to add some kind of escaping? On the other end of the spectrum, we could keep the behavior as is, and warn users about such potential misuse. Thoughts @gabemontero @vdemeester @wlynch ?

[gabemontero]

Minimally something to definitely highlight up top in the documentation, in both pipeline and trigger projects. And yes, as @dibyom noted here and as myself and others have noted in WG calls or other issues, security around event injection into triggers could use bolstering. We've done a few things in the space, but more is desirable.

Some thoughts from a Pod security perspective (when it is enabled) ... i.e. best practices to employ with triggers and pipelines:

• Restrictions on which user ID the container runs under will prevent "harmful" commands from succeeding
• as would Restrictions on the pod privilege and escalate to privileged settings
• as would Restrictions of volume mounting (i.e. access to the host file system), host network, host namespace, will also insulate what the command you can invoke can do

Some thoughts from an authentication perspective

• https://github.com/tektoncd/triggers/issues/610 would help serve as protection against this
• especially in the context of triggers, for as @dibyom noted, triggers widens the exposure

Some thoughts from a "granularity" perspective

• the existing trigger level SA I did a while back,
• as well as the multi tenant EL, triggers as CRDs, etc.
• will allow for isolation of places where we might want current behavior

On that last point, presumably, there is/are scenario(s) where being able to take advantage of bash substitution exists.

I'll defer to those who have been on the tekton project(s) longer than I to confirm/deny.

But if so, any escaping at the pipeline or trigger level would need some sort of on/off control, where administrators can then choose to employ isolation/security measures like the ones noted above as a way to mitigate these valid security concerns.

[dibyom]

I'll defer to those who have been on the tekton project(s) longer than I to confirm/deny.

But if so, any escaping at the pipeline or trigger level would need some sort of on/off control, where administrators can then choose to employ isolation/security measures like the ones noted above as a way to mitigate these valid security concerns.

Yeah while escaping might be the correct behavior from a security standpoint, I think we do have use cases we'd want this behavior. So, I'm not sure if want to escape always either. /cc @sbwsg @bobcatfish

[bigkevmcd]

I've implemented a couple of different approaches here https://github.com/tektoncd/pipeline/compare/master...bigkevmcd:escape-strings-in-script

One would escape everything in the Script element of the Task, the other would would require more explicit (using #()) escaping.

[bigkevmcd]

I'll summarise the options as I see them:

On the triggers side:

• Manual escaping (add a CEL function, perhaps shellEscape() on strings)
• Automated escaping of data in TemplateBindings?

On the pipeline side:

• Explicit manual escaping perhaps something like #(params.name) instead of $(params.name)
• Automatic escaping of all params in here https://github.com/tektoncd/pipeline/blob/master/pkg/substitution/substitution.go#L120
• Only automatic escaping of params inside Scripts, perhaps by calling something like ApplyEscapedReplacements in https://github.com/tektoncd/pipeline/blob/master/pkg/apis/pipeline/v1beta1/step_replacements.go#L23
• Automatic escaping of params inside Script, but with optional "don't escape this param" with $() but #() meaning "don't escape this parameter.

There are probably some other options I've not thought of.

[wlynch]

@bigkevmcd SGTM.

My 2c is we should lean towards being more secure by default here and only allow unescaping if opted in if possible. My expectation is that most users (myself included) would use the $() behavior by default, and only be burned when the input changes to something unexpected. That said, we should weigh this against the difficulty of dealing with escaping (e.g. perhaps this is something we can just document).

Given a binding:

- name: foo
  value: ";${}"

and a template:

params:
- name: foo
  value: $(tt.params.foo)

If the resulting request is:

params:
- name: foo
  value: ';${}'

This should be relatively similar behavior to triggers today, and should protect against any direct script injections. It would then be up to Pipelines to make sure that these params are handled properly.

Consequences I can think of:

1. You would no longer be able to do things like:

   params:
   - $(tt.params.foo): bar
   value: baz

   I'm not sure if that's actually something we want to support anyway. (I remember talking about this in a triggers issue before, but I can't seem to find it - will update if/when I find it).

2. This could break existing template replacements that require a non-string value (not sure how common this is though).

[tekton-robot]

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity. If this issue is safe to close now please do so with /close.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[tekton-robot]

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

[tekton-robot]

@tekton-robot: Closing this issue.

In response to [this](https://github.com/tektoncd/triggers/issues/675#issuecomment-674376241): >Rotten issues close after 30d of inactivity. >Reopen the issue with `/reopen`. >Mark the issue as fresh with `/remove-lifecycle rotten`. > >/close > >Send feedback to [tektoncd/plumbing](https://github.com/tektoncd/plumbing). Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[bigkevmcd]

Let me see if I can clarify what we think should happen from this.

The values of all params being output in tektoncd/pipeline should be escaped to be shell-safe?

[dibyom]

There is now a TEP that proposes adding a shell escaped version of all pipeline params: https://github.com/tektoncd/community/pull/208

@ppitonak @bigkevmcd do you think adding escaped version for pipeline params is sufficient or is there more we need to do on the Triggers side?

[bigkevmcd]

@dibyom I think that would be the best solution :+1:

[tekton-robot]

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale with a justification. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[tekton-robot]

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten with a justification. Rotten issues close after an additional 30d of inactivity. If this issue is safe to close now please do so with /close with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[tekton-robot]

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen with a justification. Mark the issue as fresh with /remove-lifecycle rotten with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

[tekton-robot]

@tekton-robot: Closing this issue.

In response to [this](https://github.com/tektoncd/triggers/issues/675#issuecomment-834217294): >Rotten issues close after 30d of inactivity. >Reopen the issue with `/reopen` with a justification. >Mark the issue as fresh with `/remove-lifecycle rotten` with a justification. >If this issue should be exempted, mark the issue as frozen with `/lifecycle frozen` with a justification. > >/close > >Send feedback to [tektoncd/plumbing](https://github.com/tektoncd/plumbing). Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[defuse]

/open This is a remote code execution vulnerability in Tekton. Has it been patched, and if so which versions are affected or not affected? What are steps that a Tekton user or developer can take to tell if they are vulnerable?

[defuse]

For anyone following along, I think I figured it out.

The problem is that when you use $(...) in a Tekton yaml, especially within a script:, the $(variable) just gets replaced with the contents of variable, and no escaping is done. Tekton actually can't escape it properly, because it doesn't know if the script: is bash or Python or some other kind of language, so the onus is on the yaml file writer to do the escaping, or use environment variables such as in https://github.com/tektoncd/pipeline/issues/3458. You can search for instances of this using a command like:

ack --color -i -A100 -t yaml 'script:'  | grep --color -E '^|\$\('

If any of the variables interpolated this way can be attacker-controlled, then the attacker can inject code into your script: (in whatever language the script is). AFAICT the only safe way to get an attacker-controlled variable into a script is to use the environment variable trick I linked to above.

[dibyom]

@defuse you are right, we are working on this in TEP-0099: https://github.com/tektoncd/community/pull/596