Albert221 has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/399776 )
Change subject: [WIP] Add double escaping detection
......................................................................
[WIP] Add double escaping detection
Change-Id: Iec0b45cd067caee99e5187b10e7a9350078d053f
---
M MediaWikiSecurityCheckPlugin.php
M scripts/generic-config.php
M scripts/mw-config.php
M scripts/mwext-config.php
M scripts/mwext-fast-config.php
M scripts/mwext-slow-config.php
M src/SecurityCheckPlugin.php
M src/TaintednessBaseVisitor.php
A tests/integration/doubleescaped/expectedResults.txt
A tests/integration/doubleescaped/test.php
10 files changed, 39 insertions(+), 17 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/tools/phan/SecurityCheckPlugin
refs/changes/76/399776/1
diff --git a/MediaWikiSecurityCheckPlugin.php b/MediaWikiSecurityCheckPlugin.php
index f9c8198..1b675f8 100644
--- a/MediaWikiSecurityCheckPlugin.php
+++ b/MediaWikiSecurityCheckPlugin.php
@@ -299,9 +299,9 @@
// '\wfMessage' => self::YES_TAINT,
'\Message::plain' => [ 'overall' => self::YES_TAINT, ],
'\Message::text' => [ 'overall' => self::YES_TAINT, ],
- '\Message::parseAsBlock' => [ 'overall' =>
self::NO_TAINT, ],
- '\Message::parse' => [ 'overall' => self::NO_TAINT, ],
- '\Message::escaped' => [ 'overall' => self::NO_TAINT, ],
+ '\Message::parseAsBlock' => [ 'overall' =>
self::ESCAPED_TAINT, ],
+ '\Message::parse' => [ 'overall' =>
self::ESCAPED_TAINT, ],
+ '\Message::escaped' => [ 'overall' =>
self::ESCAPED_TAINT, ],
'\Message::rawParams' => [
self::HTML_EXEC_TAINT,
self::HTML_EXEC_TAINT,
@@ -397,13 +397,13 @@
'overall' => self::NO_TAINT
],
'\Xml::encodeJsVar' => [
- self::YES_TAINT & ~self::HTML_TAINT,
+ ( self::YES_TAINT & ~self::HTML_TAINT ) |
self::ESCAPED_EXEC_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::YES_TAINT & ~self::HTML_TAINT ) |
self::ESCAPED_EXEC_TAINT, /* args */
self::NO_TAINT, /* pretty */
'overall' => self::NO_TAINT
],
@@ -425,16 +425,16 @@
],
'\OutputPage::parse' => [ 'overall' => self::NO_TAINT,
],
'\Parser::parse' => [
- self::YES_TAINT & ~self::HTML_TAINT,
+ ( self::YES_TAINT & ~self::HTML_TAINT ) |
self::ESCAPED_EXEC_TAINT,
'overall' => self::NO_TAINT,
],
'\Parser::recursiveTagParse' => [
- self::YES_TAINT & ~self::HTML_TAINT,
+ ( self::YES_TAINT & ~self::HTML_TAINT ) |
self::ESCAPED_EXEC_TAINT,
self::NO_TAINT,
'overall' => self::NO_TAINT,
],
'\Parser::recursiveTagParseFully' => [
- self::YES_TAINT & ~self::HTML_TAINT,
+ ( self::YES_TAINT & ~self::HTML_TAINT ) |
self::ESCAPED_EXEC_TAINT,
self::NO_TAINT,
'overall' => self::NO_TAINT,
],
@@ -443,7 +443,7 @@
'overall' => self::NO_TAINT,
],
'\Sanitizer::removeHTMLtags' => [
- self::YES_TAINT & ~self::HTML_TAINT, /* text */
+ ( self::YES_TAINT & ~self::HTML_TAINT ) |
self::ESCAPED_EXEC_TAINT, /* text */
self::SHELL_EXEC_TAINT, /* attribute callback */
self::NO_TAINT, /* callback args */
self::YES_TAINT, /* extra tags */
@@ -451,15 +451,15 @@
'overall' => self::NO_TAINT
],
'\Sanitizer::escapeHtmlAllowEntities' => [
- self::YES_TAINT & ~self::HTML_TAINT,
+ ( self::YES_TAINT & ~self::HTML_TAINT ) |
self::ESCAPED_EXEC_TAINT,
'overall' => self::NO_TAINT
],
'\Sanitizer::safeEncodeAttribute' => [
- self::YES_TAINT & ~self::HTML_TAINT,
+ ( self::YES_TAINT & ~self::HTML_TAINT ) |
self::ESCAPED_EXEC_TAINT,
'overall' => self::NO_TAINT
],
'\Sanitizer::encodeAttribute' => [
- self::YES_TAINT & ~self::HTML_TAINT,
+ ( self::YES_TAINT & ~self::HTML_TAINT ) |
self::ESCAPED_EXEC_TAINT,
'overall' => self::NO_TAINT
],
'\WebRequest::getGPCVal' => [ 'overall' =>
self::YES_TAINT, ],
@@ -489,7 +489,7 @@
'\WebRequest::getHeader' => [ 'overall' =>
self::YES_TAINT, ],
'\WebRequest::getAcceptLang' => [ 'overall' =>
self::YES_TAINT, ],
'OOUI\HtmlSnippet::__construct' => [
- self::HTML_EXEC_TAINT & self::YES_TAINT,
+ ( self::YES_TAINT & ~self::HTML_TAINT ) |
self::ESCAPED_EXEC_TAINT,
'overall' => self::NO_TAINT
],
'OOUI\FieldLayout::__construct' => [
diff --git a/scripts/generic-config.php b/scripts/generic-config.php
index 91b14ec..c40d597 100644
--- a/scripts/generic-config.php
+++ b/scripts/generic-config.php
@@ -93,6 +93,7 @@
'SecurityCheck-XSS',
'SecurityCheck-SQLInjection',
'SecurityCheck-ShellInjection',
+ 'SecurityCheck-DoubleEscaped',
'SecurityCheck-CUSTOM1',
'SecurityCheck-CUSTOM2',
'SecurityCheck-OTHER',
diff --git a/scripts/mw-config.php b/scripts/mw-config.php
index 89f79d1..2494562 100644
--- a/scripts/mw-config.php
+++ b/scripts/mw-config.php
@@ -130,6 +130,7 @@
'SecurityCheck-XSS',
'SecurityCheck-SQLInjection',
'SecurityCheck-ShellInjection',
+ 'SecurityCheck-DoubleEscaped',
'SecurityCheck-CUSTOM1',
'SecurityCheck-CUSTOM2',
'SecurityCheck-OTHER',
diff --git a/scripts/mwext-config.php b/scripts/mwext-config.php
index 1df6fe0..3f63147 100644
--- a/scripts/mwext-config.php
+++ b/scripts/mwext-config.php
@@ -134,6 +134,7 @@
'SecurityCheck-XSS',
'SecurityCheck-SQLInjection',
'SecurityCheck-ShellInjection',
+ 'SecurityCheck-DoubleEscaped',
'SecurityCheck-CUSTOM1',
'SecurityCheck-CUSTOM2',
'SecurityCheck-OTHER',
diff --git a/scripts/mwext-fast-config.php b/scripts/mwext-fast-config.php
index 79590a4..3d2af2c 100644
--- a/scripts/mwext-fast-config.php
+++ b/scripts/mwext-fast-config.php
@@ -129,6 +129,7 @@
'SecurityCheck-XSS',
'SecurityCheck-SQLInjection',
'SecurityCheck-ShellInjection',
+ 'SecurityCheck-DoubleEscaped',
'SecurityCheck-CUSTOM1',
'SecurityCheck-CUSTOM2',
'SecurityCheck-OTHER',
diff --git a/scripts/mwext-slow-config.php b/scripts/mwext-slow-config.php
index 26166f3..7cd5230 100644
--- a/scripts/mwext-slow-config.php
+++ b/scripts/mwext-slow-config.php
@@ -128,6 +128,7 @@
'SecurityCheck-XSS',
'SecurityCheck-SQLInjection',
'SecurityCheck-ShellInjection',
+ 'SecurityCheck-DoubleEscaped',
'SecurityCheck-CUSTOM1',
'SecurityCheck-CUSTOM2',
'SecurityCheck-OTHER',
diff --git a/src/SecurityCheckPlugin.php b/src/SecurityCheckPlugin.php
index 69182c5..56a0c35 100644
--- a/src/SecurityCheckPlugin.php
+++ b/src/SecurityCheckPlugin.php
@@ -77,6 +77,9 @@
// the SQL_TAINT flag set.
const SQL_NUMKEY_TAINT = 131072;
const SQL_NUMKEY_EXEC_TAINT = 262144;
+ // For double escaped variables
+ const ESCAPED_TAINT = 131072;
+ const ESCAPED_EXEC_TAINT = 262144;
// Special purpose flags(Starting at 2^28)
// Cancel's out all EXEC flags on a function arg if arg is array.
@@ -87,7 +90,7 @@
const EXEC_TAINT = 87376;
const YES_EXEC_TAINT = 131064;
// ALL_TAINT == YES_TAINT | SQL_NUMKEY_TAINT
- const ALL_TAINT = 174760;
+ const ALL_TAINT = 305832;
const ALL_EXEC_TAINT = 349520;
/**
@@ -202,15 +205,15 @@
}
/**
- * Tains for builtin php functions
+ * Taints for builtin php functions
*
* @return array List of func taints (See getBuiltinFuncTaint())
*/
protected function getPHPFuncTaints() : array {
return [
'\htmlspecialchars' => [
- ~self::HTML_TAINT & self::YES_TAINT,
- 'overall' => self::NO_TAINT
+ ( ~self::HTML_TAINT & self::YES_TAINT ) |
self::ESCAPED_TAINT,
+ 'overall' => self::ESCAPED_TAINT
],
'\escapeshellarg' => [
~self::SHELL_TAINT & self::YES_TAINT,
diff --git a/src/TaintednessBaseVisitor.php b/src/TaintednessBaseVisitor.php
index ab31b09..20bee7f 100644
--- a/src/TaintednessBaseVisitor.php
+++ b/src/TaintednessBaseVisitor.php
@@ -1461,6 +1461,10 @@
// of false positives.
// $severity = 4;
} elseif (
+ $combinedTaint === SecurityCheckPlugin::ESCAPED_TAINT
+ ) {
+ $issueType = 'SecurityCheck-DoubleEscaped';
+ } elseif (
$combinedTaint === SecurityCheckPlugin::CUSTOM1_TAINT
) {
$issueType = 'SecurityCheck-CUSTOM1';
diff --git a/tests/integration/doubleescaped/expectedResults.txt
b/tests/integration/doubleescaped/expectedResults.txt
new file mode 100644
index 0000000..bcbe7f0
--- /dev/null
+++ b/tests/integration/doubleescaped/expectedResults.txt
@@ -0,0 +1 @@
+fsdgdfgdf
diff --git a/tests/integration/doubleescaped/test.php
b/tests/integration/doubleescaped/test.php
new file mode 100644
index 0000000..98777f7
--- /dev/null
+++ b/tests/integration/doubleescaped/test.php
@@ -0,0 +1,9 @@
+<?php
+
+$someHtml = '<h1>Hello there!</h1>';
+
+$escaped = htmlspecialchars( $someHtml );
+$doubleEscaped = htmlspecialchars( $escaped );
+
+// Oh no! It's double escaped!
+echo $doubleEscaped;
--
To view, visit https://gerrit.wikimedia.org/r/399776
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iec0b45cd067caee99e5187b10e7a9350078d053f
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/tools/phan/SecurityCheckPlugin
Gerrit-Branch: master
Gerrit-Owner: Albert221 <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
