Merge branch 'main' into gh-codeql-nightly

aibaars · web-flow · commit 912dce3ea7aa · 2022-08-03T14:43:07.000+02:00
diff --git a/cpp/ql/lib/change-notes/2022-08-02-must-flow-local-only-flow.md b/cpp/ql/lib/change-notes/2022-08-02-must-flow-local-only-flow.md
@@ -0,0 +1,4 @@
+---
+category: feature
+---
+* A new class predicate `MustFlowConfiguration::allowInterproceduralFlow` has been added to the `semmle.code.cpp.ir.dataflow.MustFlow` library. The new predicate can be overridden to disable interprocedural flow.
diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll
@@ -38,6 +38,9 @@ abstract class MustFlowConfiguration extends string {
    */
   predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { none() }
 
+  /** Holds if this configuration allows flow from arguments to parameters. */
+  predicate allowInterproceduralFlow() { any() }
+
   /**
    * Holds if data must flow from `source` to `sink` for this configuration.
    *
@@ -204,10 +207,25 @@ private module Cached {
   }
 }
 
+/**
+ * Gets the enclosing callable of `n`. Unlike `n.getEnclosingCallable()`, this
+ * predicate ensures that joins go from `n` to the result instead of the other
+ * way around.
+ */
+pragma[inline]
+private Declaration getEnclosingCallable(DataFlow::Node n) {
+  pragma[only_bind_into](result) = pragma[only_bind_out](n).getEnclosingCallable()
+}
+
 /** Holds if `nodeFrom` flows to `nodeTo`. */
 private predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo, MustFlowConfiguration config) {
   exists(config) and
-  Cached::step(nodeFrom, nodeTo)
+  Cached::step(pragma[only_bind_into](nodeFrom), pragma[only_bind_into](nodeTo)) and
+  (
+    config.allowInterproceduralFlow()
+    or
+    getEnclosingCallable(nodeFrom) = getEnclosingCallable(nodeTo)
+  )
   or
   config.isAdditionalFlowStep(nodeFrom, nodeTo)
 }
diff --git a/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql b/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql
@@ -52,6 +52,18 @@ class ReturnStackAllocatedMemoryConfig extends MustFlowConfiguration {
     )
   }
 
