[MediaWiki-commits] [Gerrit] mediawiki...SecurityCheckPlugin[master]: Fix various false positives found when testing with MW
Brian Wolff has submitted this change and it was merged. ( 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(-) 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() ); +
[MediaWiki-commits] [Gerrit] mediawiki...SecurityCheckPlugin[master]: Fix various false positives found when testing with MW
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 +
