Merge pull request github#12203 from RasmusWL/import-resolution-phi

Python: Handle if-then-else definitions in import resolution

Author: RasmusWL

python/ql/lib/change-notes/2023-02-16-import-if-then-else.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Fixed module resolution so we properly recognize definitions made within if-then-else statements.

python/ql/lib/semmle/python/dataflow/new/internal/ImportResolution.qll

@@ -71,13 +71,19 @@ module ImportResolution {
    */
   pragma[nomagic]
   predicate module_export(Module m, string name, DataFlow::CfgNode defn) {
-    exists(EssaVariable v |
+    exists(EssaVariable v, EssaDefinition essaDef |
       v.getName() = name and
-      v.getAUse() = ImportStar::getStarImported*(m).getANormalExit()
+      v.getAUse() = ImportStar::getStarImported*(m).getANormalExit() and
+      (
+        essaDef = v.getDefinition()
+        or
+        // to handle definitions guarded by if-then-else
+        essaDef = v.getDefinition().(PhiFunction).getAnInput()
+      )
     |
-      defn.getNode() = v.getDefinition().(AssignmentDefinition).getValue()
+      defn.getNode() = essaDef.(AssignmentDefinition).getValue()
       or
-      defn.getNode() = v.getDefinition().(ArgumentRefinement).getArgument()
+      defn.getNode() = essaDef.(ArgumentRefinement).getArgument()
     )
     or
     exists(Alias a |

python/ql/test/experimental/import-resolution/if_then_else.py

@@ -0,0 +1,16 @@
+from trace import *
+enter(__file__)
+
+# definition based on "random" choice in this case it will always go the the if-branch,
+# but our analysis is not able to figure this out
+if eval("True"):
+    if_then_else_defined = "if_defined"
+else:
+    # we also check that nested if-then-else works, it would be easy to accidentally
+    # only support _one_ level of nesting.
+    if eval("True"):
+        if_then_else_defined = "else_defined_1"
+    else:
+        if_then_else_defined = "else_defined_2"
+
+exit(__file__)

python/ql/test/experimental/import-resolution/main.py

@@ -89,6 +89,9 @@ def local_import():
 from package.subpackage2 import *
 check("subpackage2_attr", subpackage2_attr, "subpackage2_attr", globals()) #$ prints=subpackage2_attr
 
+# check that definitions from within if-then-else are found
+from if_then_else import if_then_else_defined
+check("if_then_else_defined", if_then_else_defined, "if_defined", globals()) #$ prints=if_defined prints=else_defined_1 prints=else_defined_2
 
 exit(__file__)
 

python/ql/test/query-tests/Security/CWE-732-WeakFilePermissions/WeakFilePermissions.expected

@@ -2,5 +2,6 @@
 | test.py:8:1:8:20 | ControlFlowNode for Attribute() | Overly permissive mask in chmod sets file to world writable. |
 | test.py:9:1:9:21 | ControlFlowNode for Attribute() | Overly permissive mask in chmod sets file to world writable. |
 | test.py:11:1:11:21 | ControlFlowNode for Attribute() | Overly permissive mask in chmod sets file to group readable. |
+| test.py:13:1:13:28 | ControlFlowNode for Attribute() | Overly permissive mask in chmod sets file to group writable. |
 | test.py:14:1:14:19 | ControlFlowNode for Attribute() | Overly permissive mask in chmod sets file to group writable. |
 | test.py:16:1:16:25 | ControlFlowNode for Attribute() | Overly permissive mask in open sets file to world readable. |