Page MenuHomePhabricator

Follow-up to T272123: check if the errors are valid
Closed, ResolvedPublic

Description

To fix T272123: Merging to MediaWiki/extensions/CentralAuth is blocked and unbreak tests on CentralAuth's MASTER branch, I suppressed two SecurityCheck-XSS errors. We should check to see if they were indeed false positives, or not. If not, the underlying code should be fixed.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 moved this task from Unsorted to Reports on the User-DannyS712 board.
DannyS712 added subscribers: Daimona, RhinosF1, Iniquity and 2 others.

(adding subscribers of T272123)

The issue in SpecialGlobalUserMerge is a false positive due to taint-check not being able to handle array_map, see T204911. If you want to avoid the suppression, you can change it to a foreach, or you can wait for that bug to be resolved.

As for the issue in SpecialMultiLock, that's a false positive caused by MapCacheLRU. taint-check doesn't track taintedness for each class instance separately, since that would be very complicated, and it also assumes that nobody is going to use a class prop for values with different levels of escaping. This is usually a good assumption (and the code is probably faulty if the assumption doesn't hold true), but that is not the case for general purpose "store classes". MapCacheLRU::$cache is really a general purpose store, and it's perfectly fine to use different instances with different values. Apparently, somebody is using it to store a tainted value, and this has an effect on every other piece of code using MapCacheLRU. My suggestion here is to just mark MapCacheLRU::get as returning an untainted value. It might have false negatives, but they'd be greatly outbalanced by the false positives it already has.

I'm also investigating why no caused-by lines were reported, and also another couple of bugs with caused-by lines that I've observed while investigating, but these aren't in scope here.

Change 656428 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Annotate MapCacheLRU::get as returning a safe value

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

@Daimona Do you know how to make sec-check work "as expected" when for getWithSetCallback() such that Phan interprets the return from getWithSet as being the return of the closure, so it is naturally tainted when it should be tained, and safe when safe. Basically telling Phan to, for the purposs of static analysis, ignore the cache-hit scenario.

@Daimona Do you know how to make sec-check work "as expected" when for getWithSetCallback() such that Phan interprets the return from getWithSet as being the return of the closure, so it is naturally tainted when it should be tained, and safe when safe. Basically telling Phan to, for the purposs of static analysis, ignore the cache-hit scenario.

Support for closures is still kinda limited in taint-check. It is already able to infer when a closure is being called in simple code, e.g.

function execf( $cb ) {
   return $cb();
}
$cb = function() {
   return $_GET;
};
echo execf($cb); // <-- XSS here

but I'm unsure if getWithSetCallback is easy enough [1]. This is going to improve eventually anyway.

Note however that for MapCacheLRU the problem is different. Calling set() stores data in a class property, and taint-check doesn't store taintedness for different class props separately. That is:

function doSomething() {
   $map = new MapCacheLRU();
   $map->set( 'some-key', $_GET['tainted'] );
}

// In another file:
function doSomethingUnrelated() {
   $map = new MapCacheLRU();
   echo $map->get( 'some-other-key' ); // XSS
}

the set() call sets taintedness on MapCacheLRU::$cache which is then read when processing the get() call [2]. I expect this to remain a limitation "forever".


[1] - The main metric is: starting from getWithSet, how many call stacks are necessary to find the callback execution? If e.g. getWithSet calls func1, which calls func2, etc. until func5 and func5 executes the callback, taint-check is probably not going to see it. OTOH, if the callback is executed directly in getWithSet, it's probably going to understand it.
[2] - Note, this simple example wouldn't show an XSS because both keys are literals and they're different. In real-world code keys aren't usually literals so taint-check can't easily tell if two keys are the same.

Change 656428 merged by jenkins-bot:
[mediawiki/core@master] Annotate MapCacheLRU::get as returning a safe value

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

Change 656478 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/CentralAuth@master] Revert "Fix phan errors?"

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

Change 656478 merged by jenkins-bot:
[mediawiki/extensions/CentralAuth@master] Revert "Fix phan errors?"

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

DannyS712 assigned this task to Daimona.
DannyS712 removed a project: Patch-For-Review.

So the errors were false positives due to handling of MapCacheLRU::get, which is now annotated as returning safe, and the phan suppressions were removed.