Merge remote-tracking branch 'upstream/main' into csharp/rework-summaries

Author: hvitved

.github/workflows/docs-review.yml

@@ -429,10 +429,11 @@
   "SSA C#": [
     "csharp/ql/src/semmle/code/csharp/dataflow/internal/SsaImplCommon.qll",
     "csharp/ql/src/semmle/code/csharp/controlflow/internal/pressa/SsaImplCommon.qll",
-    "csharp/ql/src/semmle/code/csharp/dataflow/internal/basessa/SsaImplCommon.qll"
+    "csharp/ql/src/semmle/code/csharp/dataflow/internal/basessa/SsaImplCommon.qll",
+    "csharp/ql/src/semmle/code/cil/internal/SsaImplCommon.qll"
   ],
   "CryptoAlgorithms Python/JS": [
     "javascript/ql/src/semmle/javascript/security/CryptoAlgorithms.qll",
     "python/ql/src/semmle/crypto/Crypto.qll"
   ]
-}
+}

config/identical-files.json

@@ -35,6 +35,14 @@ edges
 | test.cpp:76:12:76:17 | fgets output argument | test.cpp:78:10:78:15 | buffer |
 | test.cpp:76:12:76:17 | fgets output argument | test.cpp:79:10:79:13 | (const char *)... |
 | test.cpp:76:12:76:17 | fgets output argument | test.cpp:79:10:79:13 | data |
+| test.cpp:98:17:98:22 | buffer | test.cpp:99:15:99:20 | (const char *)... |
+| test.cpp:98:17:98:22 | buffer | test.cpp:99:15:99:20 | buffer |
+| test.cpp:98:17:98:22 | recv output argument | test.cpp:99:15:99:20 | (const char *)... |
+| test.cpp:98:17:98:22 | recv output argument | test.cpp:99:15:99:20 | buffer |
+| test.cpp:106:17:106:22 | buffer | test.cpp:107:15:107:20 | (const char *)... |
+| test.cpp:106:17:106:22 | buffer | test.cpp:107:15:107:20 | buffer |
+| test.cpp:106:17:106:22 | recv output argument | test.cpp:107:15:107:20 | (const char *)... |
+| test.cpp:106:17:106:22 | recv output argument | test.cpp:107:15:107:20 | buffer |
 nodes
 | test.cpp:24:30:24:36 | *command | semmle.label | *command |
 | test.cpp:24:30:24:36 | command | semmle.label | command |
@@ -70,10 +78,22 @@ nodes
 | test.cpp:79:10:79:13 | (const char *)... | semmle.label | (const char *)... |
 | test.cpp:79:10:79:13 | (const char *)... | semmle.label | (const char *)... |
 | test.cpp:79:10:79:13 | data | semmle.label | data |
+| test.cpp:98:17:98:22 | buffer | semmle.label | buffer |
+| test.cpp:98:17:98:22 | recv output argument | semmle.label | recv output argument |
+| test.cpp:99:15:99:20 | (const char *)... | semmle.label | (const char *)... |
+| test.cpp:99:15:99:20 | (const char *)... | semmle.label | (const char *)... |
+| test.cpp:99:15:99:20 | buffer | semmle.label | buffer |
+| test.cpp:106:17:106:22 | buffer | semmle.label | buffer |
+| test.cpp:106:17:106:22 | recv output argument | semmle.label | recv output argument |
+| test.cpp:107:15:107:20 | (const char *)... | semmle.label | (const char *)... |
+| test.cpp:107:15:107:20 | (const char *)... | semmle.label | (const char *)... |
+| test.cpp:107:15:107:20 | buffer | semmle.label | buffer |
 #select
 | test.cpp:26:10:26:16 | command | test.cpp:42:18:42:23 | call to getenv | test.cpp:26:10:26:16 | command | The value of this argument may come from $@ and is being passed to system | test.cpp:42:18:42:23 | call to getenv | call to getenv |
 | test.cpp:31:10:31:16 | command | test.cpp:43:18:43:23 | call to getenv | test.cpp:31:10:31:16 | command | The value of this argument may come from $@ and is being passed to system | test.cpp:43:18:43:23 | call to getenv | call to getenv |
 | test.cpp:62:10:62:15 | buffer | test.cpp:56:12:56:17 | buffer | test.cpp:62:10:62:15 | buffer | The value of this argument may come from $@ and is being passed to system | test.cpp:56:12:56:17 | buffer | buffer |
 | test.cpp:63:10:63:13 | data | test.cpp:56:12:56:17 | buffer | test.cpp:63:10:63:13 | data | The value of this argument may come from $@ and is being passed to system | test.cpp:56:12:56:17 | buffer | buffer |
 | test.cpp:78:10:78:15 | buffer | test.cpp:76:12:76:17 | buffer | test.cpp:78:10:78:15 | buffer | The value of this argument may come from $@ and is being passed to system | test.cpp:76:12:76:17 | buffer | buffer |
 | test.cpp:79:10:79:13 | data | test.cpp:76:12:76:17 | buffer | test.cpp:79:10:79:13 | data | The value of this argument may come from $@ and is being passed to system | test.cpp:76:12:76:17 | buffer | buffer |