+  //  We disable flow into callables in this query as we'd otherwise get a result on this piece of code:
+  //   ```cpp
+  //  int* id(int* px) {
+  //   return px; // this returns the local variable `x`, but it's fine as the local variable isn't declared in this scope.
+  //  }
+  //  void f() {
+  //   int x;
+  //   int* px = id(&x);
+  //  }
+  //   ```
+  override predicate allowInterproceduralFlow() { none() }
+
   /**
    * This configuration intentionally conflates addresses of fields and their object, and pointer offsets
    * with their base pointer as this allows us to detect cases where an object's address flows to a
@@ -77,9 +89,6 @@ from
   ReturnStackAllocatedMemoryConfig conf
 where
   conf.hasFlowPath(pragma[only_bind_into](source), pragma[only_bind_into](sink)) and
-  source.getNode().asInstruction() = var and
-  // Only raise an alert if we're returning from the _same_ callable as the on that
-  // declared the stack variable.
-  var.getEnclosingFunction() = sink.getNode().getEnclosingCallable()
+  source.getNode().asInstruction() = var
 select sink.getNode(), source, sink, "May return stack-allocated memory from $@.", var.getAst(),
   var.getAst().toString()
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected
@@ -100,12 +100,6 @@ edges
 | test.cpp:190:10:190:13 | Unary | test.cpp:190:10:190:13 | (reference dereference) |
 | test.cpp:190:10:190:13 | Unary | test.cpp:190:10:190:13 | (reference to) |
 | test.cpp:190:10:190:13 | pRef | test.cpp:190:10:190:13 | Unary |
-| test.cpp:225:14:225:15 | px | test.cpp:226:10:226:11 | Load |
-| test.cpp:226:10:226:11 | Load | test.cpp:226:10:226:11 | px |
-| test.cpp:226:10:226:11 | px | test.cpp:226:10:226:11 | StoreValue |
-| test.cpp:231:16:231:17 | & ... | test.cpp:225:14:225:15 | px |
-| test.cpp:231:17:231:17 | Unary | test.cpp:231:16:231:17 | & ... |
-| test.cpp:231:17:231:17 | x | test.cpp:231:17:231:17 | Unary |
 nodes
 | test.cpp:17:9:17:11 | & ... | semmle.label | & ... |
 | test.cpp:17:9:17:11 | StoreValue | semmle.label | StoreValue |
@@ -221,13 +215,6 @@ nodes
 | test.cpp:190:10:190:13 | Unary | semmle.label | Unary |
 | test.cpp:190:10:190:13 | Unary | semmle.label | Unary |
 | test.cpp:190:10:190:13 | pRef | semmle.label | pRef |
-| test.cpp:225:14:225:15 | px | semmle.label | px |
-| test.cpp:226:10:226:11 | Load | semmle.label | Load |
-| test.cpp:226:10:226:11 | StoreValue | semmle.label | StoreValue |
-| test.cpp:226:10:226:11 | px | semmle.label | px |
-| test.cpp:231:16:231:17 | & ... | semmle.label | & ... |
-| test.cpp:231:17:231:17 | Unary | semmle.label | Unary |
-| test.cpp:231:17:231:17 | x | semmle.label | x |
 #select
 | test.cpp:17:9:17:11 | StoreValue | test.cpp:17:10:17:11 | mc | test.cpp:17:9:17:11 | StoreValue | May return stack-allocated memory from $@. | test.cpp:17:10:17:11 | mc | mc |
 | test.cpp:25:9:25:11 | StoreValue | test.cpp:23:18:23:19 | mc | test.cpp:25:9:25:11 | StoreValue | May return stack-allocated memory from $@. | test.cpp:23:18:23:19 | mc | mc |
diff --git a/java/ql/src/Violations of Best Practice/Naming Conventions/SameNameAsSuper.ql b/java/ql/src/Violations of Best Practice/Naming Conventions/SameNameAsSuper.ql
@@ -16,5 +16,5 @@ from RefType sub, RefType sup
 where
   sub.fromSource() and
   sup = sub.getASupertype() and
-  sub.getName() = sup.getName()
+  pragma[only_bind_out](sub.getName()) = pragma[only_bind_out](sup.getName())
 select sub, sub.getName() + " has the same name as its supertype $@.", sup, sup.getQualifiedName()
diff --git a/python/ql/test/query-tests/Security/CWE-022-TarSlip/TarSlip.expected b/python/ql/test/query-tests/Security/CWE-022-TarSlip/TarSlip.expected
@@ -7,8 +7,6 @@ edges
 | tarslip.py:40:7:40:39 | ControlFlowNode for Attribute() | tarslip.py:41:24:41:26 | ControlFlowNode for tar |
 | tarslip.py:56:7:56:39 | ControlFlowNode for Attribute() | tarslip.py:57:5:57:9 | GSSA Variable entry |
 | tarslip.py:57:5:57:9 | GSSA Variable entry | tarslip.py:59:21:59:25 | ControlFlowNode for entry |
-| tarslip.py:79:7:79:39 | ControlFlowNode for Attribute() | tarslip.py:80:5:80:9 | GSSA Variable entry |
-| tarslip.py:80:5:80:9 | GSSA Variable entry | tarslip.py:82:21:82:25 | ControlFlowNode for entry |
 nodes
 | tarslip.py:12:7:12:39 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
 | tarslip.py:13:1:13:3 | ControlFlowNode for tar | semmle.label | ControlFlowNode for tar |
@@ -23,14 +21,10 @@ nodes
 | tarslip.py:56:7:56:39 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
 | tarslip.py:57:5:57:9 | GSSA Variable entry | semmle.label | GSSA Variable entry |
 | tarslip.py:59:21:59:25 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry |
-| tarslip.py:79:7:79:39 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
-| tarslip.py:80:5:80:9 | GSSA Variable entry | semmle.label | GSSA Variable entry |
-| tarslip.py:82:21:82:25 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry |
 subpaths
 #select
 | tarslip.py:13:1:13:3 | ControlFlowNode for tar | tarslip.py:12:7:12:39 | ControlFlowNode for Attribute() | tarslip.py:13:1:13:3 | ControlFlowNode for tar | Extraction of tarfile from $@ | tarslip.py:12:7:12:39 | ControlFlowNode for Attribute() | a potentially untrusted source |
 | tarslip.py:18:17:18:21 | ControlFlowNode for entry | tarslip.py:16:7:16:39 | ControlFlowNode for Attribute() | tarslip.py:18:17:18:21 | ControlFlowNode for entry | Extraction of tarfile from $@ | tarslip.py:16:7:16:39 | ControlFlowNode for Attribute() | a potentially untrusted source |
 | tarslip.py:37:17:37:21 | ControlFlowNode for entry | tarslip.py:33:7:33:39 | ControlFlowNode for Attribute() | tarslip.py:37:17:37:21 | ControlFlowNode for entry | Extraction of tarfile from $@ | tarslip.py:33:7:33:39 | ControlFlowNode for Attribute() | a potentially untrusted source |
 | tarslip.py:41:24:41:26 | ControlFlowNode for tar | tarslip.py:40:7:40:39 | ControlFlowNode for Attribute() | tarslip.py:41:24:41:26 | ControlFlowNode for tar | Extraction of tarfile from $@ | tarslip.py:40:7:40:39 | ControlFlowNode for Attribute() | a potentially untrusted source |
 | tarslip.py:59:21:59:25 | ControlFlowNode for entry | tarslip.py:56:7:56:39 | ControlFlowNode for Attribute() | tarslip.py:59:21:59:25 | ControlFlowNode for entry | Extraction of tarfile from $@ | tarslip.py:56:7:56:39 | ControlFlowNode for Attribute() | a potentially untrusted source |
-| tarslip.py:82:21:82:25 | ControlFlowNode for entry | tarslip.py:79:7:79:39 | ControlFlowNode for Attribute() | tarslip.py:82:21:82:25 | ControlFlowNode for entry | Extraction of tarfile from $@ | tarslip.py:79:7:79:39 | ControlFlowNode for Attribute() | a potentially untrusted source |
diff --git a/ruby/ql/lib/codeql/ruby/printAst.qll b/ruby/ql/lib/codeql/ruby/printAst.qll
@@ -9,6 +9,7 @@
 private import AST
 private import codeql.ruby.Regexp as RE
 private import codeql.ruby.ast.internal.Synthesis
+private import ast.internal.AST
 
 /**
  * The query can extend this class to control which nodes are printed.
@@ -35,6 +36,8 @@ private predicate shouldPrintAstEdge(AstNode parent, string edgeName, AstNode ch
   any(PrintAstConfiguration config).shouldPrintAstEdge(parent, edgeName, child)
 }
 
+private int nonSynthIndex() { result = min([-1, any(int i | exists(getSynthChild(_, i)))]) - 1 }
+
 newtype TPrintNode =
   TPrintRegularAstNode(AstNode n) { shouldPrintNode(n) } or
   TPrintRegExpNode(RE::RegExpTerm term) {
@@ -112,13 +115,22 @@ class PrintRegularAstNode extends PrintAstNode, TPrintRegularAstNode {
     )
   }
 
+  private int getSynthAstNodeIndex() {
+    not astNode.isSynthesized() and result = nonSynthIndex()
+    or
+    astNode = getSynthChild(astNode.getParent(), result)
+  }
+
   override int getOrder() {
     this =
       rank[result](PrintRegularAstNode p, Location l, File f |
         l = p.getLocation() and
         f = l.getFile()
       |
-        p order by f.getBaseName(), f.getAbsolutePath(), l.getStartLine(), l.getStartColumn()
+        p
+        order by
+          f.getBaseName(), f.getAbsolutePath(), l.getStartLine(), l.getStartColumn(),
+          l.getEndLine(), l.getEndColumn(), p.getSynthAstNodeIndex()
       )
   }
 
diff --git a/ruby/ql/test/library-tests/ast/AstDesugar.expected b/ruby/ql/test/library-tests/ast/AstDesugar.expected

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +---
 +category: feature
 +---
 +* A new class predicate `MustFlowConfiguration::allowInterproceduralFlow` has been added to the `semmle.code.cpp.ir.dataflow.MustFlow` library. The new predicate can be overridden to disable interprocedural flow.