Merge remote-tracking branch 'upstream/master' into ir-crement-load

Conflicts:
	cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ir.expected
	cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ir_unsound.expected

Author: jbj

change-notes/1.24/analysis-cpp.md

@@ -26,6 +26,10 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 
 ## Changes to libraries
 
+* The data-flow library has been improved when flow through functions needs to be
+  combined with both taint tracking and flow through fields allowing more flow
+  to be tracked. This affects and improves some security queries, which may
+  report additional results.
 * Created the `semmle.code.cpp.models.interfaces.Allocation` library to model allocation such as `new` expressions and calls to `malloc`. This in intended to replace the functionality in `semmle.code.cpp.commons.Alloc` with a more consistent and useful interface.
 * Created the `semmle.code.cpp.models.interfaces.Deallocation` library to model deallocation such as `delete` expressions and calls to `free`. This in intended to replace the functionality in `semmle.code.cpp.commons.Alloc` with a more consistent and useful interface.
 * The new class `StackVariable` should be used in place of `LocalScopeVariable`

change-notes/1.24/analysis-csharp.md

@@ -18,6 +18,7 @@ The following changes in version 1.24 affect C# analysis in all applications.
 | **Query**                    | **Expected impact**    | **Change**                        |
 |------------------------------|------------------------|-----------------------------------|
 | 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`. |
 
 ## Removal of old queries
@@ -29,6 +30,10 @@ The following changes in version 1.24 affect C# analysis in all applications.
 
 ## Changes to libraries
 
+* The data-flow library has been improved when flow through methods needs to be
+  combined with both taint tracking and flow through fields allowing more flow
+  to be tracked. This affects and improves most security queries, which may
+  report additional results.
 * The taint tracking library now tracks flow through (implicit or explicit) conversion operator calls.
 * [Code contracts](https://docs.microsoft.com/en-us/dotnet/framework/debug-trace-profile/code-contracts) are now recognized, and are treated like any other assertion methods.
 * Expression nullability flow state is given by the predicates `Expr.hasNotNullFlowState()` and `Expr.hasMaybeNullFlowState()`.

change-notes/1.24/analysis-java.md

@@ -10,10 +10,11 @@ The following changes in version 1.24 affect Java analysis in all applications.
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
-| Disabled Spring CSRF protection (`java/spring-disabled-csrf-protection`) | security, external/cwe/cwe-352 | Finds disabled Cross-Site Request Forgery (CSRF) protection in Spring. |
+| Disabled Spring CSRF protection (`java/spring-disabled-csrf-protection`) | security, external/cwe/cwe-352 | Finds disabled Cross-Site Request Forgery (CSRF) protection in Spring. Results are shown on LGTM by default. |
 | Failure to use HTTPS or SFTP URL in Maven artifact upload/download (`java/maven/non-https-url`) | security, external/cwe/cwe-300, external/cwe/cwe-319, external/cwe/cwe-494, external/cwe/cwe-829 | Finds use of insecure protocols during Maven dependency resolution. Results are shown on LGTM by default. |
+| LDAP query built from user-controlled sources (`java/ldap-injection`) | security, external/cwe/cwe-090 | Finds LDAP queries vulnerable to injection of unsanitized user-controlled input. Results are shown on LGTM by default. |
 | Left shift by more than the type width (`java/lshift-larger-than-type-width`) | correctness | Finds left shifts of ints by 32 bits or more and left shifts of longs by 64 bits or more. Results are shown on LGTM by default. |
-| Suspicious date format (`java/suspicious-date-format`) | correctness | Finds date format patterns that use placeholders that are likely to be incorrect. |
+| Suspicious date format (`java/suspicious-date-format`) | correctness | Finds date format patterns that use placeholders that are likely to be incorrect. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 
@@ -25,10 +26,17 @@ The following changes in version 1.24 affect Java analysis in all applications.
 
 ## Changes to libraries
 
+* The data-flow library has been improved when flow through methods needs to be
+  combined with both taint tracking and flow through fields allowing more flow
+  to be tracked. This affects and improves most security queries, which may
+  report additional results.
 * Identification of test classes has been improved. Previously, one of the
   match conditions would classify any class with a name containing the string
   "Test" as a test class, but now this matching has been replaced with one that
   looks for the occurrence of actual unit-test annotations. This affects the
   general file classification mechanism and thus suppression of alerts, and
   also any security queries using taint tracking, as test classes act as
   default barriers stopping taint flow.
+* Parentheses are now no longer modelled directly in the AST, that is, the
+  `ParExpr` class is empty. Instead, a parenthesized expression can be
+  identified with the `Expr.isParenthesized()` member predicate.

change-notes/1.24/analysis-javascript.md

@@ -7,7 +7,9 @@
 * Imports with the `.js` extension can now be resolved to a TypeScript file,
   when the import refers to a file generated by TypeScript.
 
-- The analysis of sanitizer guards has improved, leading to fewer false-positive results from the security queries.
+* Imports that rely on path-mappings from a `tsconfig.json` file can now be resolved.
+
+* The analysis of sanitizer guards has improved, leading to fewer false-positive results from the security queries.
 
 * Support for the following frameworks and libraries has been improved:
   - [react](https://www.npmjs.com/package/react)
@@ -18,6 +20,7 @@
   - [Socket.IO](https://socket.io/)
   - [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)
 
 ## New queries
 
@@ -39,6 +42,7 @@
 | Expression has no effect (`js/useless-expression`) | Fewer false positive results | The query now recognizes block-level flow type annotations and ignores the first statement of a try block. |
 | Use of call stack introspection in strict mode (`js/strict-mode-call-stack-introspection`) | Fewer false positive results | The query no longer flags expression statements. |
 | Missing CSRF middleware (`js/missing-token-validation`) | Fewer false positive results | The query reports fewer duplicates and only flags handlers that explicitly access cookie data. |
+| Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional ways dangerous paths can be constructed. |
 
 ## Changes to libraries
 

config/identical-files.json

@@ -190,9 +190,14 @@
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstructionImports.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstructionImports.qll"
   ],
-  "C++ SSA AliasAnalysis": [
+  "SSA AliasAnalysis": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll",
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll"
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll"
+  ],
+  "C++ SSA AliasAnalysisImports": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysisImports.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysisImports.qll"
   ],
   "C++ IR ValueNumberingImports": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/internal/ValueNumberingImports.qll",
@@ -203,6 +208,10 @@
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll",
     "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll"
   ],
+  "IR AliasConfiguration (unaliased_ssa)": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasConfiguration.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/AliasConfiguration.qll"
+  ],
   "IR SSA SSAConstruction": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll",

cpp/ql/src/semmle/code/cpp/commons/Printf.qll

@@ -258,14 +258,7 @@ class FormatLiteral extends Literal {
    * Gets the position in the string at which the nth conversion specifier
    * starts.
    */
-  int getConvSpecOffset(int n) {
-    n = 0 and result = this.getFormat().indexOf("%", 0, 0)
-    or
-    n > 0 and
-    exists(int p |
-      n = p + 1 and result = this.getFormat().indexOf("%", 0, this.getConvSpecOffset(p) + 2)
-    )
-  }
+  int getConvSpecOffset(int n) { result = this.getFormat().indexOf("%", n, 0) }
 
   /*
    * Each of these predicates gets a regular expressions to match each individual

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll

@@ -6,7 +6,6 @@ private import cpp
 private import semmle.code.cpp.dataflow.internal.FlowVar
 private import semmle.code.cpp.models.interfaces.DataFlow
 private import semmle.code.cpp.controlflow.Guards
-private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 
 cached
 private newtype TNode =
@@ -689,9 +688,9 @@ class BarrierGuard extends GuardCondition {
 
   /** Gets a node guarded by this guard. */
   final ExprNode getAGuardedNode() {
-    exists(GVN value, boolean branch |
-      result.getExpr() = value.getAnExpr() and
-      this.checks(value.getAnExpr(), branch) and
+    exists(SsaDefinition def, Variable v, boolean branch |
+      result.getExpr() = def.getAUse(v) and
+      this.checks(def.getAUse(v), branch) and
       this.controls(result.getExpr().getBasicBlock(), branch)
     )
   }

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

@@ -21,31 +21,19 @@ private predicate predictableInstruction(Instruction instr) {
   predictableInstruction(instr.(UnaryInstruction).getUnary())
 }
 
+private DataFlow::Node getNodeForSource(Expr source) {
+  isUserInput(source, _) and
+  (
+    result = DataFlow::exprNode(source)
+    or
+    result = DataFlow::definitionByReferenceNode(source)
+  )
+}
+
 private class DefaultTaintTrackingCfg extends DataFlow::Configuration {
   DefaultTaintTrackingCfg() { this = "DefaultTaintTrackingCfg" }
 
-  override predicate isSource(DataFlow::Node source) {
-    exists(CallInstruction ci, WriteSideEffectInstruction wsei |
-      userInputArgument(ci.getConvertedResultExpression(), wsei.getIndex()) and
-      source.asInstruction() = wsei and
-      wsei.getPrimaryInstruction() = ci
-    )
-    or
-    userInputReturned(source.asExpr())
-    or
-    isUserInput(source.asExpr(), _)
-    or
-    source.asExpr() instanceof EnvironmentRead
-    or
-    source
-        .asInstruction()
-        .(LoadInstruction)
-        .getSourceAddress()
-        .(VariableAddressInstruction)
-        .getASTVariable()
-        .hasName("argv") and
-    source.asInstruction().getEnclosingFunction().hasGlobalName("main")
-  }
+  override predicate isSource(DataFlow::Node source) { source = getNodeForSource(_) }
 
   override predicate isSink(DataFlow::Node sink) { any() }
 
@@ -59,7 +47,7 @@ private class DefaultTaintTrackingCfg extends DataFlow::Configuration {
 private class ToGlobalVarTaintTrackingCfg extends DataFlow::Configuration {
   ToGlobalVarTaintTrackingCfg() { this = "GlobalVarTaintTrackingCfg" }
 
-  override predicate isSource(DataFlow::Node source) { isUserInput(source.asExpr(), _) }
+  override predicate isSource(DataFlow::Node source) { source = getNodeForSource(_) }
 
   override predicate isSink(DataFlow::Node sink) {
     exists(GlobalOrNamespaceVariable gv | writesVariable(sink.asInstruction(), gv))
@@ -163,6 +151,22 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   // from `a`.
   i2.(PointerAddInstruction).getLeft() = i1
   or
+  // Until we have from through indirections across calls, we'll take flow out
+  // of the parameter and into its indirection.
+  exists(IRFunction f, Parameter parameter |
+    i1 = getInitializeParameter(f, parameter) and
+    i2 = getInitializeIndirection(f, parameter)
+  )
+  or
+  // Until we have flow through indirections across calls, we'll take flow out
+  // of the indirection and into the argument.
+  // When we get proper flow through indirections across calls, this code can be
+  // moved to `adjusedSink` or possibly into the `DataFlow::ExprNode` class.
+  exists(ReadSideEffectInstruction read |
+    read.getAnOperand().(SideEffectOperand).getAnyDef() = i1 and
+    read.getArgumentDef() = i2
+  )
+  or
   // Flow from argument to return value
   i2 =
     any(CallInstruction call |
@@ -188,6 +192,18 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
     )
 }
 
+pragma[noinline]
+private InitializeIndirectionInstruction getInitializeIndirection(IRFunction f, Parameter p) {
+  result.getParameter() = p and
+  result.getEnclosingIRFunction() = f
+}
+
+pragma[noinline]
+private InitializeParameterInstruction getInitializeParameter(IRFunction f, Parameter p) {
+  result.getParameter() = p and
+  result.getEnclosingIRFunction() = f
+}
+
 /**
  * Get an instruction that goes into argument `argumentIndex` of `call`. This
  * can be either directly or through one pointer indirection.
@@ -285,31 +301,11 @@ private Element adjustedSink(DataFlow::Node sink) {
   // For compatibility, send flow into a `NotExpr` even if it's part of a
   // short-circuiting condition and thus might get skipped.
   result.(NotExpr).getOperand() = sink.asExpr()
-  or
-  // For compatibility, send flow from argument read side effects to their
-  // corresponding argument expression
-  exists(IndirectReadSideEffectInstruction read |
-    read.getAnOperand().(SideEffectOperand).getAnyDef() = sink.asInstruction() and
-    read.getArgumentDef().getUnconvertedResultExpression() = result
-  )
-  or
-  exists(BufferReadSideEffectInstruction read |
-    read.getAnOperand().(SideEffectOperand).getAnyDef() = sink.asInstruction() and
-    read.getArgumentDef().getUnconvertedResultExpression() = result
-  )
-  or
-  exists(SizedBufferReadSideEffectInstruction read |
-    read.getAnOperand().(SideEffectOperand).getAnyDef() = sink.asInstruction() and
-    read.getArgumentDef().getUnconvertedResultExpression() = result
-  )
 }
 
 predicate tainted(Expr source, Element tainted) {
   exists(DefaultTaintTrackingCfg cfg, DataFlow::Node sink |
-    cfg.hasFlow(DataFlow::exprNode(source), sink)
-    or
-    cfg.hasFlow(DataFlow::definitionByReferenceNode(source), sink)
-  |
+    cfg.hasFlow(getNodeForSource(source), sink) and
     tainted = adjustedSink(sink)
   )
 }
@@ -322,7 +318,7 @@ predicate taintedIncludingGlobalVars(Expr source, Element tainted, string global
     ToGlobalVarTaintTrackingCfg toCfg, FromGlobalVarTaintTrackingCfg fromCfg, DataFlow::Node store,
     GlobalOrNamespaceVariable global, DataFlow::Node load, DataFlow::Node sink
   |
-    toCfg.hasFlow(DataFlow::exprNode(source), store) and
+    toCfg.hasFlow(getNodeForSource(source), store) and
     store
         .asInstruction()
         .(StoreInstruction)

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

@@ -6,6 +6,7 @@ private import cpp
 private import semmle.code.cpp.ir.IR
 private import semmle.code.cpp.controlflow.IRGuards
 private import semmle.code.cpp.ir.ValueNumbering
+private import semmle.code.cpp.models.interfaces.DataFlow
 
 /**
  * A newtype wrapper to prevent accidental casts between `Node` and
@@ -289,6 +290,51 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   // Flow through the partial operand belongs in the taint-tracking libraries
   // for now.
   iTo.getAnOperand().(ChiTotalOperand).getDef() = iFrom
+  or
+  // Flow through modeled functions
+  modelFlow(iFrom, iTo)
+}
+
+private predicate modelFlow(Instruction iFrom, Instruction iTo) {
+  exists(
+    CallInstruction call, DataFlowFunction func, FunctionInput modelIn, FunctionOutput modelOut
+  |
+    call.getStaticCallTarget() = func and
+    func.hasDataFlow(modelIn, modelOut)
+  |
+    (
+      modelOut.isReturnValue() and
+      iTo = call
+      or
+      // TODO: Add write side effects for return values
+      modelOut.isReturnValueDeref() and
+      iTo = call
+      or
+      exists(WriteSideEffectInstruction outNode |
+        modelOut.isParameterDeref(outNode.getIndex()) and
+        iTo = outNode and
+        outNode.getPrimaryInstruction() = call
+      )
+      // TODO: add write side effects for qualifiers
+    ) and
+    (
+      exists(int index |
+        modelIn.isParameter(index) and
+        iFrom = call.getPositionalArgument(index)
+      )
+      or
+      exists(int index, ReadSideEffectInstruction read |
+        modelIn.isParameterDeref(index) and
+        read.getIndex() = index and
+        read.getPrimaryInstruction() = call and
+        iFrom = read.getSideEffectOperand().getAnyDef()
+      )
+      or
+      modelIn.isQualifierAddress() and
+      iFrom = call.getThisArgument()
+      // TODO: add read side effects for qualifiers
+    )
+  )
 }
 
 /**

cpp/ql/src/semmle/code/cpp/ir/implementation/MemoryAccessKind.qll

@@ -1,6 +1,7 @@
 private newtype TMemoryAccessKind =
   TIndirectMemoryAccess() or
   TBufferMemoryAccess() or
+  TEntireAllocationMemoryAccess() or
   TEscapedMemoryAccess() or
   TNonLocalMemoryAccess() or
   TPhiMemoryAccess() or
@@ -43,6 +44,16 @@ class BufferMemoryAccess extends MemoryAccessKind, TBufferMemoryAccess {
   final override predicate usesAddressOperand() { any() }
 }
 
+/**
+ * The operand or results accesses all memory in the contiguous allocation that contains the address
+ * specified by the `AddressOperand` on the same instruction.
+ */
+class EntireAllocationMemoryAccess extends MemoryAccessKind, TEntireAllocationMemoryAccess {
+  override string toString() { result = "alloc" }
+
+  final override predicate usesAddressOperand() { any() }
+}
+
 /**
  * The operand or result accesses all memory whose address has escaped.
  */