Further improvements

This batch of improvements include: - Remove a pointless PRESERVE_TAINT flag. That was probably a leftover from some experiment, and it has no effect because non-Nodes cannot have PRESERVE_TAINT; - Improve namings a bit for visitEcho - Rewrite the visitNew part: the previous one was pretty flaky, and failed e.g. for 'parent' and 'static', plus it was unnecessarily long. This new version is a bit clearer, although it still doesn't work properly with 'new static'. I suspect this is an upstream problem, phan/phan#2718. Depends-On: I2a17ad292c61c5712a68729bc085a73de4ddb31d Change-Id: Id814de479e165261b867d652fad9870cf8c90403
wikimedia · Oct 15, 2019 · 4281d0a · 4281d0a
1 parent ef1dcea
commit 4281d0a
Show file tree

Hide file tree

Showing 5 changed files with 117 additions and 47 deletions.
diff --git a/src/TaintednessVisitor.php b/src/TaintednessVisitor.php
@@ -17,13 +17,12 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  */
 
-use Phan\AST\ContextNode;
 use Phan\CodeBase;
 use Phan\Debug;
+use Phan\Exception\CodeBaseException;
 use Phan\Language\Context;
 use Phan\Language\Element\PassByReferenceVariable;
 use Phan\Language\Element\Variable;
-use Phan\Language\FQSEN\FullyQualifiedMethodName;
 use Phan\Language\FQSEN\FullyQualifiedFunctionName;
 use ast\Node;
 use Phan\Exception\IssueException;
