Merge branch 'master' of git.semmle.com:Semmle/ql into FalsySanitizer

Author: erik-krogh

change-notes/1.24/analysis-csharp.md

@@ -20,6 +20,7 @@ The following changes in version 1.24 affect C# analysis in all applications.
 | Useless assignment to local variable (`cs/useless-assignment-to-local`) | Fewer false positive results | Results have been removed when the variable is named `_` in a `foreach` statement. | 
 | Potentially dangerous use of non-short-circuit logic (`cs/non-short-circuit`) | Fewer false positive results | Results have been removed when the expression contains an `out` parameter. |
 | Dereferenced variable may be null (`cs/dereferenced-value-may-be-null`) | More results | Results are reported from parameters with a default value of `null`. |
+| Useless assignment to local variable (`cs/useless-assignment-to-local`) | Fewer false positive results | Results have been removed when the value assigned is an (implicitly or explicitly) cast default-like value. For example, `var s = (string)null` and `string s = default`. |
 
 ## Removal of old queries
 

change-notes/1.24/analysis-javascript.md

@@ -21,6 +21,9 @@
   - [ws](https://github.com/websockets/ws)
   - [WebSocket](https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API)
   - [Koa](https://www.npmjs.com/package/koa)
+  - [lazy-cache](https://www.npmjs.com/package/lazy-cache)
+  - [for-in](https://www.npmjs.com/package/for-in)
+  - [for-own](https://www.npmjs.com/package/for-own)
 
 ## New queries
 

config/identical-files.json

@@ -222,7 +222,12 @@
     "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/PrintSSA.qll",
     "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/PrintSSA.qll"
   ],
-  "IR ValueNumber": [
+  "C++ IR ValueNumberInternal": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/internal/ValueNumberingInternal.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/internal/ValueNumberingInternal.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/internal/ValueNumberingInternal.qll"
+  ],
+  "C++ IR ValueNumber": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/ValueNumbering.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll",

cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -21,6 +21,40 @@ private predicate predictableInstruction(Instruction instr) {
   predictableInstruction(instr.(UnaryInstruction).getUnary())
 }
 
+/**
+ * Functions that we should only allow taint to flow through (to the return
+ * value) if all but the source argument are 'predictable'.  This is done to
+ * emulate the old security library's implementation rather than due to any
+ * strong belief that this is the right approach.
+ *
+ * Note that the list itself is not very principled; it consists of all the
+ * functions listed in the old security library's [default] `isPureFunction`
+ * that have more than one argument, but are not in the old taint tracking
+ * library's `returnArgument` predicate.  In addition, `strlen` is included
+ * because it's also a special case in flow to return values.
+ */
+predicate predictableOnlyFlow(string name) {
+  name = "strcasestr" or
+  name = "strchnul" or
+  name = "strchr" or
+  name = "strchrnul" or
+  name = "strcmp" or
+  name = "strcspn" or
+  name = "strlen" or // special case
+  name = "strncmp" or
+  name = "strndup" or
+  name = "strnlen" or
+  name = "strrchr" or
+  name = "strspn" or
+  name = "strstr" or
+  name = "strtod" or
+  name = "strtof" or
+  name = "strtol" or
+  name = "strtoll" or
+  name = "strtoq" or
+  name = "strtoul"
+}
+
 private DataFlow::Node getNodeForSource(Expr source) {
   isUserInput(source, _) and
   (
@@ -35,7 +69,7 @@ private class DefaultTaintTrackingCfg extends DataFlow::Configuration {
 
   override predicate isSource(DataFlow::Node source) { source = getNodeForSource(_) }
 
-  override predicate isSink(DataFlow::Node sink) { any() }
+  override predicate isSink(DataFlow::Node sink) { exists(adjustedSink(sink)) }
 
   override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
     instructionTaintStep(n1.asInstruction(), n2.asInstruction())
@@ -50,18 +84,15 @@ private class ToGlobalVarTaintTrackingCfg extends DataFlow::Configuration {
   override predicate isSource(DataFlow::Node source) { source = getNodeForSource(_) }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists(GlobalOrNamespaceVariable gv | writesVariable(sink.asInstruction(), gv))
+    sink.asVariable() instanceof GlobalOrNamespaceVariable
   }
 
   override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
     instructionTaintStep(n1.asInstruction(), n2.asInstruction())
     or
-    exists(StoreInstruction i1, LoadInstruction i2, GlobalOrNamespaceVariable gv |
-      writesVariable(i1, gv) and
-      readsVariable(i2, gv) and
-      i1 = n1.asInstruction() and
-      i2 = n2.asInstruction()
-    )
+    writesVariable(n1.asInstruction(), n2.asVariable().(GlobalOrNamespaceVariable))
+    or
+    readsVariable(n2.asInstruction(), n1.asVariable().(GlobalOrNamespaceVariable))
   }
 
   override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }
@@ -71,19 +102,20 @@ private class FromGlobalVarTaintTrackingCfg extends DataFlow2::Configuration {
   FromGlobalVarTaintTrackingCfg() { this = "FromGlobalVarTaintTrackingCfg" }
 
   override predicate isSource(DataFlow::Node source) {
-    exists(
-      ToGlobalVarTaintTrackingCfg other, DataFlow::Node prevSink, GlobalOrNamespaceVariable gv
-    |
-      other.hasFlowTo(prevSink) and
-      writesVariable(prevSink.asInstruction(), gv) and
-      readsVariable(source.asInstruction(), gv)
-    )
+    // This set of sources should be reasonably small, which is good for
+    // performance since the set of sinks is very large.
+    exists(ToGlobalVarTaintTrackingCfg otherCfg | otherCfg.hasFlowTo(source))
   }
 
-  override predicate isSink(DataFlow::Node sink) { any() }
+  override predicate isSink(DataFlow::Node sink) { exists(adjustedSink(sink)) }
 
   override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
     instructionTaintStep(n1.asInstruction(), n2.asInstruction())
+    or
+    // Additional step for flow out of variables. There is no flow _into_
+    // variables in this configuration, so this step only serves to take flow
+    // out of a variable that's a source.
+    readsVariable(n2.asInstruction(), n1.asVariable())
   }
 
   override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }
@@ -123,15 +155,16 @@ private predicate nodeIsBarrier(DataFlow::Node node) {
 
 private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   // Expressions computed from tainted data are also tainted
-  i2 =
-    any(CallInstruction call |
-      isPureFunction(call.getStaticCallTarget().getName()) and
-      call.getAnArgument() = i1 and
-      forall(Instruction arg | arg = call.getAnArgument() | arg = i1 or predictableInstruction(arg)) and
-      // flow through `strlen` tends to cause dubious results, if the length is
-      // bounded.
-      not call.getStaticCallTarget().getName() = "strlen"
-    )
+  exists(CallInstruction call, int argIndex | call = i2 |
+    isPureFunction(call.getStaticCallTarget().getName()) and
+    i1 = getACallArgumentOrIndirection(call, argIndex) and
+    forall(Instruction arg | arg = call.getAnArgument() |
+      arg = getACallArgumentOrIndirection(call, argIndex) or predictableInstruction(arg)
+    ) and
+    // flow through `strlen` tends to cause dubious results, if the length is
+    // bounded.
+    not call.getStaticCallTarget().getName() = "strlen"
+  )
   or
   // Flow through pointer dereference
   i2.(LoadInstruction).getSourceAddress() = i1
@@ -172,7 +205,8 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
     any(CallInstruction call |
       exists(int indexIn |
         modelTaintToReturnValue(call.getStaticCallTarget(), indexIn) and
-        i1 = getACallArgumentOrIndirection(call, indexIn)
+        i1 = getACallArgumentOrIndirection(call, indexIn) and
+        not predictableOnlyFlow(call.getStaticCallTarget().getName())
       )
     )
   or
@@ -315,23 +349,12 @@ predicate taintedIncludingGlobalVars(Expr source, Element tainted, string global
   globalVar = ""
   or
   exists(
-    ToGlobalVarTaintTrackingCfg toCfg, FromGlobalVarTaintTrackingCfg fromCfg, DataFlow::Node store,
-    GlobalOrNamespaceVariable global, DataFlow::Node load, DataFlow::Node sink
+    ToGlobalVarTaintTrackingCfg toCfg, FromGlobalVarTaintTrackingCfg fromCfg,
+    DataFlow::VariableNode variableNode, GlobalOrNamespaceVariable global, DataFlow::Node sink
   |
-    toCfg.hasFlow(getNodeForSource(source), store) and
-    store
-        .asInstruction()
-        .(StoreInstruction)
-        .getDestinationAddress()
-        .(VariableAddressInstruction)
-        .getASTVariable() = global and
-    load
-        .asInstruction()
-        .(LoadInstruction)
-        .getSourceAddress()
-        .(VariableAddressInstruction)
-        .getASTVariable() = global and
-    fromCfg.hasFlow(load, sink) and
+    global = variableNode.getVariable() and
+    toCfg.hasFlow(getNodeForSource(source), variableNode) and
+    fromCfg.hasFlow(variableNode, sink) and
     tainted = adjustedSink(sink) and
     global = globalVarFromId(globalVar)
   )

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll

@@ -7,17 +7,17 @@ private import DataFlowDispatch
  * A data flow node that occurs as the argument of a call and is passed as-is
  * to the callable. Instance arguments (`this` pointer) are also included.
  */
-class ArgumentNode extends Node {
-  ArgumentNode() { exists(CallInstruction call | this.asInstruction() = call.getAnArgument()) }
+class ArgumentNode extends InstructionNode {
+  ArgumentNode() { exists(CallInstruction call | this.getInstruction() = call.getAnArgument()) }
 
   /**
    * Holds if this argument occurs at the given position in the given call.
    * The instance argument is considered to have index `-1`.
    */
   predicate argumentOf(DataFlowCall call, int pos) {
-    this.asInstruction() = call.getPositionalArgument(pos)
+    this.getInstruction() = call.getPositionalArgument(pos)
     or
-    this.asInstruction() = call.getThisArgument() and pos = -1
+    this.getInstruction() = call.getThisArgument() and pos = -1
   }
 
   /** Gets the call in which this node is an argument. */
@@ -36,15 +36,15 @@ class ReturnKind extends TReturnKind {
 }
 
 /** A data flow node that occurs as the result of a `ReturnStmt`. */
-class ReturnNode extends Node {
-  ReturnNode() { exists(ReturnValueInstruction ret | this.asInstruction() = ret.getReturnValue()) }
+class ReturnNode extends InstructionNode {
+  ReturnNode() { exists(ReturnValueInstruction ret | this.getInstruction() = ret.getReturnValue()) }
 
   /** Gets the kind of this returned value. */
   ReturnKind getKind() { result = TNormalReturnKind() }
 }
 
 /** A data flow node that represents the output of a call. */
-class OutNode extends Node {
+class OutNode extends InstructionNode {
   override CallInstruction instr;
 
   /** Gets the underlying call. */
@@ -181,11 +181,17 @@ private predicate suppressUnusedType(Type t) { any() }
 // Java QL library compatibility wrappers
 //////////////////////////////////////////////////////////////////////////////
 /** A node that performs a type cast. */
-class CastNode extends Node {
+class CastNode extends InstructionNode {
   CastNode() { none() } // stub implementation
 }
 
-class DataFlowCallable = Function;
+/**
+ * A function that may contain code or a variable that may contain itself. When
+ * flow crosses from one _enclosing callable_ to another, the interprocedural
+ * data-flow library discards call contexts and inserts a node in the big-step
+ * relation used for human-readable path explanations.
+ */
+class DataFlowCallable = Declaration;
 
 class DataFlowExpr = Expr;