Brian Wolff has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/395761 )
Change subject: Fix various false positives found when testing with MW
......................................................................
Fix various false positives found when testing with MW
Change-Id: I17f0dbd83c822faa0bcaa823f5adf15d6366d3f4
---
M src/MediaWikiSecurityCheckPlugin.php
M src/TaintednessBaseVisitor.php
M src/TaintednessVisitor.php
M tests/integration/arraynumkey/test.php
M tests/integration/cast/expectedResults.txt
M tests/integration/cast/test.php
M tests/integration/dbarrayplus/test.php
A tests/integration/prop2/expectedResults.txt
A tests/integration/prop2/test.php
9 files changed, 103 insertions(+), 10 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/tools/phan/SecurityCheckPlugin
refs/changes/61/395761/1
diff --git a/src/MediaWikiSecurityCheckPlugin.php
b/src/MediaWikiSecurityCheckPlugin.php
index e67f6d9..0fa770d 100644
--- a/src/MediaWikiSecurityCheckPlugin.php
+++ b/src/MediaWikiSecurityCheckPlugin.php
@@ -131,8 +131,8 @@
],
'\Wikimedia\Rdbms\IDatabase::insert' => [
self::SQL_EXEC_TAINT, // table name
- // Insert values. The keys names are unsafe.
- // Unclear how well this works for the multi
case.
+ // 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
@@ -362,12 +362,28 @@
self::NO_TAINT,
'overall' => SecurityCheckPlugin::NO_TAINT,
],
+ '\ParserOutput::getText' => [
+ self::NO_TAINT,
+ 'overall' => SecurityCheckPlugin::NO_TAINT,
+ ],
'\Sanitizer::removeHTMLtags' => [
self::YES_TAINT & ~self::HTML_TAINT, /* text */
self::SHELL_EXEC_TAINT, /* attribute callback */
self::NO_TAINT, /* callback args */
self::YES_TAINT, /* extra tags */
self::NO_TAINT, /* remove tags */
+ 'overall' => SecurityCheckPlugin::NO_TAINT
+ ],
+ '\Sanitizer::escapeHtmlAllowEntities' => [
+ self::YES_TAINT & ~self::HTML_TAINT,
+ 'overall' => SecurityCheckPlugin::NO_TAINT
+ ],
+ '\Sanitizer::safeEncodeAttribute' => [
+ self::YES_TAINT & ~self::HTML_TAINT,
+ 'overall' => SecurityCheckPlugin::NO_TAINT
+ ],
+ '\Sanitizer::encodeAttribute' => [
+ self::YES_TAINT & ~self::HTML_TAINT,
'overall' => SecurityCheckPlugin::NO_TAINT
],
'\WebRequest::getGPCVal' => [ 'overall' =>
SecurityCheckPlugin::YES_TAINT, ],
@@ -409,10 +425,24 @@
'OOUI\ButtonInputWidget::__construct' => [
'overall' => self::NO_TAINT
],
+ 'OOUI\HorizontalLayout::__construct' => [
+ 'overall' => self::NO_TAINT
+ ],
'HtmlArmor::__construct' => [
self::HTML_EXEC_TAINT,
'overall' => self::NO_TAINT
],
+ // Due to limitations in how we handle list()
+ // elements, hard code CommentStore stuff.
+ 'CommentStore::insert' => [
+ 'overall' => self::NO_TAINT
+ ],
+ 'CommentStore::getJoin' => [
+ 'overall' => self::NO_TAINT
+ ],
+ 'CommentStore::insertWithTempTable' => [
+ 'overall' => self::NO_TAINT
+ ],
];
}
diff --git a/src/TaintednessBaseVisitor.php b/src/TaintednessBaseVisitor.php
index 40c644a..26acefa 100644
--- a/src/TaintednessBaseVisitor.php
+++ b/src/TaintednessBaseVisitor.php
@@ -335,6 +335,10 @@
// Built in php.
// Assume that anything really dangerous we've already
// hardcoded. So just preserve taint
+ $taintFromReturnType = $this->getTaintByReturnType(
$func->getUnionType() );
+ if ( $taintFromReturnType ===
SecurityCheckPlugin::NO_TAINT ) {
+ return [ 'overall' =>
SecurityCheckPlugin::NO_TAINT ];
+ }
return [ 'overall' =>
SecurityCheckPlugin::PRESERVE_TAINT ];
}
if ( property_exists( $func, 'funcTaint' ) ) {
diff --git a/src/TaintednessVisitor.php b/src/TaintednessVisitor.php
index 9325058..db0b046 100644
--- a/src/TaintednessVisitor.php
+++ b/src/TaintednessVisitor.php
@@ -336,8 +336,9 @@
$rhsTaintedness &=
~SecurityCheckPlugin::SQL_NUMKEY_TAINT;
} elseif ( $rhsTaintedness &
SecurityCheckPlugin::SQL_TAINT ) {
if (
- $dim === null ||
- $this->nodeIsInt( $dim )
+ ( $dim === null ||
+ $this->nodeIsInt( $dim ) )
+ && !$this->nodeIsArray(
$node->children['expr'] )
) {
$rhsTaintedness |=
SecurityCheckPlugin::SQL_NUMKEY_TAINT;
}
@@ -768,7 +769,7 @@
$curTaint = SecurityCheckPlugin::NO_TAINT;
foreach ( $node->children as $child ) {
assert( $child->kind === \ast\AST_ARRAY_ELEM );
- $curTaint = $this->mergeAddTaint( $curTaint,
$this->getTaintedness( $child ) );
+ $childTaint = $this->getTaintedness( $child );
$key = $child->children['key'];
$value = $child->children['value'];
$sqlTaint = SecurityCheckPlugin::SQL_TAINT;
@@ -776,15 +777,17 @@
$this->getTaintedness( $value )
& SecurityCheckPlugin::SQL_NUMKEY_TAINT
) {
- $curTaint &=
~SecurityCheckPlugin::SQL_NUMKEY_TAINT;
+ $childTaint &=
~SecurityCheckPlugin::SQL_NUMKEY_TAINT;
} elseif (
( $this->getTaintedness( $key ) & $sqlTaint ) ||
( ( $key === null || $this->nodeIsInt( $key ) )
- && ( $this->getTaintedness( $value ) &
$sqlTaint ) )
+ && ( $this->getTaintedness( $value ) &
$sqlTaint )
+ && !$this->nodeIsArray( $value ) )
) {
- $curTaint |=
SecurityCheckPlugin::SQL_NUMKEY_TAINT;
+ $childTaint |=
SecurityCheckPlugin::SQL_NUMKEY_TAINT;
}
+ $curTaint = $this->mergeAddTaint( $curTaint,
$childTaint );
}
return $curTaint;
}
@@ -1005,7 +1008,7 @@
ast\flags\TYPE_OBJECT
];
- if ( in_array( $node->flags, $dangerousCasts ) ) {
+ if ( !in_array( $node->flags, $dangerousCasts ) ) {
return SecurityCheckPlugin::NO_TAINT;
}
return $this->getTaintedness( $node->children['expr'] );
diff --git a/tests/integration/arraynumkey/test.php
b/tests/integration/arraynumkey/test.php
index 7b16917..e5dace8 100644
--- a/tests/integration/arraynumkey/test.php
+++ b/tests/integration/arraynumkey/test.php
@@ -80,3 +80,6 @@
'HAVING' => $_GET['string']
]
);
+
+$b = (int)$_GET['b'];
+$db->select( 't', 'f', [ 'foo' => $_GET['a'], "bar > $b" ] );
diff --git a/tests/integration/cast/expectedResults.txt
b/tests/integration/cast/expectedResults.txt
index 9be25a4..7062b67 100644
--- a/tests/integration/cast/expectedResults.txt
+++ b/tests/integration/cast/expectedResults.txt
@@ -1 +1 @@
-integration/cast/test.php:4 SecurityCheck-XSS Echoing expression that was not
html escaped
+integration/cast/test.php:7 SecurityCheck-XSS Echoing expression that was not
html escaped
diff --git a/tests/integration/cast/test.php b/tests/integration/cast/test.php
index ec557c7..2259f47 100644
--- a/tests/integration/cast/test.php
+++ b/tests/integration/cast/test.php
@@ -5,3 +5,5 @@
// Less so
echo (string)$_GET['bar'];
+
+echo intval( $_GET['bar'] );
diff --git a/tests/integration/dbarrayplus/test.php
b/tests/integration/dbarrayplus/test.php
index 7728fab..c6c6ab1 100644
--- a/tests/integration/dbarrayplus/test.php
+++ b/tests/integration/dbarrayplus/test.php
@@ -35,3 +35,20 @@
$rows + $rows2 + $unsafe,
__METHOD__
);
+
+$db->insert(
+ 'foo',
+ [
+ [ 'first' => $_GET['a'] ],
+ [ 'first' => $_GET['a'] ],
+ [ 'first' => $_GET['a'] ],
+ ],
+ __METHOD__
+);
+
+$items = [];
+$items[] = [ 'first' => $_GET['a'] ];
+$items[] = [ 'first' => $_GET['a'] ];
+$items[] = [ 'first' => $_GET['a'] ];
+
+$db->insert( 'foo', $items, __METHOD__ );
diff --git a/tests/integration/prop2/expectedResults.txt
b/tests/integration/prop2/expectedResults.txt
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/tests/integration/prop2/expectedResults.txt
diff --git a/tests/integration/prop2/test.php b/tests/integration/prop2/test.php
new file mode 100644
index 0000000..358051f
--- /dev/null
+++ b/tests/integration/prop2/test.php
@@ -0,0 +1,34 @@
+<?php
+
+class Foo {
+ public $f = '';
+
+ /** @var Context */
+ private $context;
+
+ public function __construct() {
+ $this->context = new Context;
+ }
+
+ public function bar() {
+ $out = $this->context->getOutput();
+ $out->addHTML( $this->f );
+ }
+}
+
+class Context {
+ /**
+ * @return OutputPage
+ */
+ public function getOutput() {
+ return new OutputPage;
+ }
+}
+
+class OutputPage {
+ public function addHTML( $html ) {
+ }
+}
+
+$foo = new Foo;
+$foo->bar();
--
To view, visit https://gerrit.wikimedia.org/r/395761
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I17f0dbd83c822faa0bcaa823f5adf15d6366d3f4
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
