Brian Wolff has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/395762 )
Change subject: Fix some false positives discovered while testing with MW
......................................................................
Fix some false positives discovered while testing with MW
Change-Id: I3bcbd5bbfa7417c490adb2ec7b580e13eaa70741
---
M src/MWVisitor.php
M src/MediaWikiSecurityCheckPlugin.php
M src/TaintednessBaseVisitor.php
M src/TaintednessVisitor.php
M tests/integration/arraynumkey/db.php
M tests/integration/arraynumkey/expectedResults.txt
M tests/integration/arraynumkey/test.php
M tests/integration/lists/test.php
A tests/integration/lowercase/OutputPage.php
A tests/integration/lowercase/expectedResults.txt
A tests/integration/lowercase/test.php
A tests/integration/ooui/HorizontalLayout.php
A tests/integration/ooui/OutputPage.php
A tests/integration/ooui/expectedResults.txt
A tests/integration/ooui/test.php
15 files changed, 245 insertions(+), 13 deletions(-)
diff --git a/src/MWVisitor.php b/src/MWVisitor.php
index 24c1328..d7a1533 100644
--- a/src/MWVisitor.php
+++ b/src/MWVisitor.php
@@ -270,13 +270,16 @@
if ( !$alreadyRegistered ) {
// If this is the first time seeing this, re-analyze the
// node, just in case we had already passed it by.
+ $func = null;
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 );
+ if ( $this->context->isInFunctionLikeScope() ) {
+ $func =
$this->context->getFunctionLikeInScope( $this->code_base );
+ }
} elseif ( $callback instanceof
FullyQualifiedMethodName ) {
$func = $this->code_base->getMethodByFQSEN(
$callback );
} else {
@@ -286,10 +289,12 @@
// Make sure we reanalyze the hook function now that
// we know what it is, in case its already been
// analyzed.
- $func->analyze(
- $func->getContext(),
- $this->code_base
- );
+ if ( $func ) {
+ $func->analyze(
+ $func->getContext(),
+ $this->code_base
+ );
+ }
}
}
diff --git a/src/MediaWikiSecurityCheckPlugin.php
b/src/MediaWikiSecurityCheckPlugin.php
index 0fa770d..31d7274 100644
--- a/src/MediaWikiSecurityCheckPlugin.php
+++ b/src/MediaWikiSecurityCheckPlugin.php
@@ -88,30 +88,55 @@
// What should DB results be considered?
'overall' => self::YES_TAINT
],
+ '\Wikimedia\Rdbms\IMaintainableDatabase::query' => [
+ self::SQL_EXEC_TAINT,
+ // What should DB results be considered?
+ 'overall' => self::YES_TAINT
+ ],
'\Wikimedia\Rdbms\DBConnRef::query' => [
self::SQL_EXEC_TAINT,
// What should DB results be considered?
'overall' => self::YES_TAINT
],
'\Wikimedia\Rdbms\IDatabase::select' => $selectWrapper,
+ '\Wikimedia\Rdbms\IMaintainableDatabase::select' =>
$selectWrapper,
'\Wikimedia\Rdbms\Database::select' => $selectWrapper,
'\Wikimedia\Rdbms\DBConnRef::select' => $selectWrapper,
'\Wikimedia\Rdbms\IDatabase::selectField' =>
$selectWrapper,
+ '\Wikimedia\Rdbms\IMaintainableDatabase::selectField'
=> $selectWrapper,
'\Wikimedia\Rdbms\Database::selectField' =>
$selectWrapper,
'\Wikimedia\Rdbms\DBConnRef::selectField' =>
$selectWrapper,
'\Wikimedia\Rdbms\IDatabase::selectFieldValues' =>
$selectWrapper,
+
'\Wikimedia\Rdbms\IMaintainableDatabase::selectFieldValues' => $selectWrapper,
'\Wikimedia\Rdbms\DBConnRef::selectFieldValues' =>
$selectWrapper,
'\Wikimedia\Rdbms\Database::selectFieldValues' =>
$selectWrapper,
- '\Wikimedia\Rdbms\IDatabase::selectSQLText' =>
$selectWrapper,
- '\Wikimedia\Rdbms\DBConnRef::selectSQLText' =>
$selectWrapper,
- '\Wikimedia\Rdbms\Database::selectSQLText' =>
$selectWrapper,
+ '\Wikimedia\Rdbms\IMaintainableDatabase::selectSQLText'
=> [
+ 'overall' => self::YES_TAINT &
~self::SQL_TAINT
+ ] + $selectWrapper,
+ '\Wikimedia\Rdbms\IDatabase::selectSQLText' => [
+ 'overall' => self::YES_TAINT &
~self::SQL_TAINT
+ ] + $selectWrapper,
+ '\Wikimedia\Rdbms\DBConnRef::selectSQLText' => [
+ 'overall' => self::YES_TAINT &
~self::SQL_TAINT
+ ] + $selectWrapper,
+ '\Wikimedia\Rdbms\Database::selectSQLText' => [
+ 'overall' => self::YES_TAINT &
~self::SQL_TAINT
+ ] + $selectWrapper,
'\Wikimedia\Rdbms\IDatabase::selectRowCount' =>
$selectWrapper,
+
'\Wikimedia\Rdbms\IMaintainableDatabase::selectRowCount' => $selectWrapper,
'\Wikimedia\Rdbms\Database::selectRowCount' =>
$selectWrapper,
'\Wikimedia\Rdbms\DBConnRef::selectRowCount' =>
$selectWrapper,
'\Wikimedia\Rdbms\IDatabase::selectRow' =>
$selectWrapper,
+ '\Wikimedia\Rdbms\IMaintainableDatabase::selectRow' =>
$selectWrapper,
'\Wikimedia\Rdbms\Database::selectRow' =>
$selectWrapper,
'\Wikimedia\Rdbms\DBConnRef::selectRow' =>
$selectWrapper,
'\Wikimedia\Rdbms\IDatabase::delete' => [
+ self::SQL_EXEC_TAINT,
+ self::SQL_NUMKEY_EXEC_TAINT,
+ self::SQL_EXEC_TAINT,
+ 'overall' => self::NO_TAINT
+ ],
+ '\Wikimedia\Rdbms\IMaintainableDatabase::delete' => [
self::SQL_EXEC_TAINT,
self::SQL_NUMKEY_EXEC_TAINT,
self::SQL_EXEC_TAINT,
@@ -130,6 +155,15 @@
'overall' => self::NO_TAINT
],
'\Wikimedia\Rdbms\IDatabase::insert' => [
+ self::SQL_EXEC_TAINT, // table name
+ // FIXME This doesn't correctly work
+ // when inserting multiple things at once.
+ self::SQL_NUMKEY_EXEC_TAINT,
+ self::SQL_EXEC_TAINT, // method name
+ self::SQL_EXEC_TAINT, // options. They are not
escaped
+ 'overall' => self::NO_TAINT
+ ],
+ '\Wikimedia\Rdbms\IMaintainableDatabase::insert' => [
self::SQL_EXEC_TAINT, // table name
// FIXME This doesn't correctly work
// when inserting multiple things at once.
@@ -157,6 +191,14 @@
'overall' => self::NO_TAINT
],
'\Wikimedia\Rdbms\IDatabase::update' => [
+ self::SQL_EXEC_TAINT, // table name
+ self::SQL_NUMKEY_EXEC_TAINT,
+ self::SQL_NUMKEY_EXEC_TAINT,
+ self::SQL_EXEC_TAINT, // method name
+ self::NO_TAINT, // options. They are validated
+ 'overall' => self::NO_TAINT
+ ],
+ '\Wikimedia\Rdbms\IMaintainableDatabase::update' => [
self::SQL_EXEC_TAINT, // table name
self::SQL_NUMKEY_EXEC_TAINT,
self::SQL_NUMKEY_EXEC_TAINT,
@@ -219,6 +261,10 @@
'overall' => self::NO_TAINT,
],
'\Wikimedia\Rdbms\IDatabase::addQuotes' => [
+ self::YES_TAINT & ~self::SQL_TAINT,
+ 'overall' => self::NO_TAINT,
+ ],
+ '\Wikimedia\Rdbms\IMaintainableDatabase::addQuotes' => [
self::YES_TAINT & ~self::SQL_TAINT,
'overall' => self::NO_TAINT,
],
@@ -331,6 +377,17 @@
SecurityCheckPlugin::NO_TAINT,
'overall' => SecurityCheckPlugin::NO_TAINT
],
+ '\Xml::encodeJsVar' => [
+ self::YES_TAINT & ~self::HTML_TAINT,
+ self::NO_TAINT, /* pretty */
+ 'overall' => self::NO_TAINT
+ ],
+ '\Xml::encodeJsCall' => [
+ self::YES_TAINT, /* func name. unescaped */
+ self::YES_TAINT & ~self::HTML_TAINT, /* args */
+ self::NO_TAINT, /* pretty */
+ 'overall' => self::NO_TAINT
+ ],
'\OutputPage::addHeadItem' => [
SecurityCheckPlugin::HTML_EXEC_TAINT,
'overall' => self::NO_TAINT,
@@ -443,6 +500,35 @@
'CommentStore::insertWithTempTable' => [
'overall' => self::NO_TAINT
],
+ // TODO FIXME, Why couldn't it figure out
+ // that this is safe on its own?
+ // It seems that it has issue with
+ // the url query parameters.
+ 'Linker::linkKnown' => [
+ self::NO_TAINT, /* target */
+ self::YES_TAINT, /* raw html text */
+ // The array keys for this aren't escaped (!)
+ self::NO_TAINT, /* customAttribs */
+ self::NO_TAINT, /* query */
+ self::NO_TAINT, /* options. All are safe */
+ 'overall' => self::NO_TAINT
+ ],
+ 'MediaWiki\Linker\LinkRenderer::buildAElement' => [
+ self::NO_TAINT, /* target */
+ self::NO_TAINT, /* text (using HtmlArmor) */
+ // The array keys for this aren't escaped (!)
+ self::NO_TAINT, /* attribs */
+ self::NO_TAINT, /* known */
+ 'overall' => self::NO_TAINT
+ ],
+ 'MediaWiki\Linker\LinkRenderer::makeLink' => [
+ self::NO_TAINT, /* target */
+ self::NO_TAINT, /* text (using HtmlArmor) */
+ // The array keys for this aren't escaped (!)
+ self::NO_TAINT, /* attribs */
+ self::NO_TAINT, /* query */
+ 'overall' => self::NO_TAINT
+ ],
];
}
diff --git a/src/TaintednessBaseVisitor.php b/src/TaintednessBaseVisitor.php
index 26acefa..c63003b 100644
--- a/src/TaintednessBaseVisitor.php
+++ b/src/TaintednessBaseVisitor.php
@@ -1529,7 +1529,7 @@
} elseif ( isset( $taint[$i] ) ) {
if (
( $taint[$i] &
SecurityCheckPlugin::SQL_NUMKEY_EXEC_TAINT )
- && !$this->nodeIsArray( $argument )
+ && $this->nodeIsString( $argument )
&& ( $curArgTaintedness &
SecurityCheckPlugin::SQL_TAINT )
) {
// Special case to make NUMKEY work
right for non-array
@@ -1716,6 +1716,42 @@
}
/**
+ * Given a Node, is it a string? (And definitely not an array)
+ *
+ * @todo Unclear if this should return true for things that can
+ * autocast to a string (e.g. ints)
+ * @param Mixed|Node $node A node object or simple value from AST tree
+ * @return bool Is it a string?
+ */
+ protected function nodeIsString( $node ) : bool {
+ if ( is_string( $node ) ) {
+ return true;
+ }
+ if ( !( $node instanceof Node ) ) {
+ // simple literal
+ return false;
+ }
+ try {
+ $type = UnionTypeVisitor::unionTypeFromNode(
+ $this->code_base,
+ $this->context,
+ $node
+ );
+ if (
+ !$type->hasArrayLike() &&
+ $type->hasType( StringType::instance() )
+ ) {
+ // FIXME TODO: Should having Mixed type
+ // result in returning false here?
+ return true;
+ }
+ } catch ( Exception $e ) {
+ $this->debug( __METHOD__, "Got error " . get_class( $e
) );
+ }
+ return false;
+ }
+
+ /**
* Given a Node, is it definitely an int (and nothing else)
*
* Floats are not considered ints here.
diff --git a/src/TaintednessVisitor.php b/src/TaintednessVisitor.php
index db0b046..11cb08e 100644
--- a/src/TaintednessVisitor.php
+++ b/src/TaintednessVisitor.php
@@ -778,12 +778,12 @@
& SecurityCheckPlugin::SQL_NUMKEY_TAINT
) {
$childTaint &=
~SecurityCheckPlugin::SQL_NUMKEY_TAINT;
-
- } elseif (
+ }
+ if (
( $this->getTaintedness( $key ) & $sqlTaint ) ||
( ( $key === null || $this->nodeIsInt( $key ) )
&& ( $this->getTaintedness( $value ) &
$sqlTaint )
- && !$this->nodeIsArray( $value ) )
+ && $this->nodeIsString( $value ) )
) {
$childTaint |=
SecurityCheckPlugin::SQL_NUMKEY_TAINT;
}
@@ -888,6 +888,8 @@
// Don't include serialize here due to high
false positives
// Eventhough unserializing stuff from db can
be very
// problematic if user can ever control.
+ // FIXME This is MW specific so should not be
+ // in the generic visitor.
return SecurityCheckPlugin::YES_TAINT &
~SecurityCheckPlugin::SERIALIZE_TAINT;
}
if (
diff --git a/tests/integration/arraynumkey/db.php
b/tests/integration/arraynumkey/db.php
index 7186cfe..25dd571 100644
--- a/tests/integration/arraynumkey/db.php
+++ b/tests/integration/arraynumkey/db.php
@@ -7,18 +7,40 @@
$table, $vars, $conds = '', $fname = __METHOD__,
$options = [], $join_conds = []
);
+ public function selectSQLText(
+ $table, $vars, $conds = '', $fname = __METHOD__,
+ $options = [], $join_conds = []
+ );
+ public function selectRow(
+ $table, $vars, $conds = '', $fname = __METHOD__,
+ $options = [], $join_conds = []
+ );
}
class Database implements IDatabase {
public function query( $sql, $method ) {
// do some stuff
- return (object)[ 'some_field' => 'some value' ];
+ return [ (object)[ 'some_field' => 'some value' ] ];
}
public function select(
$table, $vars, $conds = '', $fname = __METHOD__,
$options = [], $join_conds = []
) {
+ return [ (object)[ 'some_field' => 'some value' ] ];
+ }
+
+ public function selectSQLText(
+ $table, $vars, $conds = '', $fname = __METHOD__,
+ $options = [], $join_conds = []
+ ) {
+ return 'SELECT * FROM foo';
+ }
+
+ public function selectRow(
+ $table, $vars, $conds = '', $fname = __METHOD__,
+ $options = [], $join_conds = []
+ ) {
return (object)[ 'some_field' => 'some value' ];
}
}
diff --git a/tests/integration/arraynumkey/expectedResults.txt
b/tests/integration/arraynumkey/expectedResults.txt
index 949e140..4b38154 100644
--- a/tests/integration/arraynumkey/expectedResults.txt
+++ b/tests/integration/arraynumkey/expectedResults.txt
@@ -7,3 +7,4 @@
integration/arraynumkey/test.php:54 SecurityCheck-SQLInjection Calling method
\Wikimedia\Rdbms\MysqlDatabase::select() in [no method] that outputs using
tainted argument $where5. (Caused by: integration/arraynumkey/test.php +53)
integration/arraynumkey/test.php:59 SecurityCheck-SQLInjection The ON
conditions are not properly escaped for the join to `t` (Caused by:
integration/arraynumkey/test.php +53) (Originally at:
integration/arraynumkey/test.php:57)
integration/arraynumkey/test.php:80 SecurityCheck-SQLInjection HAVING clause
is user controlled (Originally at: integration/arraynumkey/test.php:78)
+integration/arraynumkey/test.php:109 SecurityCheck-XSS Echoing expression that
was not html escaped (Caused by: integration/arraynumkey/test.php +92)
diff --git a/tests/integration/arraynumkey/test.php
b/tests/integration/arraynumkey/test.php
index e5dace8..f0dd600 100644
--- a/tests/integration/arraynumkey/test.php
+++ b/tests/integration/arraynumkey/test.php
@@ -83,3 +83,27 @@
$b = (int)$_GET['b'];
$db->select( 't', 'f', [ 'foo' => $_GET['a'], "bar > $b" ] );
+
+$row = (object)[ 'foo' => $_GET['bar'] ];
+$db->selectRow( 't', 'f', [ 'foo2' => $row->foo ] );
+$whereRow = [ 'foo2' => $row->foo ];
+$db->selectRow( 't', 'f', $whereRow );
+
+$subquery = $db->selectSQLText(
+ 'Foo',
+ '1',
+ [ 'value' => $_GET['val'], 'baz=red', 'foo' => '<script>' ],
+ __METHOD__,
+ [ 'LIMIT' => 1 ]
+);
+
+// safe
+$db->select(
+ 'Bar',
+ '*',
+ [ 'NOT EXISTS( ' . $subquery . ')' ],
+ __METHOD__
+);
+
+// unsafe
+echo $subquery;
diff --git a/tests/integration/lists/test.php b/tests/integration/lists/test.php
index c1bbbac..beb8b6d 100644
--- a/tests/integration/lists/test.php
+++ b/tests/integration/lists/test.php
@@ -6,3 +6,10 @@
list( $foo ) = getStuff();
echo $foo;
+
+function getSafeStuff() {
+ return [ 'a', 'b' ];
+}
+
+list( $a, $b ) = getSafeStuff();
+echo $b;
diff --git a/tests/integration/lowercase/OutputPage.php
b/tests/integration/lowercase/OutputPage.php
new file mode 100644
index 0000000..9d54903
--- /dev/null
+++ b/tests/integration/lowercase/OutputPage.php
@@ -0,0 +1,5 @@
+<?php
+class OutputPage {
+ public function addHTML( $html ) {
+ }
+}
diff --git a/tests/integration/lowercase/expectedResults.txt
b/tests/integration/lowercase/expectedResults.txt
new file mode 100644
index 0000000..d2a2014
--- /dev/null
+++ b/tests/integration/lowercase/expectedResults.txt
@@ -0,0 +1,4 @@
+integration/lowercase/test.php:5 SecurityCheck-XSS Calling method
\OutputPage::addHTML() in [no method] that outputs using tainted argument $[arg
#1].
+integration/lowercase/test.php:6 SecurityCheck-XSS Calling method
\OutputPage::addHTML() in [no method] that outputs using tainted argument $[arg
#1].
+integration/lowercase/test.php:7 SecurityCheck-XSS Calling method
\OutputPage::addHTML() in [no method] that outputs using tainted argument $[arg
#1].
+integration/lowercase/test.php:8 SecurityCheck-XSS Calling method
\OutputPage::addHTML() in [no method] that outputs using tainted argument $[arg
#1].
diff --git a/tests/integration/lowercase/test.php
b/tests/integration/lowercase/test.php
new file mode 100644
index 0000000..a510c65
--- /dev/null
+++ b/tests/integration/lowercase/test.php
@@ -0,0 +1,8 @@
+<?php
+
+$out = new OutputPage;
+
+$out->addHTML( $_GET['evil'] );
+$out->addHtml( $_GET['evil'] );
+$out->addhtml( $_GET['evil'] );
+$out->ADDHTML( $_GET['evil'] );
diff --git a/tests/integration/ooui/HorizontalLayout.php
b/tests/integration/ooui/HorizontalLayout.php
new file mode 100644
index 0000000..190955d
--- /dev/null
+++ b/tests/integration/ooui/HorizontalLayout.php
@@ -0,0 +1,18 @@
+<?php
+namespace OOUI;
+
+class Layout {
+
+ protected $contents;
+ public function __toString() {
+ return $this->contents;
+ }
+
+}
+
+class HorizontalLayout extends Layout {
+ public function __construct( $contents ) {
+ $this->contents = $contents;
+ }
+
+}
diff --git a/tests/integration/ooui/OutputPage.php
b/tests/integration/ooui/OutputPage.php
new file mode 100644
index 0000000..9d54903
--- /dev/null
+++ b/tests/integration/ooui/OutputPage.php
@@ -0,0 +1,5 @@
+<?php
+class OutputPage {
+ public function addHTML( $html ) {
+ }
+}
diff --git a/tests/integration/ooui/expectedResults.txt
b/tests/integration/ooui/expectedResults.txt
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/tests/integration/ooui/expectedResults.txt
diff --git a/tests/integration/ooui/test.php b/tests/integration/ooui/test.php
new file mode 100644
index 0000000..2b0577e
--- /dev/null
+++ b/tests/integration/ooui/test.php
@@ -0,0 +1,9 @@
+<?php
+
+$out = new OutputPage;
+
+$stuff = new OOUI\HorizontalLayout( "More stuff" );
+$stuff2 = new OOUI\HorizontalLayout( $_GET['stuff'] );
+
+$out->addHtml( "<span>" . $stuff . "</span>" );
+$out->addHtml( "<span>" . $stuff2 . "</span>" );
--
To view, visit https://gerrit.wikimedia.org/r/395762
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I3bcbd5bbfa7417c490adb2ec7b580e13eaa70741
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
