Brian Wolff has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/394870 )
Change subject: Add support for IDatabase::select style arguments
......................................................................
Add support for IDatabase::select style arguments
Change-Id: Id52f45e3dfc21a4c522902a8d4d92e61b40b7202
---
M src/MediaWikiSecurityCheckPlugin.php
M src/TaintednessBaseVisitor.php
M src/TaintednessVisitor.php
A tests/integration/arraynumkey/db.php
A tests/integration/arraynumkey/expectedResults.txt
A tests/integration/arraynumkey/test.php
A tests/integration/multiassign/expectedResults.txt
A tests/integration/multiassign/test.php
A tests/integration/multiassign/test2.php
9 files changed, 289 insertions(+), 1 deletion(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/tools/phan/SecurityCheckPlugin
refs/changes/70/394870/1
diff --git a/src/MediaWikiSecurityCheckPlugin.php
b/src/MediaWikiSecurityCheckPlugin.php
index d17ebfa..2c25a07 100644
--- a/src/MediaWikiSecurityCheckPlugin.php
+++ b/src/MediaWikiSecurityCheckPlugin.php
@@ -76,6 +76,42 @@
// What should DB results be considered?
'overall' => self::YES_TAINT
],
+ '\Wikimedia\Rdbms\IDatabase::select' => [
+ self::SQL_EXEC_TAINT,
+ self::SQL_EXEC_TAINT,
+ self::SQL_NUMKEY_EXEC_TAINT,
+ // the function name doesn't seem to be escaped
+ self::SQL_EXEC_TAINT,
+ // I'm not even sure for options
+ self::SQL_EXEC_TAINT,
+ self::SQL_NUMKEY_EXEC_TAINT,
+ // What should DB results be considered?
+ 'overall' => self::YES_TAINT
+ ],
+ '\Wikimedia\Rdbms\Database::select' => [
+ self::SQL_EXEC_TAINT,
+ self::SQL_EXEC_TAINT,
+ self::SQL_NUMKEY_EXEC_TAINT,
+ // the function name doesn't seem to be escaped
+ self::SQL_EXEC_TAINT,
+ // I'm not even sure for options
+ self::SQL_EXEC_TAINT,
+ self::SQL_NUMKEY_EXEC_TAINT,
+ // What should DB results be considered?
+ 'overall' => self::YES_TAINT
+ ],
+ '\Wikimedia\Rdbms\DBConnRef::select' => [
+ self::SQL_EXEC_TAINT,
+ self::SQL_EXEC_TAINT,
+ self::SQL_NUMKEY_EXEC_TAINT,
+ // the function name doesn't seem to be escaped
+ self::SQL_EXEC_TAINT,
+ // I'm not even sure for options
+ self::SQL_EXEC_TAINT,
+ self::SQL_NUMKEY_EXEC_TAINT,
+ // What should DB results be considered?
+ 'overall' => self::YES_TAINT
+ ],
'\Wikimedia\Rdbms\Database::addIdentifierQuotes' => [
self::YES_TAINT & ~self::SQL_TAINT,
'overall' => self::NO_TAINT,
diff --git a/src/TaintednessBaseVisitor.php b/src/TaintednessBaseVisitor.php
index 89a47b1..4a01337 100644
--- a/src/TaintednessBaseVisitor.php
+++ b/src/TaintednessBaseVisitor.php
@@ -12,6 +12,7 @@
use Phan\Language\UnionType;
use Phan\Language\Type\CallableType;
use Phan\Language\Type\MixedType;
+use Phan\Language\Type\IntType;
use Phan\Language\Type\StringType;
use Phan\Language\FQSEN\FullyQualifiedFunctionLikeName;
use Phan\Language\FQSEN\FullyQualifiedFunctionName;
@@ -1359,7 +1360,12 @@
) {
$issueType = 'SecurityCheck-XSS';
} elseif (
- $combinedTaint === SecurityCheckPlugin::SQL_TAINT
+ $combinedTaint === SecurityCheckPlugin::SQL_TAINT ||
+ $combinedTaint ===
SecurityCheckPlugin::SQL_NUMKEY_TAINT ||
+ $combinedTaint === (
+ SecurityCheckPlugin::SQL_TAINT |
+ SecurityCheckPlugin::SQL_NUMKEY_TAINT
+ )
) {
$issueType = 'SecurityCheck-SQLInjection';
$severity = Issue::SEVERITY_CRITICAL;
@@ -1459,6 +1465,16 @@
$curArgTaintedness =
SecurityCheckPlugin::NO_TAINT;
$effectiveArgTaintedness =
SecurityCheckPlugin::NO_TAINT;
} elseif ( isset( $taint[$i] ) ) {
+ if (
+ ( $taint[$i] &
SecurityCheckPlugin::SQL_NUMKEY_EXEC_TAINT )
+ && !$this->nodeIsArray( $argument )
+ && ( $curArgTaintedness &
SecurityCheckPlugin::SQL_TAINT )
+ ) {
+ // Special case to make NUMKEY work
right for non-array
+ // values. Should consider if this is
really best
+ // approach.
+ $curArgTaintedness |=
SecurityCheckPlugin::SQL_NUMKEY_TAINT;
+ }
$effectiveArgTaintedness = $curArgTaintedness &
( $taint[$i] | $this->execToYesTaint(
$taint[$i] ) );
// $this->debug( __METHOD__, "effective
$effectiveArgTaintedness"
@@ -1636,4 +1652,39 @@
}
return false;
}
+
+ /**
+ * Given a Node, is it definitely an int (and nothing else)
+ *
+ * Floats are not considered ints here.
+ *
+ * @param Mixed|Node $node A node object or simple value from AST tree
+ * @return bool Is it an int?
+ */
+ protected function nodeIsInt( $node ) : bool {
+ if ( is_int( $node ) ) {
+ return true;
+ }
+ if ( !( $node instanceof Node ) ) {
+ // simple literal that's not an int.
+ return false;
+ }
+ try {
+ $type = UnionTypeVisitor::unionTypeFromNode(
+ $this->code_base,
+ $this->context,
+ $node
+ );
+ if (
+ $type->hasType( IntType::instance() ) &&
+ $type->typeCount() === 1
+ ) {
+ return true;
+ }
+ } catch ( Exception $e ) {
+ $this->debug( __METHOD__, "Got error " . get_class( $e
) );
+ }
+ return false;
+ }
+
}
diff --git a/src/TaintednessVisitor.php b/src/TaintednessVisitor.php
index d4b588d..2d45a58 100644
--- a/src/TaintednessVisitor.php
+++ b/src/TaintednessVisitor.php
@@ -287,6 +287,7 @@
/**
* Also handles visitAssignOp
*
+ * @todo FIXME, doesn't handle list( $a, $b ) = $foo;
* @param Node $node
* @return int Taint
*/
@@ -319,6 +320,26 @@
// TODO, be more specific for different OPs
// Expand rhs to include implicit lhs ophand.
$rhsTaintedness = $this->mergeAddTaint(
$rhsTaintedness, $lhsTaintedness );
+ }
+
+ // Special case for SQL_NUMKEY_TAINT
+ // If we're assigning an SQL tainted value as an array key
+ // or as the value of a numeric key, then set NUMKEY taint.
+ $var = $node->children['var'];
+ if ( $var->kind === \ast\AST_DIM ) {
+ $dim = $var->children['dim'];
+ if ( $rhsTaintedness & SecurityCheckPlugin::SQL_TAINT )
{
+ if (
+ $dim === null ||
+ $this->nodeIsInt( $dim )
+ ) {
+ $rhsTaintedness |=
SecurityCheckPlugin::SQL_NUMKEY_TAINT;
+ }
+ }
+ if ( $this->getTaintedness( $dim ) &
SecurityCheckPlugin::SQL_TAINT ) {
+ $rhsTaintedness |=
SecurityCheckPlugin::SQL_NUMKEY_TAINT;
+
+ }
}
// If we're assigning to a variable we know will be output later
@@ -739,6 +760,16 @@
foreach ( $node->children as $child ) {
assert( $child->kind === \ast\AST_ARRAY_ELEM );
$curTaint = $this->mergeAddTaint( $curTaint,
$this->getTaintedness( $child ) );
+ $key = $child->children['key'];
+ $value = $child->children['value'];
+ $sqlTaint = SecurityCheckPlugin::SQL_TAINT;
+ if (
+ ( $this->getTaintedness( $key ) & $sqlTaint ) ||
+ ( ( $key === null || $this->nodeIsInt( $key ) )
+ && ( $this->getTaintedness( $value ) &
$sqlTaint ) )
+ ) {
+ $curTaint |=
SecurityCheckPlugin::SQL_NUMKEY_TAINT;
+ }
}
return $curTaint;
}
diff --git a/tests/integration/arraynumkey/db.php
b/tests/integration/arraynumkey/db.php
new file mode 100644
index 0000000..c607198
--- /dev/null
+++ b/tests/integration/arraynumkey/db.php
@@ -0,0 +1,26 @@
+<?php
+namespace Wikimedia\Rdbms;
+
+interface IDatabase {
+ public function query( $sql, $method );
+}
+
+class Database implements IDatabase {
+ public function query( $sql, $method ) {
+ // do some stuff
+ return (object)[ 'some_field' => 'some value' ];
+ }
+
+ public function select(
+ $table, $vars, $conds = '', $fname = __METHOD__,
+ $options = [], $join_conds = []
+ ) {
+ return (object)[ 'some_field' => 'some value' ];
+ }
+}
+
+class MysqlDatabase extends Database {
+ public function getType() {
+ return 'mysql';
+ }
+}
diff --git a/tests/integration/arraynumkey/expectedResults.txt
b/tests/integration/arraynumkey/expectedResults.txt
new file mode 100644
index 0000000..ec727f1
--- /dev/null
+++ b/tests/integration/arraynumkey/expectedResults.txt
@@ -0,0 +1,7 @@
+integration/arraynumkey/test.php:15 SecurityCheck-SQLInjection Calling method
\Wikimedia\Rdbms\MysqlDatabase::select() in [no method] that outputs using
tainted argument $[arg #3].
+integration/arraynumkey/test.php:22 SecurityCheck-SQLInjection Calling method
\Wikimedia\Rdbms\MysqlDatabase::select() in [no method] that outputs using
tainted argument $[arg #3].
+integration/arraynumkey/test.php:32 SecurityCheck-SQLInjection Calling method
\Wikimedia\Rdbms\MysqlDatabase::select() in [no method] that outputs using
tainted argument $where. (Caused by: integration/arraynumkey/test.php +29)
+integration/arraynumkey/test.php:41 SecurityCheck-SQLInjection Calling method
\Wikimedia\Rdbms\MysqlDatabase::select() in [no method] that outputs using
tainted argument $where2. (Caused by: integration/arraynumkey/test.php +38)
+integration/arraynumkey/test.php:45 SecurityCheck-SQLInjection Calling method
\Wikimedia\Rdbms\MysqlDatabase::select() in [no method] that outputs using
tainted argument $where3. (Caused by: integration/arraynumkey/test.php +44)
+integration/arraynumkey/test.php:50 SecurityCheck-SQLInjection Calling method
\Wikimedia\Rdbms\MysqlDatabase::select() in [no method] that outputs using
tainted argument $where4.
+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)
diff --git a/tests/integration/arraynumkey/test.php
b/tests/integration/arraynumkey/test.php
new file mode 100644
index 0000000..ca63c55
--- /dev/null
+++ b/tests/integration/arraynumkey/test.php
@@ -0,0 +1,54 @@
+<?php
+
+use Wikimedia\Rdbms\MysqlDatabase;
+
+$db = new MysqlDatabase;
+
+// safe
+$db->select(
+ 'table',
+ 'field',
+ [ 'foo' => $_GET['bar'] ]
+);
+
+// unsafe
+$db->select(
+ 'table',
+ 'field',
+ [ $_GET['bar'] ]
+);
+
+// unsafe
+$db->select(
+ 'table',
+ 'field',
+ $_GET['bar']
+);
+
+$where = [];
+$where[] = $_GET['bar'];
+
+// unsafe
+$db->select(
+ 'table',
+ 'field',
+ $where
+);
+
+$where2 = [ $_GET['bar'] ];
+$where2[] = 'Something';
+// unsafe
+$db->select( 't', 'f', $where2 );
+
+// unsafe
+$where3 = [ $_GET['d'] => 'foo' ];
+$db->select( 't', 'f', $where3 );
+
+$where4 = [];
+$where4[$_GET['d']] = 'foo';
+// unsafe
+$db->select( 't', 'f', $where4 );
+
+// unsafe
+$where5 = [ 1 => $_GET['d'] ];
+$db->select( 't', 'f', $where5 );
diff --git a/tests/integration/multiassign/expectedResults.txt
b/tests/integration/multiassign/expectedResults.txt
new file mode 100644
index 0000000..1f2f885
--- /dev/null
+++ b/tests/integration/multiassign/expectedResults.txt
@@ -0,0 +1,5 @@
+integration/multiassign/test.php:16 SecurityCheck-XSS Echoing expression that
was not html escaped (Caused by: integration/multiassign/test.php +12)
+integration/multiassign/test2.php:20 SecurityCheck-XSS Echoing expression that
was not html escaped (Caused by: integration/multiassign/test2.php +12)
+integration/multiassign/test2.php:39 SecurityCheck-XSS Echoing expression that
was not html escaped (Caused by: integration/multiassign/test2.php +12)
+integration/multiassign/test2.php:41 SecurityCheck-XSS Echoing expression that
was not html escaped
+integration/multiassign/test2.php:54 SecurityCheck-XSS Echoing expression that
was not html escaped (Caused by: integration/multiassign/test2.php +51)
diff --git a/tests/integration/multiassign/test.php
b/tests/integration/multiassign/test.php
new file mode 100644
index 0000000..5e6e2f1
--- /dev/null
+++ b/tests/integration/multiassign/test.php
@@ -0,0 +1,17 @@
+<?php
+
+function test() {
+ $a = $_GET['d'];
+
+ $a = 'ok';
+
+ echo $a;
+}
+
+function test2() {
+ $b = $_GET['d'];
+ if ( false ) {
+ $b = 'ok';
+ }
+ echo $b;
+}
diff --git a/tests/integration/multiassign/test2.php
b/tests/integration/multiassign/test2.php
new file mode 100644
index 0000000..f0c1d8e
--- /dev/null
+++ b/tests/integration/multiassign/test2.php
@@ -0,0 +1,61 @@
+<?php
+
+$globalFoo = '';
+$globalBar = '';
+
+class Foo {
+ private $bar;
+ private $bar2;
+
+ public function setEvil() {
+ global $globalFoo;
+ $this->bar2 = $_GET['evil'];
+ $globalFoo = $_GET['evil'];
+ // Note: intentional lack of global decleration.
+ $globalBar = $_GET['evil'];
+ }
+
+ public function echoThing() {
+ // Should give warning
+ echo $this->bar2;
+ }
+
+ public function setGood() {
+ global $globalFoo, $globalBar;
+ $this->bar2 = 'good';
+ $globalFoo = 'good';
+ $globalBar = 'good';
+ }
+
+ /**
+ * This should trigger a warning, since
+ * there is no garuntee that setGood()
+ * was called before this despite it being
+ * earlier in the source file
+ */
+ public function echoThing() {
+ global $globalFoo, $globalBar;
+ // Should give warning
+ echo $this->bar2;
+ // Should give warning
+ echo $globalFoo;
+ // No warning
+ echo $globalBar;
+ }
+
+ public function f1() {
+ // At the moment, this triggers a warning
+ // Because we are not smart enough to know
+ // if any non-local functions have modified
+ // $this->bar in between setting it to something good.
+ $this->bar = $_GET['tainted'];
+ $this->bar = 'ok';
+ // Should give warning
+ echo $this->bar;
+ }
+}
+
+$f = new Foo;
+
+$f->setEvil();
+$f->echoThing();
--
To view, visit https://gerrit.wikimedia.org/r/394870
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id52f45e3dfc21a4c522902a8d4d92e61b40b7202
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/tools/phan/SecurityCheckPlugin
Gerrit-Branch: master
Gerrit-Owner: Brian Wolff <bawolff...@gmail.com>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
