Page MenuHomePhabricator

Avoid setting EXEC bits on variables
Closed, ResolvedPublic

Description

So this is marginally useful but makes it quite harder to reason about and refactor a couple of methods. If we have something like

// In the global scope
function doEcho( $a ) {
   echo $a;
}
$x = 'safe';
doEcho( $x );
$x = $_GET['foo'];

the plugin will backpropagate the echo from the function, and it's also supposed [1] to set an EXEC bit on the global variable. This is so that when the unsafe assignment is found, it already knows that the variable can be output, and will report the assignment with

Assigning a tainted value to a variable that later does something unsafe with it

Of course this doesn't make sense in the code above, which is oversimplified. Instead, it's meant to catch things like:

function doEcho( $a ) {
   echo $a;
}

$x = 'safe';

function f1() {
   global $x;
   doEcho( $x );
}

function f2() {
   global $x;
   $x = $_GET['foo'];
}

where we don't know which function is going to be called first. Similar examples would also work for class properties instead of global variables. Setting EXEC bits on variables like this is currently the only way for us to report that something evil might be going on, but it has several disadvantages:

  • It's imprecise, as the first example shows.
  • It might be confusing, e.g. if f1 and f2 from above are in different files.
  • As I wrote in the introduction, it makes it harder to refactor the backpropagation logic
  • It might be affecting performance, since for every assignment we check whether the LHS has exec bits; which maybe is not too expensive, but it happens for all assignment and currently has no effect due to the bug at [1].

My proposal is that this logic is removed altogether. The replacement would be running phan (and taint-check) with --analyze-twice (T269816), that will catch the issue above [2]. This would also bring additional performance improvements, see T203651#7031088.


[1] - It currently doesn't due to an incomplete check, see r598486, although fixing it would basically only create more false positives.
[2] - Actually, not really, at least for global variables, because it currently overrides taintedness if the write happens in the global scope. This is easy to resolve though, we can either check for $variableObj instanceof Variable && $this->context->isInGlobalScope() in setTaintedness, or check up whether this can be improved upstream, e.g. wrapping globals in a GlobalVariable in the global scope.

Event Timeline

Change 683699 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/tools/phan/SecurityCheckPlugin@master] [WIP] Stop setting EXEC flags on variables

https://gerrit.wikimedia.org/r/683699

Change 683699 merged by jenkins-bot:

[mediawiki/tools/phan/SecurityCheckPlugin@master] Drop hacky/expensive code and recommend --analyze-twice instead.

https://gerrit.wikimedia.org/r/683699