Page MenuHomePhabricator

Expand resolveOffset to account for multiple values
Open, Needs TriagePublic

Description

Currently, when setting and getting taintedness at a given offset, the plugin uses ContextNode::getEquivalentPHPScalarValue to determine which offset it is. This works well when the offset is constant or can be determined statically with 100% accuracy, but just as long as it can only have a single value. Otherwise, it just adds the taintedness to the "unknown keys" pile. This will fail when a value can be determined statically, but it's not a single value. For instance:

$arr = [];
$key = $GLOBALS['unknown'];
if ( $key === 'foo' || $key === 'bar' ) {
    $arr[$key] = $_GET['tainted'];
}

currently, this has YES_TAINT in unknownKeys, and no taintedness for specific elements. Instead, it should add YES_TAINT to both 'foo' and 'bar'.

Phan can tell us if the possible types are known (for the example above, inside the if, the real type is 'foo'|'bar'). However, what makes it non-trivial is that now, not every possibility is guaranteed to happen. In fact, in the example above, either 'foo' or 'bar' will be written (or none). ContextMergeVisitor might be able to "fix" this for us, but attention should be paid to avoid inferring too much.

Event Timeline

Memo: UnionType::asScalarValues can be used for this (used by resolveOffset, passing $strict=true). ContextNode doesn't have a method that returns a list of possible values, and that would be implemented by essentially copying getEquivalentPHPValueForNode, but it should use UnionType::asScalarValues instead of asValueOrNullOrSelf.