Fix #4605 - taint parent-declared property

vimeo · Nov 18, 2020 · be275ae · be275ae
1 parent 39c508f
commit be275ae
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 3 deletions.
diff --git a/...Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php b/...Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php
@@ -412,7 +412,7 @@ public static function analyze(
                             $has_regular_setter = true;
                             $property_exists = true;
 
-                            if (!$context->collect_initializations) {
+                            if (!$context->collect_initializations && !$context->collect_mutations) {
                                 self::taintProperty(
                                     $statements_analyzer,
                                     $stmt,
@@ -483,7 +483,7 @@ public static function analyze(
                      * not in that list, fall through
                      */
                     if (!$var_id || !$class_storage->sealed_properties) {
-                        if (!$context->collect_initializations) {
+                        if (!$context->collect_initializations && !$context->collect_mutations) {
                             self::taintProperty(
                                 $statements_analyzer,
                                 $stmt,
@@ -535,7 +535,10 @@ public static function analyze(
                     }
                 }
 
-                if ($statements_analyzer->data_flow_graph && !$context->collect_initializations) {
+                if ($statements_analyzer->data_flow_graph
+                    && !$context->collect_initializations
+                    && !$context->collect_mutations
+                ) {
                     $class_storage = $codebase->classlike_storage_provider->get($fq_class_name);
 
                     self::taintProperty(
@@ -1162,6 +1165,8 @@ private static function taintProperty(
             return;
         }
 
+        $codebase = $statements_analyzer->getCodebase();
+
         $data_flow_graph = $statements_analyzer->data_flow_graph;
 
         $var_location = new CodeLocation($statements_analyzer->getSource(), $stmt->var);
@@ -1265,6 +1270,29 @@ private static function taintProperty(
                     $data_flow_graph->addPath($parent_node, $localized_property_node, '=');
                 }
             }
+
+            $declaring_property_class = $codebase->properties->getDeclaringClassForProperty(
+                $property_id,
+                false,
+                $statements_analyzer
+            );
+
+            if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph
+                && $declaring_property_class
+                && $declaring_property_class !== $class_storage->name
+                && $stmt->name instanceof PhpParser\Node\Identifier
+            ) {
+                $declaring_property_node = new DataFlowNode(
+                    $declaring_property_class . '::$' . $stmt->name,
+                    $declaring_property_class . '::$' . $stmt->name,
+                    null,
+                    null
+                );
+
+                $data_flow_graph->addNode($declaring_property_node);
+
+                $data_flow_graph->addPath($property_node, $declaring_property_node, 'property-assignment');
+            }
         }
     }
 }
diff --git a/tests/TaintTest.php b/tests/TaintTest.php
@@ -1782,6 +1782,26 @@ public function __construct($taint) {
                     ldap_search($ds, $dn, $filter, []);',
                 'error_message' => 'TaintedLdap',
             ],
+            'potentialTaintThroughChildClassSettingProperty' => [
+                '<?php
+                    class A {
+                        public string $taint = "";
+
+                        public function getTaint() : string {
+                            return $this->taint;
+                        }
+                    }
+
+                    class B extends A {
+                        public function __construct(string $taint) {
+                            $this->taint = $taint;
+                        }
+                    }
+
+                    $b = new B($_GET["bar"]);
+                    echo $b->getTaint();',
+                'error_message' => 'TaintedHtml',
+            ],
             /*
             // TODO: Stubs do not support this type of inference even with $this->message = $message.
             // Most uses of getMessage() would be with caught exceptions, so this is not representative of real code.