Brian Wolff has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/393540 )
Change subject: Support ParserFunctions, and start of work for hooks in general
......................................................................
Support ParserFunctions, and start of work for hooks in general
Also fix bug sometimes function taint would get lost if
setting a different taint for another parameter
Add -v option to the test runner
Change-Id: Idcd74eee6ad5fb38d7c8b81f4cf1058dc26f080b
---
M .phpcs.xml
A src/MWPreVisitor.php
A src/MWVisitor.php
M src/MediaWikiSecurityCheckPlugin.php
M src/TaintednessBaseVisitor.php
M src/TaintednessVisitor.php
A tests/integration/callbackhook/Parser.php
A tests/integration/callbackhook/db.php
A tests/integration/callbackhook/expectedResults.txt
A tests/integration/callbackhook/func.php
A tests/integration/callbackhook/test.php
A tests/integration/stripitem/Parser.php
A tests/integration/stripitem/expectedResults.txt
A tests/integration/stripitem/test.php
M tests/runtests.sh
15 files changed, 833 insertions(+), 17 deletions(-)
diff --git a/.phpcs.xml b/.phpcs.xml
index 60518ae..8b2fe7e 100644
--- a/.phpcs.xml
+++ b/.phpcs.xml
@@ -4,6 +4,7 @@
<exclude
name="MediaWiki.NamingConventions.PrefixedGlobalFunctions.wfPrefix"/>
<exclude name="MediaWiki.Usage.ScalarTypeHintUsage.Found"/>
<exclude
name="MediaWiki.Commenting.FunctionComment.MissingParamComment"/>
+ <exclude
name="MediaWiki.WhiteSpace.SpaceBeforeSingleLineComment.NewLineComment"/>
</rule>
<file>.</file>
<arg name="bootstrap"
value="./vendor/mediawiki/mediawiki-codesniffer/utils/bootstrap-ci.php"/>
@@ -23,6 +24,12 @@
<rule
ref="MediaWiki.Commenting.FunctionComment.MissingDocumentationPublic">
<exclude-pattern>./tests</exclude-pattern>
</rule>
+ <rule
ref="MediaWiki.Commenting.FunctionComment.MissingDocumentationProtected">
+ <exclude-pattern>./tests</exclude-pattern>
+ </rule>
+ <rule
ref="MediaWiki.Commenting.FunctionComment.MissingDocumentationPrivate">
+ <exclude-pattern>./tests</exclude-pattern>
+ </rule>
<rule ref="MediaWiki.Files.ClassMatchesFilename.NotMatch">
<exclude-pattern>./tests</exclude-pattern>
</rule>
diff --git a/src/MWPreVisitor.php b/src/MWPreVisitor.php
new file mode 100644
index 0000000..ebe5042
--- /dev/null
+++ b/src/MWPreVisitor.php
@@ -0,0 +1,119 @@
+<?php
+
+use Phan\Language\Context;
+use ast\Node;
+use ast\Node\Decl;
+use Phan\Debug;
+use Phan\CodeBase;
+use Phan\Language\Element\FunctionInterface;
+
+/**
+ * Class for visiting any nodes we want to handle in pre-order.
+ *
+ */
+class MWPreVisitor extends TaintednessBaseVisitor {
+
+
+ /**
+ * Ensure type of plugin is instance of MediaWikiSecurityCheckPlugin
+ *
+ * @param CodeBase $code_base
+ * @param Context $context
+ * @param MediaWikiSecurityCheckPlugin
+ */
+ public function __construct(
+ CodeBase $code_base,
+ Context $context,
+ MediaWikiSecurityCheckPlugin $plugin
+ ) {
+ parent::__construct( $code_base, $context, $plugin );
+ // Ensure phan knows plugin is right type
+ $this->plugin = $plugin;
+ }
+
+ /**
+ * Handle any node not otherwise handled.
+ *
+ * Currently a no-op.
+ *
+ * @param Node $node
+ */
+ public function visit( Node $node ) {
+ }
+
+ /**
+ * @see visitMethod
+ * @param Decl $node
+ * @return void Just has a return statement in case visitMethod changes
+ */
+ public function visitFuncDecl( Decl $node ) {
+ return $this->visitMethod( $node );
+ }
+
+ /**
+ * Set taint for certain hook types.
+ *
+ * Also handles FuncDecl
+ * @param Decl $node
+ */
+ public function visitMethod( Decl $node ) {
+ $method = $this->context->getFunctionLikeInScope(
$this->code_base );
+ $hookType = $this->plugin->isSpecialHookSubscriber(
$method->getFQSEN() );
+ if ( !$hookType ) {
+ return;
+ }
+ $params = $node->children['params']->children;
+
+ switch ( $hookType ) {
+ case '!ParserFunctionHook':
+ $this->setFuncHookParamTaint( $params, $method );
+ break;
+ }
+ }
+
+ /**
+ * Set the appropriate taint for a parser function hook
+ *
+ * Basically all but the first arg comes from wikitext
+ * and is tainted.
+ *
+ * @todo This is handling SFH_OBJECT type func hooks incorrectly.
+ * @param Node[] $params Children of the AST_PARAM_LIST
+ * @param FunctionInterface $method
+ */
+ private function setFuncHookParamTaint( array $params,
FunctionInterface $method ) {
+ $this->debug( __METHOD__, "Setting taint for hook $method" );
+ $funcTaint = $this->getTaintOfFunction( $method );
+ $varObjs = [];
+ foreach ( $params as $i => $param ) {
+ if ( $i === 0 ) {
+ continue;
+ }
+ $scope = $this->context->getScope();
+ if ( !$scope->hasVariableWithName(
$param->children['name'] ) ) {
+ // Well uh-oh.
+ $this->debug( __METHOD__, "Missing variable for
param \$" . $param->children['name'] );
+ continue;
+ }
+ $varObj = $scope->getVariableByName(
$param->children['name'] );
+ $this->setTaintedness( $varObj,
SecurityCheckPlugin::YES_TAINT );
+ /*** Is this needed ? Disabling for now.
+ if ( isset( $funcTaint[$i] ) ) {
+ if ( !$this->isSafeAssignment(
+ $funcTaint[$i],
+ SecurityCheckPlugin::YES_TAINT
+ ) ) {
+ $funcName = $method->getFQSEN();
+ $this->plugin->emitIssue(
+ $this->code_base,
+ $this->context,
+ 'SecurityCheckTaintedOutput',
+ "Outputting evil HTML from
Parser function hook $funcName (case 2)"
+ .
$this->getOriginalTaintLine( $method )
+ );
+ }
+ }
+ */
+ }
+ }
+}
diff --git a/src/MWVisitor.php b/src/MWVisitor.php
new file mode 100644
index 0000000..76a9909
--- /dev/null
+++ b/src/MWVisitor.php
@@ -0,0 +1,169 @@
+<?php
+
+use Phan\Language\Context;
+use Phan\Language\FQSEN\FullyQualifiedMethodName;
+use Phan\Language\FQSEN\FullyQualifiedFunctionName;
+use Phan\Language\FQSEN;
+use Phan\Plugin;
+use Phan\CodeBase;
+use ast\Node;
+
+/**
+ * MediaWiki specific node visitor
+ */
+class MWVisitor extends TaintednessBaseVisitor {
+
+ /**
+ * Constructor to enforce plugin is instance of
MediaWikiSecurityCheckPlugin
+ *
+ * @param CodeBase $code_base
+ * @param Context $context
+ * @param MediaWikiSecurityCheckPlugin $plugin
+ */
+ public function __construct(
+ CodeBase $code_base,
+ Context $context,
+ MediaWikiSecurityCheckPlugin $plugin
+ ) {
+ parent::__construct( $code_base, $context, $plugin );
+ // Ensure phan knows plugin is right type
+ $this->plugin = $plugin;
+ }
+
+ /**
+ * @param Node $node
+ */
+ public function visit( Node $node ) {
+ }
+
+ /**
+ * Try and recognize
+ * @param Node $node
+ */
+ public function visitMethodCall( Node $node ) {
+ try {
+ $ctx = $this->getCtxN( $node );
+ $methodName = $node->children['method'];
+ $method = $ctx->getMethod(
+ $methodName,
+ false /* not a static call */
+ );
+ // Should this be getDefiningFQSEN() instead?
+ switch ( (string)$method->getFQSEN() ) {
+ case '\Parser::setFunctionHook':
+ $this->handleFuncRegistration( $node );
+ break;
+ }
+ } catch ( Exception $e ) {
+ // ignore
+ }
+ }
+
+ /**
+ * When someone calls $parser->setFunctionHook()
+ *
+ * @note Causes phan to error out if given non-existent class
+ * @param Node $node
+ */
+ private function handleFuncRegistration( Node $node ) {
+ $args = $node->children['args']->children;
+ if ( count( $args ) < 2 ) {
+ return;
+ }
+ $callback = $this->getFQSENFromCallable( $args[1] );
+ if ( $callback ) {
+ $alreadyRegistered = $this->plugin->registerHook(
'!ParserFunctionHook', $callback );
+ if ( !$alreadyRegistered ) {
+ // If this is the first time seeing this,
re-analyze the
+ // node, just in case we had already passed it
by.
+ if ( $callback->isClosure() ) {
+ // For closures we have to reanalyze
the parent
+ // function, as we can't reanalyze the
closure, and
+ // we definitely need to since the
closure would
+ // have already been analyzed at this
point since
+ // we are operating in post-order.
+ $func =
$this->context->getFunctionLikeInScope( $this->code_base );
+ } elseif ( $callback instanceof
FullyQualifiedMethodName ) {
+ $func =
$this->code_base->getMethodByFQSEN( $callback );
+ } else {
+ assert( $callback instanceof
FullyQualifiedFunctionName );
+ $func =
$this->code_base->getFunctionByFQSEN( $callback );
+ }
+ $func->analyze(
+ $func->getContext(),
+ $this->code_base
+ );
+ }
+ }
+ }
+
+ /**
+ * For special hooks, check their return value
+ *
+ * e.g. A tag hook's return value is output as html.
+ * @param Node $node
+ */
+ public function visitReturn( Node $node ) {
+ if (
+ !$this->context->isInFunctionLikeScope()
+ || !$node->children['expr'] instanceof Node
+ ) {
+ return;
+ }
+ $funcFQSEN = $this->context->getFunctionLikeFQSEN();
+ $hookType = $this->plugin->isSpecialHookSubscriber( $funcFQSEN
);
+ switch ( $hookType ) {
+ case '!ParserFunctionHook':
+ $this->visitReturnOfFunctionHook(
$node->children['expr'], $funcFQSEN );
+ break;
+
+ }
+ }
+
+ /**
+ * Check to see if isHTML => true and is tainted.
+ *
+ * @param Node $node The expr child of the return. NOT the return itself
+ * @suppress PhanTypeMismatchForeach
+ */
+ private function visitReturnOfFunctionHook( Node $node, FQSEN $funcName
) {
+ if (
+ !( $node instanceof Node ) ||
+ $node->kind !== \ast\AST_ARRAY ||
+ count( $node->children ) < 2
+ ) {
+ return;
+ }
+ $isHTML = false;
+ foreach ( $node->children as $child ) {
+ assert(
+ $child instanceof Node
+ && $child->kind === \ast\AST_ARRAY_ELEM
+ );
+
+ if (
+ $child->children['key'] === 'isHTML' &&
+ $child->children['value'] instanceof Node &&
+ $child->children['value']->kind ===
\ast\AST_CONST &&
+ $child->children['value']->children['name']
instanceof Node &&
+
$child->children['value']->children['name']->children['name'] === 'true'
+ ) {
+ $isHTML = true;
+ break;
+ }
+ }
+ if ( !$isHTML ) {
+ return;
+ }
+ $taintedness = $this->getTaintedness( $node->children[0] );
+ if ( !$this->isSafeAssignment(
SecurityCheckPlugin::HTML_EXEC_TAINT, $taintedness ) ) {
+ $this->plugin->emitIssue(
+ $this->code_base,
+ $this->context,
+ 'SecurityCheckTaintedOutput',
+ "Outputting evil HTML from Parser function hook
$funcName"
+ . $this->getOriginalTaintLine(
$node->children[0] )
+ );
+ }
+ }
+}
diff --git a/src/MediaWikiSecurityCheckPlugin.php
b/src/MediaWikiSecurityCheckPlugin.php
index 232ecfe..e621b94 100644
--- a/src/MediaWikiSecurityCheckPlugin.php
+++ b/src/MediaWikiSecurityCheckPlugin.php
@@ -3,8 +3,52 @@
* Copyright Brian Wolff 2017. Released under the GPL version 2 or later.
*/
require_once __DIR__ . "/SecurityCheckPlugin.php";
+require_once __DIR__ . "/MWVisitor.php";
+require_once __DIR__ . "/MWPreVisitor.php";
+
+use Phan\CodeBase;
+use Phan\Language\Context;
+use Phan\Language\FQSEN\FullyQualifiedFunctionLikeName;
+use ast\Node;
class MediaWikiSecurityCheckPlugin extends SecurityCheckPlugin {
+
+ /**
+ * @var Array A mapping from hook names to FQSEN that implement it
+ */
+ protected $hookSubscribers = [];
+
+ /**
+ * Override so we can check for hook registration
+ *
+ * @param CodeBase $code_base
+ * @param Context $context
+ * @param Node $node
+ * @param Node $parentNode
+ */
+ public function analyzeNode(
+ CodeBase $code_base,
+ Context $context,
+ Node $node,
+ Node $parentNode = null
+ ) {
+ parent::analyzeNode( $code_base, $context, $node, $parentNode );
+
+ $visitor = new MWVisitor( $code_base, $context, $this );
+ $visitor( $node );
+ }
+
+ /**
+ * Called on every node in the ast, but in pre-order
+ *
+ * @param CodeBase $code_base
+ * @param Context $context
+ * @param Node $node
+ */
+ public function preAnalyzeNode( CodeBase $code_base, Context $context,
Node $node ) {
+ parent::preAnalyzeNode( $code_base, $context, $node );
+ ( new MWPreVisitor( $code_base, $context, $this ) )( $node );
+ }
/**
* @inheritDoc
@@ -76,6 +120,13 @@
SecurityCheckPlugin::HTML_EXEC_TAINT,
// meh, not sure how right the overall is.
'overall' => SecurityCheckPlugin::HTML_TAINT
+ ],
+ // AddItem should also take care of addGeneral and
friends.
+ '\StripState::addItem' => [
+ self::NO_TAINT, // type
+ self::NO_TAINT, // marker
+ self::HTML_EXEC_TAINT, // contents
+ 'overall' => self::NO_TAINT
],
// FIXME Doesn't handle array args right.
'\wfShellExec' => [
@@ -188,6 +239,68 @@
];
}
+ /**
+ * Add a hook implementation to our list.
+ *
+ * This also handles parser hooks which aren't normal hooks.
+ * Non-normal hooks start their name with a "!"
+ *
+ * @param string $hookName Name of hook
+ * @param FullyQualifiedFunctionLikeName $fqsen The implementing method
+ * @return bool true if already registered, false otherwise
+ */
+ public function registerHook( string $hookName,
FullyQualifiedFunctionLikeName $fqsen ) {
+ if ( !isset( $this->hookSubscribers[$hookName] ) ) {
+ $this->hookSubscribers[$hookName] = [];
+ }
+ foreach ( $this->hookSubscribers[$hookName] as $subscribe ) {
+ if ( (string)$subscribe === (string)$fqsen ) {
+ // dupe
+ return true;
+ }
+ }
+ $this->hookSubscribers[$hookName][] = $fqsen;
+ return false;
+ }
+
+ /**
+ * Get a list of subscribers for hook
+ *
+ * @param string $hookName Hook in question. Hooks starting with ! are
special.
+ * @return FullyQualifiedFunctionLikeName[]
+ */
+ public function getHookSubscribers( string $hookName ) : array {
+ if ( isset( $this->hookSubscribers[$hookName] ) ) {
+ return $this->hookSubscribers[$hookName];
+ }
+ return [];
+ }
+
+ /**
+ * Is a particular function implementing a special hook.
+ *
+ * @note This assumes that any given func will only implement
+ * one hook
+ * @param FullyQualifiedFunctionLikeName $fqsen The function to check
+ * @return string The hook it is implementing
+ */
+ public function isSpecialHookSubscriber( FullyQualifiedFunctionLikeName
$fqsen ) {
+ $specialHooks = [
+ '!ParserFunctionHook'
+ ];
+
+ // @todo This is probably not the most efficient thing.
+ foreach ( $specialHooks as $hook ) {
+ if ( !isset( $this->hookSubscribers[$hook] ) ) {
+ continue;
+ }
+ foreach ( $this->hookSubscribers[$hook] as $implFQSEN )
{
+ if ( (string)$implFQSEN === (string)$fqsen ) {
+ return $hook;
+ }
+ }
+ }
+ }
}
return new MediaWikiSecurityCheckPlugin;
diff --git a/src/TaintednessBaseVisitor.php b/src/TaintednessBaseVisitor.php
index 58195b7..52fb30d 100644
--- a/src/TaintednessBaseVisitor.php
+++ b/src/TaintednessBaseVisitor.php
@@ -9,7 +9,10 @@
use Phan\Language\Element\TypedElementInterface;
use Phan\Language\Element\ClassElement;
use Phan\Language\UnionType;
+use Phan\Language\Type\CallableType;
use Phan\Language\FQSEN\FullyQualifiedFunctionLikeName;
+use Phan\Language\FQSEN\FullyQualifiedFunctionName;
+use Phan\Language\FQSEN\FullyQualifiedMethodName;
use Phan\Plugin;
use ast\Node;
use Phan\Debug;
@@ -74,7 +77,10 @@
if ( $override ) {
$newTaint = $taint;
}
- foreach ( $taint as $index => $t ) {
+
+ $bothTaint = $taint + $curTaint;
+ foreach ( $bothTaint as $index => $_ ) {
+ $t = $taint[$index] ?? 0;
assert( is_int( $t ) );
if ( !$override ) {
$newTaint[$index] = ( $curTaint[$index] ?? 0 )
| $t;
@@ -192,7 +198,7 @@
$parentVarObj->taintedOriginalError =&
$variableObj->taintedOriginalError;
} else {
- $this->debug( __METHOD__, "var
{$variableObj->getName()} does not exist outside branch!" );
+ // $this->debug( __METHOD__, "var
{$variableObj->getName()} does not exist outside branch!" );
}
}
// This may not be executed, so it can only increase
@@ -825,7 +831,7 @@
$paramTaint = [ 'overall' =>
SecurityCheckPlugin::NO_TAINT ];
foreach ( $paramInfo as $i => $_ ) {
$paramTaint[$i] = $taint;
- // $this->debug( __METHOD__ , "Setting method
$method" .
+ // $this->debug( __METHOD__, "Setting method
$method" .
// " arg $i as $taint due to depenency
on $var" );
}
$this->setFuncTaint( $method, $paramTaint );
@@ -1114,4 +1120,161 @@
assert( is_int( $t ) && $t >= 0, "Taint index $i wrong
$t" . $this->dbgInfo() );
}
}
+
+ /**
+ * Given an AST node that's a callable, try and determine what it is
+ *
+ * This is intended for functions that register callbacks. It will
+ * only really work for callbacks that are basically literals.
+ *
+ * @note $node may not be the current node in $this->context.
+ *
+ * @param Node|string $node The thingy from AST expected to be a
Callable
+ * @return FullyQualifiedFunctionLikeName|null The corresponding FQSEN
+ */
+ protected function getFQSENFromCallable( $node ) {
+ $callback = null;
+ if ( is_string( $node ) ) {
+ // Easy case, 'Foo::Bar'
+ if ( strpos( '::', $callback ) === false ) {
+ $callback =
FullyQualifiedFunctionName::fromFullyQualifiedString(
+ $node
+ );
+ } else {
+ $callback =
FullyQualifiedMethodName::fromFullyQualifiedString(
+ $node
+ );
+ }
+ } elseif ( $node instanceof Node && $node->kind ===
\ast\AST_CLOSURE ) {
+ $method = (
+ new ContextNode(
+ $this->code_base,
+ $this->context->withLineNumberStart(
+ $node->lineno ?? 0
+ ),
+ $node
+ )
+ )->getClosure();
+ $callback = $method->getFQSEN();
+ } elseif (
+ $node instanceof Node
+ && $node->kind === \ast\AST_VAR
+ && is_string( $node->children['name'] )
+ ) {
+ $cnode = $this->getCtxN( $node );
+ $var = $cnode->getVariable();
+ $types = $var->getUnionType()->getTypeSet();
+ foreach ( $types as $type ) {
+ if (
+ $type instanceof CallableType &&
+ $type->asFQSEN() instanceof
FullyQualifiedFunctionLikeName
+ ) {
+ // @todo FIXME This doesn't work if the
closure
+ // is defined in a different function
scope
+ // then the one we are currently in.
Perhaps
+ // we could look up the closure in
+ // $this->code_base to figure out what
func
+ // its defined on via its parent scope.
Or
+ // something.
+ $callback = $type->asFQSEN();
+ break;
+ }
+ }
+ } elseif ( $node instanceof Node && $node->kind ===
\ast\AST_ARRAY ) {
+ if ( count( $node->children ) !== 2 ) {
+ return null;
+ }
+ if (
+ $node->children[0]->children['key'] !== null ||
+ $node->children[1]->children['key'] !== null ||
+ !is_string(
$node->children[1]->children['value'] )
+ ) {
+ return null;
+ }
+ $methodName = $node->children[1]->children['value'];
+ $classNode = $node->children[0]->children['value'];
+ if ( is_string( $node->children[0]->children['value'] )
) {
+ $className = $classNode;
+ } elseif ( $classNode instanceof Node ) {
+ switch ( $classNode->kind ) {
+ case \ast\AST_MAGIC_CONST:
+ // Mostly a special case for MediaWiki
+ // CoreParserFunctions.php
+ if (
+ $classNode->flags &
\ast\flags\MAGIC_CLASS !== 0
+ &&
$this->context->isInClassScope()
+ ) {
+ $className =
(string)$this->context->getClassFQSEN();
+ } else {
+ return null;
+ }
+ break;
+ case \ast\AST_CLASS_CONST:
+ if (
+ $classNode->children['const']
=== 'class' &&
+
$classNode->children['class']->kind === \ast\AST_NAME &&
+ is_string(
$classNode->children['class']->children['name'] )
+ ) {
+ $className =
$classNode->children['class']->children['name'];
+ } else {
+ return null;
+ }
+ break;
+ case \ast\AST_VAR:
+ $var = $this->getCtxN( $classNode
)->getVariable();
+ $type = $var->getUnionType();
+ if ( $type->typeCount() !== 1 ||
$type->isScalar() ) {
+ return null;
+ }
+ $cl = $type->asClassList(
+ $this->code_base,
+ $this->context
+ );
+ $clazz = false;
+ foreach ( $cl as $item ) {
+ $clazz = $item;
+ break;
+ }
+ if ( !$clazz ) {
+ return null;
+ }
+ $className = (string)$clazz->getFQSEN();
+ break;
+ case \ast\AST_PROP:
+ $var = $this->getCtxN( $classNode )
+ ->getProperty(
$classNode->children['prop'] );
+ $type = $var->getUnionType();
+ if ( $type->typeCount() !== 1 ||
$type->isScalar() ) {
+ return null;
+ }
+ $cl = $type->asClassList(
+ $this->code_base,
+ $this->context
+ );
+ $clazz = false;
+ foreach ( $cl as $item ) {
+ $clazz = $item;
+ break;
+ }
+ if ( !$clazz ) {
+ return null;
+ }
+ $className = (string)$clazz->getFQSEN();
+ break;
+ default:
+ return null;
+ }
+
+ } else {
+ return null;
+ }
+ // Note, not from in context, since this goes to
call_user_func.
+ $callback =
FullyQualifiedMethodName::fromFullyQualifiedString(
+ $className . '::' . $methodName
+ );
+ } else {
+ return null;
+ }
+ return $callback;
+ }
}
diff --git a/src/TaintednessVisitor.php b/src/TaintednessVisitor.php
index aa4b54c..7a45000 100644
--- a/src/TaintednessVisitor.php
+++ b/src/TaintednessVisitor.php
@@ -104,6 +104,22 @@
* @param Node $node
* @return int Taint
*/
+ public function visitUseElem( Node $node ) : int {
+ return SecurityCheckPlugin::INAPLICABLE_TAINT;
+ }
+
+ /**
+ * @param Node $node
+ * @return int Taint
+ */
+ public function visitType( Node $node ) : int {
+ return SecurityCheckPlugin::INAPLICABLE_TAINT;
+ }
+
+ /**
+ * @param Node $node
+ * @return int Taint
+ */
public function visitArgList( Node $node ) : int {
return SecurityCheckPlugin::INAPLICABLE_TAINT;
}
@@ -546,8 +562,8 @@
if ( isset( $taint[$i] ) ) {
$effectiveArgTaintedness = $curArgTaintedness &
( $taint[$i] | $this->execToYesTaint(
$taint[$i] ) );
- # $this->debug( __METHOD__, "effective
$effectiveArgTaintedness"
- # . " via arg $i $funcName" );
+ // $this->debug( __METHOD__, "effective
$effectiveArgTaintedness"
+ // . " via arg $i $funcName" );
} elseif ( ( $taint['overall'] &
( SecurityCheckPlugin::PRESERVE_TAINT |
SecurityCheckPlugin::UNKNOWN_TAINT )
) ) {
@@ -621,8 +637,8 @@
// someFunc( $execArg ) for pass by reference where
// the parameter is later executed outside the func.
if ( $func && $this->isYesTaint( $curArgTaintedness ) )
{
- # $this->debug( __METHOD__, "cur arg $i is YES
taint " .
- # "($curArgTaintedness). Marking dependent
$funcName" );
+ // $this->debug( __METHOD__, "cur arg $i is YES
taint " .
+ // "($curArgTaintedness). Marking dependent
$funcName" );
// Mark all dependent vars as tainted.
$this->markAllDependentVarsYes( $func, $i );
}
@@ -631,7 +647,7 @@
// where $arg is a parameter to the current function.
// So backpropagate that assigning to $arg can cause
evilness.
if ( $this->isExecTaint( $taint[$i] ?? 0 ) ) {
- # $this->debug( __METHOD__, "cur param is EXEC.
$funcName" );
+ // $this->debug( __METHOD__, "cur param is
EXEC. $funcName" );
try {
$phanObjs = $this->getPhanObjsForNode(
$argument );
foreach ( $phanObjs as $phanObj ) {
@@ -646,7 +662,7 @@
// gets passed to return value, but which taint is
EXECed.
// $this->debug( __METHOD__, "Checking safe assing
$funcName" .
// " arg=$i paramTaint= " . ( $taint[$i] ??
"MISSING" ) .
- // " vs argTaint= $curArgTaintedness" );
+ // " vs argTaint= $curArgTaintedness" );
if ( !$this->isSafeAssignment( $taint[$i] ?? 0,
$curArgTaintedness ) ) {
$containingMethod = $this->getCurrentMethod();
$this->plugin->emitIssue(
diff --git a/tests/integration/callbackhook/Parser.php
b/tests/integration/callbackhook/Parser.php
new file mode 100644
index 0000000..9def58e
--- /dev/null
+++ b/tests/integration/callbackhook/Parser.php
@@ -0,0 +1,64 @@
+<?php
+
+class Parser {
+
+ const MARKER_SUFFIX = "-QINU`\"'\x7f";
+ const MARKER_PREFIX = "\x7f'\"`UNIQ-";
+ /** @var int */
+ private $mMarkerIndex = 0;
+ /** @var StripState */
+ public $mStripState;
+ public function setFunctionHook( $id, callable $cb, $opts = 0 ) {
+ }
+
+ public function __construct() {
+ $this->mStripState = new StripState;
+ }
+ public function bar( Parser $parser, $arg1, $arg2 ) {
+ return $arg1;
+ }
+
+ public function insertStripItem( $text ) {
+ $marker = self::MARKER_PREFIX . "-item-{$this->mMarkerIndex}-"
. self::MARKER_SUFFIX;
+ $this->mMarkerIndex++;
+ $this->mStripState->addGeneral( $marker, $text );
+ return $marker;
+ }
+
+}
+
+class StripState {
+ protected $prefix;
+ protected $data;
+ protected $regex;
+
+ protected $tempType, $tempMergePrefix;
+ protected $circularRefGuard;
+ protected $recursionLevel = 0;
+
+ const UNSTRIP_RECURSION_LIMIT = 20;
+ protected function addItem( $type, $marker, $value ) {
+ if ( !preg_match( $this->regex, $marker, $m ) ) {
+ throw new Exception( "Invalid marker: $marker" );
+ }
+
+ $this->data[$type][$m[1]] = $value;
+ }
+
+ /**
+ * Add a nowiki strip item
+ * @param string $marker
+ * @param string $value
+ */
+ public function addNoWiki( $marker, $value ) {
+ $this->addItem( 'nowiki', $marker, $value );
+ }
+
+ /**
+ * @param string $marker
+ * @param string $value
+ */
+ public function addGeneral( $marker, $value ) {
+ $this->addItem( 'general', $marker, $value );
+ }
+}
diff --git a/tests/integration/callbackhook/db.php
b/tests/integration/callbackhook/db.php
new file mode 100644
index 0000000..93be506
--- /dev/null
+++ b/tests/integration/callbackhook/db.php
@@ -0,0 +1,20 @@
+<?php
+namespace Wikimedia\Rdbms;
+
+interface IDatabase {
+ public function query( $sql, $method );
+}
+
+class Database implements IDatabase {
+ public function query( $sql, $method = 'query' ) {
+ // do some stuff
+ return [ (object)[ 'some_field' => 'some value' ] ];
+ }
+
+}
+
+class MysqlDatabase extends Database {
+ public function getType() {
+ return 'mysql';
+ }
+}
diff --git a/tests/integration/callbackhook/expectedResults.txt
b/tests/integration/callbackhook/expectedResults.txt
new file mode 100644
index 0000000..550ef70
--- /dev/null
+++ b/tests/integration/callbackhook/expectedResults.txt
@@ -0,0 +1,7 @@
+integration/callbackhook/func.php:5 SecurityCheckTaintedOutput Outputting evil
HTML from Parser function hook \wfSomeFunc (Caused by:
integration/callbackhook/func.php +3; integration/callbackhook/func.php +4)
+integration/callbackhook/func.php:5 SecurityCheckTaintedOutput Outputting evil
HTML from Parser function hook \wfSomeFunc (Caused by:
integration/callbackhook/func.php +3; integration/callbackhook/func.php +4;
integration/callbackhook/func.php +3; integration/callbackhook/func.php +4)
+integration/callbackhook/test.php:12 SecurityCheckTaintedOutput Outputting
evil HTML from Parser function hook \closure_eabad951a64c
+integration/callbackhook/test.php:20 SecurityCheckTaintedOutput Outputting
evil HTML from Parser function hook \closure_fbf3d4f34852
+integration/callbackhook/test.php:30 SecurityCheckTaintedOutput Calling Method
\Wikimedia\Rdbms\MysqlDatabase::query in \MyNS\SomeClass::bar that outputs
using tainted (2040; 64) argument $[arg #1]. (Caused by:
integration/callbackhook/test.php +27; integration/callbackhook/test.php +30;
integration/callbackhook/test.php +30)
+integration/callbackhook/test.php:30 SecurityCheckTaintedOutput Calling Method
\Wikimedia\Rdbms\MysqlDatabase::query in \MyNS\SomeClass::bar that outputs
using tainted (680; 64) argument $[arg #1]. (Caused by:
integration/callbackhook/test.php +27; integration/callbackhook/test.php +30)
+integration/callbackhook/test.php:40 SecurityCheckTaintedOutput Outputting
evil HTML from Parser function hook \MyNS\SomeClass::baz (Caused by:
integration/callbackhook/test.php +39)
diff --git a/tests/integration/callbackhook/func.php
b/tests/integration/callbackhook/func.php
new file mode 100644
index 0000000..eef9f60
--- /dev/null
+++ b/tests/integration/callbackhook/func.php
@@ -0,0 +1,6 @@
+<?php
+function wfSomeFunc() {
+ $someEvil = $_GET['d'];
+ $someEvil .= "More text";
+ return [ $someEvil, 'isHTML' => true ];
+}
diff --git a/tests/integration/callbackhook/test.php
b/tests/integration/callbackhook/test.php
new file mode 100644
index 0000000..148501f
--- /dev/null
+++ b/tests/integration/callbackhook/test.php
@@ -0,0 +1,47 @@
+<?php
+namespace MyNS;
+
+use \Parser;
+use Wikimedia\Rdbms\MysqlDatabase;
+
+class SomeClass {
+ public function register() {
+ $parser = new Parser;
+
+ $indirectClosure = function ( Parser $parser, $arg ) {
+ return [ $arg, 'isHTML' => true ];
+ };
+
+ $parser->setFunctionHook( 'something', [ __CLASS__, 'bar' ] );
+ $parser->setFunctionHook( 'something', [ __CLASS__, 'baz' ] );
+
+ $parser->setFunctionHook( 'afunc', 'wfSomeFunc' );
+ $parser->setFunctionHook( 'two', function ( Parser $parser,
$arg1 ) {
+ return [ $_GET['d'], 'isHTML' => true ];
+ }
+ );
+
+ $parser->setFunctionHook( 'three', $indirectClosure );
+ }
+
+ public function bar( Parser $parser, $arg1, $arg2 ) {
+ $out = '';
+ $db = new MysqlDatabase;
+ $res = $db->query( "Select foo from someTable WHERE field =
'$arg2' LIMIT 1" );
+ foreach ( $res as $row ) {
+ $out .= $row->foo;
+ }
+ $out .= $arg1;
+
+ return $out;
+ }
+
+ public function baz( Parser $parser, $arg1 ) {
+ return [ $arg1, 'isHTML' => true ];
+ }
+}
+
+function wfSomeFunc() {
+ // This should not trigger because its in wrong NS
+ return [ $_GET['d'], 'isHTML' => true ];
+}
diff --git a/tests/integration/stripitem/Parser.php
b/tests/integration/stripitem/Parser.php
new file mode 100644
index 0000000..ac3ebc1
--- /dev/null
+++ b/tests/integration/stripitem/Parser.php
@@ -0,0 +1,61 @@
+<?php
+
+class Parser {
+
+ const MARKER_SUFFIX = "-QINU`\"'\x7f";
+ const MARKER_PREFIX = "\x7f'\"`UNIQ-";
+ /** @var int */
+ private $mMarkerIndex = 0;
+ /** @var StripState */
+ public $mStripState;
+ public function setFunctionHook( $id, callable $cb, $opts = 0 ) {
+ }
+
+ public function __construct() {
+ $this->mStripState = new StripState;
+ }
+
+ public function insertStripItem( $text ) {
+ $marker = self::MARKER_PREFIX . "-item-{$this->mMarkerIndex}-"
. self::MARKER_SUFFIX;
+ $this->mMarkerIndex++;
+ $this->mStripState->addGeneral( $marker, $text );
+ return $marker;
+ }
+
+}
+
+class StripState {
+ protected $prefix;
+ protected $data;
+ protected $regex;
+
+ protected $tempType, $tempMergePrefix;
+ protected $circularRefGuard;
+ protected $recursionLevel = 0;
+
+ const UNSTRIP_RECURSION_LIMIT = 20;
+ protected function addItem( $type, $marker, $value ) {
+ if ( !preg_match( $this->regex, $marker, $m ) ) {
+ throw new Exception( "Invalid marker: $marker" );
+ }
+
+ $this->data[$type][$m[1]] = $value;
+ }
+
+ /**
+ * Add a nowiki strip item
+ * @param string $marker
+ * @param string $value
+ */
+ public function addNoWiki( $marker, $value ) {
+ $this->addItem( 'nowiki', $marker, $value );
+ }
+
+ /**
+ * @param string $marker
+ * @param string $value
+ */
+ public function addGeneral( $marker, $value ) {
+ $this->addItem( 'general', $marker, $value );
+ }
+}
diff --git a/tests/integration/stripitem/expectedResults.txt
b/tests/integration/stripitem/expectedResults.txt
new file mode 100644
index 0000000..e5e03e8
--- /dev/null
+++ b/tests/integration/stripitem/expectedResults.txt
@@ -0,0 +1,4 @@
+integration/stripitem/test.php:11 SecurityCheckTaintedOutput Calling Method
\Parser::insertStripItem in \SomeClass::xyzzy that outputs using tainted (2040;
1360) argument $arg1. (Caused by: integration/stripitem/Parser.php +22;
integration/stripitem/Parser.php +22) (Caused by:
integration/stripitem/test.php +10; integration/stripitem/test.php +11;
integration/stripitem/test.php +11)
+integration/stripitem/test.php:11 SecurityCheckTaintedOutput Calling Method
\Parser::insertStripItem in \SomeClass::xyzzy that outputs using tainted (2040;
1360) argument $arg1. (Caused by: integration/stripitem/Parser.php +22;
integration/stripitem/Parser.php +22; integration/stripitem/Parser.php +22)
(Caused by: integration/stripitem/test.php +10; integration/stripitem/test.php
+11; integration/stripitem/test.php +11)
+integration/stripitem/test.php:11 SecurityCheckTaintedOutput Calling Method
\Parser::insertStripItem in \SomeClass::xyzzy that outputs using tainted (680;
1360) argument $arg1. (Caused by: integration/stripitem/Parser.php +22;
integration/stripitem/Parser.php +22) (Caused by:
integration/stripitem/test.php +10; integration/stripitem/test.php +11)
+integration/stripitem/test.php:11 SecurityCheckTaintedOutput Calling Method
\Parser::insertStripItem in \SomeClass::xyzzy that outputs using tainted (680;
1360) argument $arg1. (Caused by: integration/stripitem/Parser.php +22;
integration/stripitem/Parser.php +22; integration/stripitem/Parser.php +22)
(Caused by: integration/stripitem/test.php +10; integration/stripitem/test.php
+11)
diff --git a/tests/integration/stripitem/test.php
b/tests/integration/stripitem/test.php
new file mode 100644
index 0000000..ba8aaf5
--- /dev/null
+++ b/tests/integration/stripitem/test.php
@@ -0,0 +1,14 @@
+<?php
+
+class SomeClass {
+ public function register() {
+ $parser = new Parser;
+
+ $parser->setFunctionHook( 'something', [ __CLASS__, 'xyzzy' ] );
+ }
+
+ public function xyzzy( Parser $parser, $arg1 ) {
+ $a = $parser->insertStripItem( $arg1 );
+ return $a;
+ }
+}
diff --git a/tests/runtests.sh b/tests/runtests.sh
index d5bcd5c..a3de9af 100755
--- a/tests/runtests.sh
+++ b/tests/runtests.sh
@@ -1,18 +1,25 @@
#!/bin/bash
-# If you want to see debug output
-# call with SECCHECK_DEBUG=/dev/stderr
-# environment variable.
#
-# If you want to run a specific test
-# then give it as first arg.
+# Run the tests in integration.
+# Usage: ./runtests.sh [-v] [testname]
+#
+# -v will add debug output. testname will run only that test
+#
+
cd `dirname $0`/
-testList=${1:-`ls integration`}
tmpFile=`mktemp testtmp.XXXXXXXX`
+trap "rm $tmpFile" exit
totalTests=0
failedTests=0
php=`which php7.0`
php=${php:-`which php`}
-SECCHECK_DEBUG=${SECCHECK_DEBUG:-/dev/null}
+if [ "$1" = '-v' ]
+ then SECCHECK_DEBUG=/dev/stderr
+ shift
+ export SECCHECK_DEBUG
+ else SECCHECK_DEBUG=${SECCHECK_DEBUG:-/dev/null}
+fi
+testList=${1:-`ls integration`}
for i in $testList
do
@@ -28,7 +35,6 @@
then failedTests=$((failedTests+1))
fi
done
-rm $tmpFile
if [ $failedTests -gt 0 ]
then echo $failedTests out of $totalTests failed.
exit 1
--
To view, visit https://gerrit.wikimedia.org/r/393540
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Idcd74eee6ad5fb38d7c8b81f4cf1058dc26f080b
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/tools/phan/SecurityCheckPlugin
Gerrit-Branch: master
Gerrit-Owner: Brian Wolff <bawolff...@gmail.com>
Gerrit-Reviewer: Brian Wolff <bawolff...@gmail.com>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
