[MediaWiki-commits] [Gerrit] mediawiki...SecurityCheckPlugin[master]: Fix bug where non-local variables are treated like local
Brian Wolff has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/394869 )
Change subject: Fix bug where non-local variables are treated like local
..
Fix bug where non-local variables are treated like local
Previously for globals and $this->classProps if someone
assigned to them, the taint was totally overriden. This is
often wrong, as they can be modified in other functions/methods.
Change-Id: Icc34010ed39c58e9d6bf2007f63257e4072a5592
---
M .phpcs.xml
M src/TaintednessVisitor.php
2 files changed, 12 insertions(+), 4 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/tools/phan/SecurityCheckPlugin
refs/changes/69/394869/1
diff --git a/.phpcs.xml b/.phpcs.xml
index 309c64f..c9196a4 100644
--- a/.phpcs.xml
+++ b/.phpcs.xml
@@ -6,6 +6,7 @@
+
.
@@ -43,4 +44,7 @@
./tests
+
+ ./tests
+
diff --git a/src/TaintednessVisitor.php b/src/TaintednessVisitor.php
index e7a5221..d4b588d 100644
--- a/src/TaintednessVisitor.php
+++ b/src/TaintednessVisitor.php
@@ -294,11 +294,15 @@
// echo __METHOD__ . $this->dbgInfo() . ' ';
// Debug::printNode($node);
- // FIXME This is wrong for non-local vars (including class
props)
- // Depending on order of methods in the class file.
+ // Note: If there is a local variable that is a reference
+ // to another non-local variable, this will probably incorrectly
+ // override the taint (Pass by reference variables are handled
+ // specially and should be ok).
// Make sure $foo[2] = 0; doesn't kill taint of $foo generally.
- $override = $node->children['var']->kind !== \ast\AST_DIM;
+ // Ditto for $this->bar, or props in general just in case.
+ $override = $node->children['var']->kind !== \ast\AST_DIM
+ && $node->children['var']->kind !== \ast\AST_PROP;
try {
$variableObjs = $this->getPhanObjsForNode(
$node->children['var'] );
} catch ( Exception $e ) {
@@ -671,7 +675,7 @@
}
$localVar->taintedness =& $globalVar->taintedness;
- $localVar->taintednessHasExtendedScope = true;
+ $localVar->taintednessHasOuterScope = true;
}
return SecurityCheckPlugin::INAPLICABLE_TAINT;
}
--
To view, visit https://gerrit.wikimedia.org/r/394869
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icc34010ed39c58e9d6bf2007f63257e4072a5592
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/tools/phan/SecurityCheckPlugin
Gerrit-Branch: master
Gerrit-Owner: Brian Wolff
___
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
[MediaWiki-commits] [Gerrit] mediawiki...SecurityCheckPlugin[master]: Fix bug where non-local variables are treated like local
Brian Wolff has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/394869 )
Change subject: Fix bug where non-local variables are treated like local
..
Fix bug where non-local variables are treated like local
Previously for globals and $this->classProps if someone
assigned to them, the taint was totally overriden. This is
often wrong, as they can be modified in other functions/methods.
Change-Id: Icc34010ed39c58e9d6bf2007f63257e4072a5592
---
M .phpcs.xml
M src/TaintednessVisitor.php
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/.phpcs.xml b/.phpcs.xml
index 309c64f..c9196a4 100644
--- a/.phpcs.xml
+++ b/.phpcs.xml
@@ -6,6 +6,7 @@
+
.
@@ -43,4 +44,7 @@
./tests
+
+ ./tests
+
diff --git a/src/TaintednessVisitor.php b/src/TaintednessVisitor.php
index e7a5221..d4b588d 100644
--- a/src/TaintednessVisitor.php
+++ b/src/TaintednessVisitor.php
@@ -294,11 +294,15 @@
// echo __METHOD__ . $this->dbgInfo() . ' ';
// Debug::printNode($node);
- // FIXME This is wrong for non-local vars (including class
props)
- // Depending on order of methods in the class file.
+ // Note: If there is a local variable that is a reference
+ // to another non-local variable, this will probably incorrectly
+ // override the taint (Pass by reference variables are handled
+ // specially and should be ok).
// Make sure $foo[2] = 0; doesn't kill taint of $foo generally.
- $override = $node->children['var']->kind !== \ast\AST_DIM;
+ // Ditto for $this->bar, or props in general just in case.
+ $override = $node->children['var']->kind !== \ast\AST_DIM
+ && $node->children['var']->kind !== \ast\AST_PROP;
try {
$variableObjs = $this->getPhanObjsForNode(
$node->children['var'] );
} catch ( Exception $e ) {
@@ -671,7 +675,7 @@
}
$localVar->taintedness =& $globalVar->taintedness;
- $localVar->taintednessHasExtendedScope = true;
+ $localVar->taintednessHasOuterScope = true;
}
return SecurityCheckPlugin::INAPLICABLE_TAINT;
}
--
To view, visit https://gerrit.wikimedia.org/r/394869
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Icc34010ed39c58e9d6bf2007f63257e4072a5592
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/tools/phan/SecurityCheckPlugin
Gerrit-Branch: master
Gerrit-Owner: Brian Wolff
Gerrit-Reviewer: Brian Wolff
___
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