+| test.cpp:99:15:99:20 | buffer | test.cpp:98:17:98:22 | buffer | test.cpp:99:15:99:20 | buffer | The value of this argument may come from $@ and is being passed to LoadLibrary | test.cpp:98:17:98:22 | buffer | buffer |
+| test.cpp:107:15:107:20 | buffer | test.cpp:106:17:106:22 | buffer | test.cpp:107:15:107:20 | buffer | The value of this argument may come from $@ and is being passed to LoadLibrary | test.cpp:106:17:106:22 | buffer | buffer |

cpp/ql/test/query-tests/Security/CWE/CWE-114/semmle/UncontrolledProcessOperation/UncontrolledProcessOperation.expected

@@ -81,3 +81,29 @@ void testReferencePointer2()
 		system(data2); // BAD [NOT DETECTED]
 	}
 }
+
+// ---
+
+typedef unsigned long size_t;
+
+void accept(int arg, char *buf, size_t *bufSize);
+void recv(int arg, char *buf, size_t bufSize);
+void LoadLibrary(const char *arg);
+
+void testAcceptRecv(int socket1, int socket2)
+{
+	{
+		char buffer[1024];
+
+		recv(socket1, buffer, 1024);
+		LoadLibrary(buffer); // BAD: using data from recv
+	}
+	
+	{
+		char buffer[1024];
+
+		accept(socket2, 0, 0);
+		recv(socket2, buffer, 1024);
+		LoadLibrary(buffer); // BAD: using data from recv
+	}
+}

cpp/ql/test/query-tests/Security/CWE/CWE-114/semmle/UncontrolledProcessOperation/test.cpp

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* A static single assignment (SSA) library has been added to the CIL analysis library. The SSA library replaces the existing `DefUse` module, which has been deprecated.

csharp/change-notes/2021-03-24-cil-ssa.md

@@ -240,6 +240,9 @@ class BasicBlock extends Cached::TBasicBlockStart {
 
   /** Gets a textual representation of this basic block. */
   string toString() { result = getFirstNode().toString() }
+
+  /** Gets the location of this basic block. */
+  Location getLocation() { result = this.getFirstNode().getLocation() }
 }
 
 /**
@@ -305,7 +308,7 @@ class EntryBasicBlock extends BasicBlock {
 }
 
 /** Holds if `bb` is an entry basic block. */
-private predicate entryBB(BasicBlock bb) { bb.getFirstNode() instanceof EntryPoint }
+private predicate entryBB(BasicBlock bb) { bb.getFirstNode() instanceof MethodImplementation }
 
 /**
  * An exit basic block, that is, a basic block whose last node is

csharp/ql/src/semmle/code/cil/BasicBlock.qll

@@ -20,3 +20,4 @@ import Attribute
 import Stubs
 import CustomModifierReceiver
 import Parameterizable
+import semmle.code.cil.Ssa

csharp/ql/src/semmle/code/cil/CIL.qll

@@ -42,7 +42,11 @@ private predicate alwaysNullExpr(Expr expr) {
   or
   alwaysNullMethod(expr.(StaticCall).getTarget())
   or
-  forex(VariableUpdate vu | DefUse::variableUpdateUse(_, vu, expr) | alwaysNullVariableUpdate(vu))
+  forex(Ssa::Definition def |
+    expr = any(Ssa::Definition def0 | def = def0.getAnUltimateDefinition()).getARead()
+  |
+    alwaysNullVariableUpdate(def.getVariableUpdate())
+  )
 }
 
 pragma[noinline]
@@ -58,7 +62,9 @@ private predicate alwaysNotNullExpr(Expr expr) {
   or
   alwaysNotNullMethod(expr.(StaticCall).getTarget())
   or
-  forex(VariableUpdate vu | DefUse::variableUpdateUse(_, vu, expr) |
-    alwaysNotNullVariableUpdate(vu)
+  forex(Ssa::Definition def |
+    expr = any(Ssa::Definition def0 | def = def0.getAnUltimateDefinition()).getARead()
+  |
+    alwaysNotNullVariableUpdate(def.getVariableUpdate())
   )
 }

csharp/ql/src/semmle/code/cil/CallableReturns.qll

@@ -9,6 +9,9 @@ class ControlFlowNode extends @cil_controlflow_node {
   /** Gets a textual representation of this control flow node. */
   string toString() { none() }
 
+  /** Gets the location of this control flow node. */
+  Location getLocation() { none() }
+
   /**
    * Gets the number of items this node pushes onto the stack.
    * This value is either 0 or 1, except for the instruction `dup`

csharp/ql/src/semmle/code/cil/ControlFlow.qll

@@ -57,12 +57,40 @@ class Untainted extends TaintType, TExactValue { }
 /** A taint type where the data is tainted. */
 class Tainted extends TaintType, TTaintedValue { }
 
