Brian Wolff has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/393539 )
Change subject: Add taint for db related function. Fix handling of subclasses
......................................................................
Add taint for db related function. Fix handling of subclasses
Change-Id: I8b8c6a4350ebb605216411124631984c8bdaf54c
---
M .phpcs.xml
M src/MediaWikiSecurityCheckPlugin.php
M src/SecurityCheckPlugin.php
M src/TaintednessBaseVisitor.php
A tests/integration/subclasses/db.php
A tests/integration/subclasses/expectedResults.txt
A tests/integration/subclasses/test.php
M tests/runtests.sh
8 files changed, 149 insertions(+), 2 deletions(-)
diff --git a/.phpcs.xml b/.phpcs.xml
index d5331bd..60518ae 100644
--- a/.phpcs.xml
+++ b/.phpcs.xml
@@ -26,4 +26,7 @@
<rule ref="MediaWiki.Files.ClassMatchesFilename.NotMatch">
<exclude-pattern>./tests</exclude-pattern>
</rule>
+ <rule ref="MediaWiki.Files.OneClassPerFile.MultipleFound">
+ <exclude-pattern>./tests</exclude-pattern>
+ </rule>
</ruleset>
diff --git a/src/MediaWikiSecurityCheckPlugin.php
b/src/MediaWikiSecurityCheckPlugin.php
index 75d5166..232ecfe 100644
--- a/src/MediaWikiSecurityCheckPlugin.php
+++ b/src/MediaWikiSecurityCheckPlugin.php
@@ -11,6 +11,51 @@
*/
protected function getCustomFuncTaints() : array {
return [
+ // Note, at the moment, this checks where the function
+ // is implemented, so you can't use IDatabase.
+ '\Wikimedia\Rdbms\Database::query' => [
+ self::SQL_EXEC_TAINT,
+ // What should DB results be considered?
+ 'overall' => self::YES_TAINT
+ ],
+ '\Wikimedia\Rdbms\IDatabase::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\Database::addQuotes' => [
+ self::YES_TAINT & ~self::SQL_TAINT,
+ 'overall' => self::NO_TAINT,
+ ],
+ '\Wikimedia\Rdbms\DBConnRef::addQuotes' => [
+ self::YES_TAINT & ~self::SQL_TAINT,
+ 'overall' => self::NO_TAINT,
+ ],
+ '\Wikimedia\Rdbms\DatabaseMysqlBase::addQuotes' => [
+ self::YES_TAINT & ~self::SQL_TAINT,
+ 'overall' => self::NO_TAINT,
+ ],
+ '\Wikimedia\Rdbms\DatabaseMssql::addQuotes' => [
+ self::YES_TAINT & ~self::SQL_TAINT,
+ 'overall' => self::NO_TAINT,
+ ],
+ '\Wikimedia\Rdbms\IDatabase::addQuotes' => [
+ self::YES_TAINT & ~self::SQL_TAINT,
+ 'overall' => self::NO_TAINT,
+ ],
+ '\Wikimedia\Rdbms\DatabasePostgres::addQuotes' => [
+ self::YES_TAINT & ~self::SQL_TAINT,
+ 'overall' => self::NO_TAINT,
+ ],
+ '\Wikimedia\Rdbms\DatabaseSqlite::addQuotes' => [
+ self::YES_TAINT & ~self::SQL_TAINT,
+ 'overall' => self::NO_TAINT,
+ ],
// '\Message::__construct' =>
SecurityCheckPlugin::YES_TAINT,
// '\wfMessage' => SecurityCheckPlugin::YES_TAINT,
'\Message::plain' => [ 'overall' =>
SecurityCheckPlugin::YES_TAINT, ],
diff --git a/src/SecurityCheckPlugin.php b/src/SecurityCheckPlugin.php
index 94d6b5b..e566975 100644
--- a/src/SecurityCheckPlugin.php
+++ b/src/SecurityCheckPlugin.php
@@ -152,6 +152,46 @@
self::SERIALIZE_EXEC_TAINT,
'overall' => self::NO_TAINT,
],
+ '\mysql_query' => [
+ self::SQL_EXEC_TAINT,
+ 'overall' => self::UNKNOWN_TAINT
+ ],
+ '\mysqli_query' => [
+ self::NO_TAINT,
+ self::SQL_EXEC_TAINT,
+ 'overall' => self::UNKNOWN_TAINT
+ ],
+ '\mysqli::query' => [
+ self::SQL_EXEC_TAINT,
+ 'overall' => self::UNKNOWN_TAINT
+ ],
+ '\mysqli_real_query' => [
+ self::NO_TAINT,
+ self::SQL_EXEC_TAINT,
+ 'overall' => self::UNKNOWN_TAINT
+ ],
+ '\mysqli::real_query' => [
+ self::SQL_EXEC_TAINT,
+ 'overall' => self::UNKNOWN_TAINT
+ ],
+ '\mysqli_escape_string' => [
+ self::NO_TAINT,
+ self::YES_TAINT & ~self::SQL_TAINT,
+ 'overall' => self::NO_TAINT
+ ],
+ '\mysqli_real_escape_string' => [
+ self::NO_TAINT,
+ self::YES_TAINT & ~self::SQL_TAINT,
+ 'overall' => self::NO_TAINT
+ ],
+ '\mysqli::escape_string' => [
+ self::YES_TAINT & ~self::SQL_TAINT,
+ 'overall' => self::NO_TAINT
+ ],
+ '\mysqli::real_escape_string' => [
+ self::YES_TAINT & ~self::SQL_TAINT,
+ 'overall' => self::NO_TAINT
+ ],
];
}
}
diff --git a/src/TaintednessBaseVisitor.php b/src/TaintednessBaseVisitor.php
index d04cd3b..58195b7 100644
--- a/src/TaintednessBaseVisitor.php
+++ b/src/TaintednessBaseVisitor.php
@@ -7,6 +7,7 @@
use Phan\Language\Element\FunctionInterface;
use Phan\Language\Element\Variable;
use Phan\Language\Element\TypedElementInterface;
+use Phan\Language\Element\ClassElement;
use Phan\Language\UnionType;
use Phan\Language\FQSEN\FullyQualifiedFunctionLikeName;
use Phan\Plugin;
@@ -49,11 +50,22 @@
* @param FunctionInterface $func
* @param int[] $taint Numeric keys for each arg and an 'overall' key.
* @param bool $override Whether to merge taint or override
+ * @suppress PhanUndeclaredMethod
*/
protected function setFuncTaint( FunctionInterface $func, array $taint,
bool $override = false ) {
+ if (
+ $func instanceof ClassElement &&
+ (string)$func->getDefiningFQSEN() !==
(string)$func->getFQSEN()
+ ) {
+ $this->debug( __METHOD__, "Setting taint on function "
. $func->getFQSEN() . " other than"
+ . " its implementation " .
$func->getDefiningFQSEN()
+ );
+ // FIXME we should maybe do something here.
+ // As it stands, this case probably can't be reached.
+ }
$curTaint = [];
$newTaint = [];
- // What taint we're setting, to do bookkeeping about whenever
+ // What taint we're setting, to do book-keeping about whenever
// we add a dangerous taint.
$mergedTaint = SecurityCheckPlugin::NO_TAINT;
if ( property_exists( $func, 'funcTaint' ) ) {
@@ -245,6 +257,7 @@
* TAINT flags for what taint gets passed through
func.
* If func has an arg that is mssing from array, then it should be
* treated as TAINT_NO if its a number or bool. TAINT_YES otherwise.
+ * @suppress PhanUndeclaredMethod
*/
protected function getTaintOfFunction( FunctionInterface $func ) {
$funcName = $func->getFQSEN();
@@ -261,6 +274,20 @@
}
if ( property_exists( $func, 'funcTaint' ) ) {
$taint = $func->funcTaint;
+ } elseif (
+ $func instanceof ClassElement
+ && $func->hasDefiningFQSEN()
+ && (string)$func->getDefiningFQSEN() !==
(string)$func->getFQSEN()
+ ) {
+ // @todo Should we check this earlier. Should we skip
right to
+ // the taint of the defining method, and not even look
at
+ // whatever the method is called as?
+ // In the case of builtin taints, perhaps it should
still look
+ // at base classes even if the base class got
overriden(?)
+ $definingFqsen = $func->getDefiningFQSEN();
+ $definingFunc = $this->code_base->getMethodByFQSEN(
$definingFqsen );
+ $this->debug( __METHOD__, "Checking base implemntation
$definingFqsen of $funcName" );
+ return $this->getTaintOfFunction( $definingFunc );
} else {
// Ensure we don't indef loop.
if (
diff --git a/tests/integration/subclasses/db.php
b/tests/integration/subclasses/db.php
new file mode 100644
index 0000000..517e4e2
--- /dev/null
+++ b/tests/integration/subclasses/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 ) {
+ // do some stuff
+ return (object)[ 'some_field' => 'some value' ];
+ }
+
+}
+
+class MysqlDatabase extends Database {
+ public function getType() {
+ return 'mysql';
+ }
+}
diff --git a/tests/integration/subclasses/expectedResults.txt
b/tests/integration/subclasses/expectedResults.txt
new file mode 100644
index 0000000..e43c804
--- /dev/null
+++ b/tests/integration/subclasses/expectedResults.txt
@@ -0,0 +1 @@
+integration/subclasses/test.php:10 SecurityCheckTaintedOutput Calling Method
\Wikimedia\Rdbms\MysqlDatabase::query in [no method] that outputs using tainted
(680; 64) argument $myQuery. (Caused by: integration/subclasses/test.php +8;
integration/subclasses/test.php +7)
diff --git a/tests/integration/subclasses/test.php
b/tests/integration/subclasses/test.php
new file mode 100644
index 0000000..e13ee6c
--- /dev/null
+++ b/tests/integration/subclasses/test.php
@@ -0,0 +1,10 @@
+<?php
+
+use Wikimedia\Rdbms\MysqlDatabase;
+
+$db = new MysqlDatabase;
+
+$name = $_GET['name'];
+$myQuery = "Select * from foo where name = '$name' LIMIT 3;";
+
+$db->query( $myQuery, "query" );
diff --git a/tests/runtests.sh b/tests/runtests.sh
index 5a32398..d5bcd5c 100755
--- a/tests/runtests.sh
+++ b/tests/runtests.sh
@@ -12,6 +12,7 @@
failedTests=0
php=`which php7.0`
php=${php:-`which php`}
+SECCHECK_DEBUG=${SECCHECK_DEBUG:-/dev/null}
for i in $testList
do
@@ -21,7 +22,7 @@
--project-root-directory "." \
--config-file "integration-test-config.php" \
--output "php://stdout" \
- -l "integration/$i" | grep SecurityCheckTaintedOutput >
$tmpFile
+ -l "integration/$i" | tee "$SECCHECK_DEBUG" | grep
SecurityCheckTaintedOutput > $tmpFile
diff -u "integration/$i/expectedResults.txt" "$tmpFile"
if [ $? -gt 0 ]
then failedTests=$((failedTests+1))
--
To view, visit https://gerrit.wikimedia.org/r/393539
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I8b8c6a4350ebb605216411124631984c8bdaf54c
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/tools/phan/SecurityCheckPlugin
Gerrit-Branch: master
Gerrit-Owner: Brian Wolff <[email protected]>
Gerrit-Reviewer: Brian Wolff <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
