feat: analyze expressions for safety

[woodruffw]

WIP, needs tests.

This will improve template-injection's precision by allowing it to filter expressions that are "safe," i.e. can't produce arbitrary input-derived code regardless of how they're executed.

For now, as "safe" expression is defined as:

1. Any primitive (null, number, string, etc.)
2. !{expr}
3. {expr} == {expr} and {expr} != {expr}
4. Any other binary expression where both the LHS and RHS are themselves safe per cases 1-3

Notably, this means that some expressions that contain code or can be evaluated to code will be considered safe, since that code will effectively be static within those expressions and not derived from user input (i.e. context accesses).

[carlocab]

For now, as "safe" expression is defined as:

1. Any primitive (null, number, string, etc.)

Wait, aren't strings unsafe? Or am I misunderstanding what you mean by a primitive string here?

[woodruffw]

Wait, aren't strings unsafe? Or am I misunderstanding what you mean by a primitive string here?

I mean something like this:

${{ 'foo' }}

That will expand to foo in whatever context it appears in, which means it might be code. But it isn't attacker injected code, since it's a literal string defined in the workflow itself and not in a context.

In other words, this PR removes positives like the following:

some-command ${{ some.condition && '--some-arg' || '' }}

...since some.condition influences the expansion, but the expansion itself is defined entirely by non-attacker controlled strings.

I could be convinced that this is a bad idea however, and instead consider string literals unsafe 🙂

[carlocab]

I could be convinced that this is a bad idea however, and instead consider string literals unsafe 🙂

TBH I think erring on the side of false positives to minimize the possibility of false negatives at this stage is the better move. If you're certain that this doesn't increase the likelihood of false negatives, then it should be fine -- but I'd suspect one would want a large corpus of test data available to ensure this first.

[woodruffw]

If you're certain that this doesn't increase the likelihood of false negatives, then it should be fine -- but I'd suspect one would want a large corpus of test data available to ensure this first.

I think it'll be fine, but yeah, I'll be adding a lot of tests to support this.

(There's a fine line here, but template-injection is specifically intended to catch injections of attacker-controlled input, not all possible ways an attacker can influence an expression. The latter is potentially a good separate audit to have.)