@@ -379,7 +378,6 @@ public function visitAssignOp( Node $node ) : int {
 	/**
 	 * Also handles visitAssignOp
 	 *
-	 * @todo FIXME, doesn't handle list( $a, $b ) = $foo;
 	 * @param Node $node
 	 * @return int Taint
 	 */
@@ -642,32 +640,29 @@ public function visitIncludeOrEval( Node $node ) : int {
 	 * @return int Taint
 	 */
 	public function visitEcho( Node $node ) : int {
-		$taintedness = $this->getTaintedness( $node->children['expr'] );
+		$echoTaint = SecurityCheckPlugin::HTML_EXEC_TAINT;
+		$echoedExpr = $node->children['expr'];
+		$taintedness = $this->getTaintedness( $echoedExpr );
 		# $this->debug( __METHOD__, "Echoing with taint $taintedness" );
 
 		$this->maybeEmitIssue(
-			SecurityCheckPlugin::HTML_EXEC_TAINT,
+			$echoTaint,
 			$taintedness,
 			"Echoing expression that was not html escaped"
-				. $this->getOriginalTaintLine( $node->children['expr'] )
+				. $this->getOriginalTaintLine( $echoedExpr )
 		);
 
-		// FIXME what is the PRESERVE_TAINT doing in the condition??
-		if (
-			$this->isSafeAssignment( SecurityCheckPlugin::HTML_EXEC_TAINT, $taintedness ) &&
-			( is_object( $node->children['expr'] ) ||
-			$taintedness & SecurityCheckPlugin::PRESERVE_TAINT )
-		) {
+		if ( $this->isSafeAssignment( $echoTaint, $taintedness ) && is_object( $echoedExpr ) ) {
 			// In the event the assignment looks safe, keep track of it,
 			// in case it later turns out not to be safe.
-			$phanObjs = $this->getPhanObjsForNode( $node->children['expr'], [ 'return' ] );
+			$phanObjs = $this->getPhanObjsForNode( $echoedExpr, [ 'return' ] );
 			foreach ( $phanObjs as $phanObj ) {
 				$this->debug( __METHOD__, "Setting {$phanObj->getName()} exec due to echo" );
 				// FIXME, maybe not do this for local variables
 				// since they don't have other code paths that can set them.
 				$this->markAllDependentMethodsExec(
 					$phanObj,
-					SecurityCheckPlugin::HTML_EXEC_TAINT
+					$echoTaint
 				);
 			}
 		}
@@ -709,11 +704,7 @@ public function visitNew( Node $node ) : int {
 	 * @return int Taint
 	 */
 	public function visitMethodCall( Node $node ) : int {
-		$ctxNode = new ContextNode(
-			$this->code_base,
-			$this->context,
-			$node
-		);
+		$ctxNode = $this->getCtxN( $node );
 		$isStatic = ( $node->kind === \ast\AST_STATIC_CALL );
 		$isFunc = ( $node->kind === \ast\AST_CALL );
 
@@ -724,43 +715,33 @@ public function visitMethodCall( Node $node ) : int {
 				// We check the __construct() method first, but the
 				// final resulting taint is from the __toString()
 				// method. This is a little hacky.
-				$clazzName = $node->children['class']->children['name'];
-				if ( $clazzName === 'self' && $this->context->isInClassScope() ) {
-					$clazzName = (string)$this->context->getClassFQSEN();
-				}
-				$fqsen = FullyQualifiedMethodName::fromStringInContext(
-					$clazzName . '::__construct',
-					$this->context
+				$constructor = $ctxNode->getMethod(
+					'__construct',
+					false,
+					false,
+					true
 				);
-				if ( !$this->code_base->hasMethodWithFQSEN( $fqsen ) ) {
-					$this->debug( __METHOD__, "FIXME no constructor for $fqsen" );
-					throw new exception( "Cannot find __construct" );
-				}
-				$func = $this->code_base->getMethodByFQSEN( $fqsen );
 				// First do __construct()
 				$this->handleMethodCall(
-					$func,
-					$func->getFQSEN(),
-					$this->getTaintOfFunction( $func ),
+					$constructor,
+					$constructor->getFQSEN(),
+					$this->getTaintOfFunction( $constructor ),
 					$node->children['args']->children
 				);
 				// Now return __toString()
-				$fqsen = FullyQualifiedMethodName::fromStringInContext(
-					$clazzName . '::__toString',
-					$this->context
-				);
-				if ( !$this->code_base->hasMethodWithFQSEN( $fqsen ) ) {
-					$this->debug( __METHOD__, "no __toString() $fqsen" );
-					// If there is no __toString(), then presumably
-					// the object can't be outputed, so should be
-					// safe.
+				$clazz = $constructor->getClass( $this->code_base );
+				try {
+					$toString = $clazz->getMethodByName( $this->code_base, '__toString' );
+				} catch ( CodeBaseException $_ ) {
+					// There is no __toString(), then presumably the object can't be outputed, so should be safe.
+					$this->debug( __METHOD__, "no __toString() in $clazz" );
 					return SecurityCheckPlugin::NO_TAINT;
 				}
-				$func = $this->code_base->getMethodByFQSEN( $fqsen );
+
 				return $this->handleMethodCall(
-					$func,
-					$func->getFQSEN(),
-					$this->getTaintOfFunction( $func ),
+					$toString,
+					$toString->getFQSEN(),
+					$this->getTaintOfFunction( $toString ),
 					[] // __toString() has no args
 				);
 			} elseif ( $isFunc ) {

diff --git a/tests/integration/constructors/expectedResults.txt b/tests/integration/constructors/expectedResults.txt
@@ -0,0 +1,2 @@
+integration/constructors/test.php:50 SecurityCheck-XSS Calling method \Bad::printArg() in [no method] that outputs using tainted argument $unsafe. (Caused by: integration/constructors/test.php +27) (Caused by: integration/constructors/test.php +43)
+integration/constructors/test.php:62 SecurityCheck-XSS Echoing expression that was not html escaped (Caused by: integration/constructors/test.php +35)
diff --git a/tests/integration/constructors/test.php b/tests/integration/constructors/test.php
@@ -0,0 +1,63 @@
+<?php
+
+class Good {
+	public static function getMyInstance() {
+		return new self;
+	}
+	public static function getStaticInstance() {
+		return new static;
+	}
+
+	public function printArg( $arg ) {
+		echo htmlspecialchars( $arg );
+	}
+	public function __toString() : string {
+		return 'safe';
+	}
+}
+class Bad extends Good {
+	public static function getMyInstance() {
+		return new self;
+	}
+	public static function getParent() {
+		return new parent;
+	}
+
+	public function printArg( $arg ) {
+		echo $arg;
+	}
+	public function __toString() : string {
+		return $_GET['unsafe'];
+	}
+}
+
+$a1 = Good::getMyInstance();
+$b1 = Bad::getMyInstance();
+
+$a2 = Good::getStaticInstance();
+$b2 = Bad::getStaticInstance();
+
+$a3 = Bad::getParent();
+
+$safe = 'foobar';
+$unsafe = $_GET['foobar'];
+
+// phpcs:disable Generic.Files.LineLength
+// Calls on $a* are always safe, calls on $b* depend on the param
+$a1->printArg( $safe );
+$a1->printArg( $unsafe );
+$b1->printArg( $safe );
+$b1->printArg( $unsafe ); // Unsafe
+$a2->printArg( $safe );
+$a2->printArg( $unsafe );
+$b2->printArg( $safe );
+$b2->printArg( $unsafe ); // NOTE This is unsafe but isn't reported due to wrong type being inferred for $b2, https://github.com/phan/phan/issues/2718
+$a3->printArg( $safe );
+$a3->printArg( $unsafe );
+
+// The __toString method is only safe for $a*
+echo $a1;
+echo $a2;
+echo $a3;
+echo $b1;
+echo $b2; // NOTE This is unsafe but isn't reported due to wrong type being inferred for $b2, https://github.com/phan/phan/issues/2718
diff --git a/tests/integration/listassign/expectedResults.txt b/tests/integration/listassign/expectedResults.txt
@@ -0,0 +1,4 @@
+integration/listassign/test.php:13 SecurityCheck-XSS Calling method \foo() in [no method] that outputs using tainted argument $unsafe. (Caused by: integration/listassign/test.php +5) (Caused by: integration/listassign/test.php +9)
+integration/listassign/test.php:14 SecurityCheck-XSS Calling method \foo() in [no method] that outputs using tainted argument $mixed. (Caused by: integration/listassign/test.php +5) (Caused by: integration/listassign/test.php +10)
+integration/listassign/test.php:19 SecurityCheck-XSS Echoing expression that was not html escaped (Caused by: integration/listassign/test.php +16; integration/listassign/test.php +10)
+integration/listassign/test.php:20 SecurityCheck-XSS Echoing expression that was not html escaped (Caused by: integration/listassign/test.php +16; integration/listassign/test.php +10)
diff --git a/tests/integration/listassign/test.php b/tests/integration/listassign/test.php
@@ -0,0 +1,20 @@
+<?php
+
+function foo( array $arr ) {
+	list( $a, $b ) = $arr;
+	echo "$a and $b";
+}
+
+$safe = [ 'foo', 'bar' ];
+$unsafe = [ $_GET['Good'], $_GET['b'] ];
+$mixed = [ 'foo', $_GET['Good'] ];
+
+foo( $safe );
+foo( $unsafe );
+foo( $mixed );
+
+list( $safe, $unsafe ) = $mixed;
+
+// This is safe, but is not reported as such because array elements share taintedness
+echo $safe;
+echo $unsafe;