+private predicate localFlowPhiInput(DataFlowNode input, Ssa::PhiNode phi) {
+  exists(Ssa::Definition def, BasicBlock bb, int i | phi.hasLastInputRef(def, bb, i) |
+    def.definesAt(_, bb, i) and
+    input = def.getVariableUpdate().getSource()
+    or
+    input =
+      any(ReadAccess ra |
+        bb.getNode(i) = ra and
+        ra.getTarget() = def.getSourceVariable()
+      )
+  )
+  or
+  exists(Ssa::PhiNode mid, BasicBlock bb, int i |
+    localFlowPhiInput(input, mid) and
+    phi.hasLastInputRef(mid, bb, i) and
+    mid.definesAt(_, bb, i)
+  )
+}
+
 private predicate localExactStep(DataFlowNode src, DataFlowNode sink) {
   src = sink.(Opcodes::Dup).getAnOperand()
   or
-  defUse(_, src, sink)
+  exists(Ssa::Definition def, VariableUpdate vu |
+    vu = def.getVariableUpdate() and
+    src = vu.getSource() and
+    sink = def.getAFirstRead()
+  )
+  or
+  any(Ssa::Definition def).hasAdjacentReads(src, sink)
   or
-  src = sink.(ParameterReadAccess).getTarget()
+  exists(Ssa::PhiNode phi |
+    localFlowPhiInput(src, phi) and
+    sink = phi.getAFirstRead()
+  )
   or
   src = sink.(Conversion).getExpr()
   or
@@ -73,12 +101,6 @@ private predicate localExactStep(DataFlowNode src, DataFlowNode sink) {
   src = sink.(Return).getExpr()
   or
   src = sink.(ConditionalBranch).getAnOperand()
-  or
-  src = sink.(MethodParameter).getAWrite()
-  or
-  exists(VariableUpdate update |
-    update.getVariable().(Parameter) = sink and src = update.getSource()
-  )
 }
 
 private predicate localTaintStep(DataFlowNode src, DataFlowNode sink) {
@@ -87,8 +109,7 @@ private predicate localTaintStep(DataFlowNode src, DataFlowNode sink) {
   src = sink.(UnaryBitwiseOperation).getOperand()
 }
 
-cached
-module DefUse {
+deprecated module DefUse {
   /**
    * A classification of variable references into reads and writes.
    */
@@ -189,7 +210,7 @@ module DefUse {
 
   /** Holds if the variable update `vu` can be used at the read `use`. */
   cached
-  predicate variableUpdateUse(StackVariable target, VariableUpdate vu, ReadAccess use) {
+  deprecated predicate variableUpdateUse(StackVariable target, VariableUpdate vu, ReadAccess use) {
     defReachesReadWithinBlock(target, vu, use)
     or
     exists(BasicBlock bb, int i |
@@ -202,23 +223,40 @@ module DefUse {
 
   /** Holds if the update `def` can be used at the read `use`. */
   cached
-  predicate defUse(StackVariable target, Expr def, ReadAccess use) {
+  deprecated predicate defUse(StackVariable target, Expr def, ReadAccess use) {
     exists(VariableUpdate vu | def = vu.getSource() | variableUpdateUse(target, vu, use))
   }
 }
 
-private import DefUse
-
-abstract library class VariableUpdate extends Instruction {
-  abstract Expr getSource();
+/** A node that updates a variable. */
+abstract class VariableUpdate extends DataFlowNode {
+  /** Gets the value assigned, if any. */
+  abstract DataFlowNode getSource();
 
+  /** Gets the variable that is updated. */
   abstract Variable getVariable();
+
+  /** Holds if this variable update happens at index `i` in basic block `bb`. */
+  abstract predicate updatesAt(BasicBlock bb, int i);
+}
+
+private class MethodParameterDef extends VariableUpdate, MethodParameter {
+  override MethodParameter getSource() { result = this }
+
+  override MethodParameter getVariable() { result = this }
+
+  override predicate updatesAt(BasicBlock bb, int i) {
+    bb.(EntryBasicBlock).getANode().getImplementation().getMethod() = this.getMethod() and
+    i = -1
+  }
 }
 
 private class VariableWrite extends VariableUpdate, WriteAccess {
-  override Expr getSource() { result = getExpr() }
+  override Expr getSource() { result = this.getExpr() }
 
-  override Variable getVariable() { result = getTarget() }
+  override Variable getVariable() { result = this.getTarget() }
+
+  override predicate updatesAt(BasicBlock bb, int i) { this = bb.getNode(i) }
 }
 
 private class MethodOutOrRefTarget extends VariableUpdate, Call {
@@ -230,5 +268,7 @@ private class MethodOutOrRefTarget extends VariableUpdate, Call {
     result = this.getRawArgument(parameterIndex).(ReadAccess).getTarget()
   }
 
-  override Expr getSource() { result = this }
+  override Expr getSource() { none() }
+
+  override predicate updatesAt(BasicBlock bb, int i) { this = bb.getNode(i) }
 }