JS: support rest-patterns inside property patterns

[erik-krogh]

To support the below case, where a rest-pattern is inside a property-pattern.

(function () {
	const {
		argv: {
			...args
		},
	} = require('yargs')
		.usage('Usage: foo bar')
		.command();

	cp.exec("cmd.sh " + args); // NOT OK
});

I feel that the change in DataFlow.qll shouldn't be needed.
I'm hoping I overlooked something, and that there's a better way of doing it.

Gets a TP/TN for CVE-2020-8149

[asgerf]

I can't immediately see why the rest pattern isn't working, but this isn't quite right. The rest pattern should be found by the recursions in lvalAux and getABindingVarRef.

Isn't it just a case of us not having a taint step through rest patterns?

[erik-krogh]

I can't immediately see why the rest pattern isn't working, but this isn't quite right. The rest pattern should be found by the recursions in lvalAux and getABindingVarRef.

Yes, it is recognized, but not with the correct def.

The rest-pattern in the example contains all the properties from the argv property, but according to lvalAux the def is the outer object, it should be the inner argv property-pattern.

[erik-krogh]

And the expected output is breaking a lot because of the change in DataFlow.qll, so I'm hoping there is a better way.

[asgerf]

def is the CFG node where the assignment to all variables on the LHS actually happen. It's modelled as one big bulk assignment, in order to model parallel assignments:

[x, y] = [y, x]

There's probably a simpler way to model that, but that's how DefUse.qll operates, and that's why def is inherited from the outer pattern.

[erik-krogh]

Thanks for the feedback, I found a good solution.

I just needed to add a new lvalueFlowStep.

[asgerf]

Let's just review the runtime behavior of rest patterns:

• { ...args } = x stores a shallow copy of x in args.
• { p1, p2, ...args } = x stores a shallow copy of x without properties p1, p2, etc in args.

What this change is doing is essentially modelling this copying step via a pure data flow step x -> args.

There are two issues here:

• How do we best express that step?
• Is it safe to use a pure data flow step for this?

For the first issue, I think the step should go from the object pattern itself to its rest pattern:

exists(ObjectPattern pattern |
  pred = lvalueNode(pattern) and
  succ = lvalueNode(pattern.getRest())
)

For the second issue, I can completely see how this would work great most of the time, but not always. For example, this would cause getALocalSource to backtrack through a copy step, making some of our store-steps spuriously propagate back to the original object.

I think the best would be to add it as an AdditionalFlowStep. Those steps don't affect getALocalSource but do affect all data-flow configurations. And since the above formulation doesn't depend on any internal predicates it should be a straightforward addition.

[erik-krogh]

Thanks.

The only data-flow related use of rest-patterns I could fine was in DefUse::lvalAux, I think that threw me off.

I've added it as a default taint-tracking step for now.
Do you think it should be a default data-flow step?

[asgerf]

Have you tried the version I proposed earlier?

Suggested change

	exists(PropertyPattern pattern |
	pred.(DataFlow::PropNode).getAstNode() = pattern and
	succ = DataFlow::lvalueNode(pattern.getValuePattern().(ObjectPattern).getRest())
	)
	exists(ObjectPattern pattern |
	pred = lvalueNode(pattern) and
	succ = lvalueNode(pattern.getRest())
	)

That should also work for rest patterns that aren't part of a property pattern, like:

let { ...args } = x
let [ { ...args} ] = x;

[erik-krogh]

I had not, I should have.

I've implemented your suggestion, but I used DestructuringPattern instead of ObjectPattern, that way we also support array-spreads.

[asgerf]

Excellent! 👍