Protection against code injection attacks

[kislyuk]

By default, WDL is not hardened vs. string injection attacks in placeholder expression string interpolations in command blocks, etc. Given malicious input, this can lead to straightforward escape into the enclosing container and possibly beyond, breakage of multi-user isolation boundaries, etc. and so it must be assumed that all WDL inputs are fully trusted. This necessitates comprehensive input validation logic before inputs can be fed to the WDL itself. For usability in environments where the implementer must take reasonable care to defend against malicious input, it would be nice to have a string escape mechanism like ~{quote(var)} which would emit a double-quoted string with any quotes inside escaped.

[illusional]

Hi @kislyuk, there does exist an squote function introduces in WDL 1.1, but AFAIK it's not implemented in Cromwell: https://github.com/openwdl/wdl/blob/main/versions/1.1/SPEC.md#-arraystring-squotearrayp

[kislyuk]

Thanks for the suggestion. Can you give an example of how squote would be used for this task?

From what I can tell, neither quote nor squote are a good fit for the task of safely interpolating strings, because they only work on arrays and don't seem to work as expected in code block expression placeholder interpolations.

[mlin]

Yea per the original PR squote() is effectively defined as just suffix("'",prefix("'",elt)) for each element -- it doesn't go inside elt and escape anything that might be malicious in there. I mentioned then wishing for a secure shell-quoting function, but I'm not sure how to write the detailed specification of it.

[rhpvorderman]

I am not in favour of a shell hardening function. Shouldn't that behavior be by default? I'd rather have an unsafe() function in case the shell hardening is too strict. But by default it should be secure.

[kislyuk]

@rhpvorderman I agree! I think we should move to revise the definition of both quote() and squote() to escape internal quote characters (and also to work on scalars as well as arrays) in a future OpenWDL spec, and to add guidance for developers to use this updated squote() as the recommended safe interpolation technique in shell scripts.

[pshapiro4broad]

I think there are two different areas of concern regarding code injection in WDL. One is the case where a string is used by the runtime. For example, if the WDL runtime interprets a File string as a GCS path and generates a gsutil command to copy the file, an attacker could try and generate a malicious filename to break the runtime. The other case is in the WDL command block.

I would say that the first set of cases is in the domain of a WDL runtime implementor and may be mentioned in the spec as something to pay attention to, but not as a required part of a WDL implementation.

Regarding string interpolation inside a command block, I agree that WDL should provide help so WDL script writers don't have to worry about this problem. The only risk I see in changing string interpolation behavior is that a command block can contain other languages besides a shell script, such as using a heredoc to embed data that's not sh script code. Using an opt-out method like unsafe() could be used to handle that case though.

[mlin]

In chanzuckerberg/miniwdl#496 we have a prototype of evolving the current squote() function to make the string safe to interpolate into a task command script (which means single-quoting it, and escaping any single-quote marks therein; as in Python's shlex.quote).

However, I also have a prototype (#458) of automatically propagating WDL task values into environment variables. I think this might be a slightly better solution to this problem, as you then just have to handle the environment variables using standard bash practices (which admittedly has pitfalls of its own, but at least well-known ones).

[patmagee]

this came up in a product discussion I was having around security today. I would be a very strong advocate for including this behaviour in WDL ASAP

[geoffjentry]

Isn't WDL a code injection engine by design?

[kislyuk]

Hi Jeff - it is really important to us as users of WDL to be able to reason about what user-provided inputs are safe to pass to our WDL-based pipelines without knowing how to fully parse and interpret them. Imagine a production WDL pipeline where users (lab scientists, third-party scientists, people with access to a sample submission interface, etc.) are using a web UI to submit jobs and get results. To separate concerns, we want the web UI to be able to render the pipeline inputs without having to know how to interpret and sanitize each of them. Without string escaping, any form like this that doesn't completely scrub the inputs of interpretable characters is insecure. This necessitates overly specific front-end logic and reduces WDL's usability as a general purpose bioinformatics tool.

[kislyuk]

You can read more about the general class of attacks that make this behavior hazardous here in the OWASP top 10 guidelines: https://owasp.org/www-project-proactive-controls/v3/en/c4-encode-escape-data

[mlin]

Exapting squote() is a band-aid, but I don't think it helps enough at this point (you'd want to put it in almost every string interpolation). The secure quoting should be opt-out, as @rhpvorderman suggested above, or we should move to deprecating textual interpolations in favor of environment variables (#458). Both are WDL 2.x matters clearly.... 😕

[geoffjentry]

@kislyuk Yes, sorry - I was being cheeky. I'm familiar with injection :) My comment was that the command block gets run as-is by the engine. In a scenario where one is e.g. running arbitrary WDLs from a 3rd party, that can be problematic. I recognize that's now what you're after here.

[geoffjentry]

@kislyuk Hi yes, I understand the concern. I was being mostly facetious. My point was that a WDL script is running a shell script by the engine w/o question anyways - and at that point what are you gaining.

That said, I'm not at all opposed to putting up roadblocks.

[patmagee]

Hello all, I did want to come back to this thread and share that there is an open pr for introducing a way to define inputs as environment variables into the WDL specification. You can see and comment on the thread here: #504