Merge branch 'master' into alistairs-docs-preparation-1

Author: hubwriter

change-notes/1.24/analysis-cpp.md

@@ -19,6 +19,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 | Memory is never freed (`cpp/memory-never-freed`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
 | Memory may not be freed (`cpp/memory-may-not-be-freed`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
 | Missing return statement (`cpp/missing-return`) | Fewer false positive results | Functions containing `asm` statements are no longer highlighted by this query. |
+| No space for zero terminator (`cpp/no-space-for-terminator`) | More correct results | String arguments to formatting functions are now (usually) expected to be null terminated strings. |
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) |  | This query is no longer run on LGTM. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | This query has been modified to be more conservative when identifying which pointers point to null-terminated strings.  This approach produces fewer, more accurate results. |
 | Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | This query no longer reports incorrect results in template classes. |

change-notes/1.24/analysis-csharp.md

@@ -27,7 +27,9 @@ The following changes in version 1.24 affect C# analysis in all applications.
 ## Changes to code extraction
 
 * Tuple expressions, for example `(int,bool)` in `default((int,bool))` are now extracted correctly.
-* Expression nullability flow state is extracted. 
+* Expression nullability flow state is extracted.
+* Implicitly typed `stackalloc` expressions are now extracted correctly.
+* The difference between `stackalloc` array creations and normal array creations is extracted.
 
 ## Changes to libraries
 
@@ -38,5 +40,6 @@ The following changes in version 1.24 affect C# analysis in all applications.
 * 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()`.
+* `stackalloc` array creations are now represented by the QL class `Stackalloc`. Previously they were represented by the class `ArrayCreation`.
 
 ## Changes to autobuilder

change-notes/1.24/analysis-python.md

@@ -0,0 +1,37 @@
+# Improvements to Python analysis
+
+The following changes in version 1.24 affect Python analysis in all applications.
+
+## General improvements
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+
+## Changes to existing queries
+
+| **Query**                  | **Expected impact**    | **Change**                                                       |
+|----------------------------|------------------------|------------------------------------------------------------------|
+
+### Web framework support
+
+The QL-library support for the web frameworks Bottle, CherryPy, Falcon, Pyramid, TurboGears, Tornado, and Twisted have
+been fixed so they provide a proper HttpRequestTaintSource, instead of a TaintSource. This will enable results for the following queries:
+
+- py/path-injection
+- py/command-line-injection
+- py/reflective-xss
+- py/sql-injection
+- py/code-injection
+- py/unsafe-deserialization
+- py/url-redirection
+
+The QL-library support for the web framework Twisted have been fixed so they provide a proper
+HttpResponseTaintSink, instead of a TaintSink. This will enable results for the following
+queries:
+
+- py/reflective-xss
+- py/stack-trace-exposure
+
+## Changes to libraries

cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql

@@ -22,16 +22,25 @@ import semmle.code.cpp.models.interfaces.Allocation
 predicate terminationProblem(AllocationExpr malloc, string msg) {
   // malloc(strlen(...))
   exists(StrlenCall strlen | DataFlow::localExprFlow(strlen, malloc.getSizeExpr())) and
-  // flows into a null-terminated string function
+  // flows to a call that implies this is a null-terminated string
   exists(ArrayFunction af, FunctionCall fc, int arg |
     DataFlow::localExprFlow(malloc, fc.getArgument(arg)) and
     fc.getTarget() = af and
     (
-      // null terminated string
+      // flows into null terminated string argument
       af.hasArrayWithNullTerminator(arg)
       or
-      // likely a null terminated string (such as `strcpy`, `strcat`)
+      // flows into likely null terminated string argument (such as `strcpy`, `strcat`)
       af.hasArrayWithUnknownSize(arg)
+      or
+      // flows into string argument to a formatting function (such as `printf`)
+      exists(int n, FormatLiteral fl |
+        fc.getArgument(arg) = fc.(FormattingFunctionCall).getConversionArgument(n) and
+        fl = fc.(FormattingFunctionCall).getFormat() and
+        fl.getConversionType(n) instanceof PointerType and // `%s`, `%ws` etc
+        not fl.getConversionType(n) instanceof VoidPointerType and // exclude: `%p`
+        not fl.hasPrecision(n) // exclude: `%.*s`
+      )
     )
   ) and
   msg = "This allocation does not include space to null-terminate the string."

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

@@ -335,6 +335,12 @@ 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
+  // Taint postfix and prefix crement operations when their operand is tainted.
+  result.(CrementOperation).getAnOperand() = sink.asExpr()
+  or
+  // Taint `e1 += e2`, `e &= e2` and friends when `e1` or `e2` is tainted.
+  result.(AssignOperation).getAnOperand() = sink.asExpr()
 }
 
 predicate tainted(Expr source, Element tainted) {

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

@@ -365,10 +365,10 @@ private predicate modelFlow(Instruction iFrom, Instruction iTo) {
       modelOut.isReturnValueDeref() and
       iTo = call
       or
-      exists(WriteSideEffectInstruction outNode |
-        modelOut.isParameterDeref(outNode.getIndex()) and
+      exists(int index, WriteSideEffectInstruction outNode |
+        modelOut.isParameterDeref(index) and
         iTo = outNode and
-        outNode.getPrimaryInstruction() = call
+        outNode = getSideEffectFor(call, index)
       )
       // TODO: add write side effects for qualifiers
     ) and
@@ -380,8 +380,7 @@ private predicate modelFlow(Instruction iFrom, Instruction iTo) {
       or
       exists(int index, ReadSideEffectInstruction read |
         modelIn.isParameterDeref(index) and
-        read.getIndex() = index and
-        read.getPrimaryInstruction() = call and
+        read = getSideEffectFor(call, index) and
         iFrom = read.getSideEffectOperand().getAnyDef()
       )
       or
@@ -392,6 +391,18 @@ private predicate modelFlow(Instruction iFrom, Instruction iTo) {
   )
 }
 
+/**
+ * Holds if the result is a side effect for instruction `call` on argument
+ * index `argument`. This helper predicate makes it easy to join on both of
+ * these columns at once, avoiding pathological join orders in case the
+ * argument index should get joined first.
+ */
+pragma[noinline]
+SideEffectInstruction getSideEffectFor(CallInstruction call, int argument) {
+  call = result.getPrimaryInstruction() and
+  argument = result.(IndexedInstruction).getIndex()
+}
+
 /**
  * Holds if data flows from `source` to `sink` in zero or more local
  * (intra-procedural) steps.

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll

@@ -11,13 +11,19 @@ cached
 private newtype TOperand =
   TRegisterOperand(Instruction useInstr, RegisterOperandTag tag, Instruction defInstr) {
     defInstr = Construction::getRegisterOperandDefinition(useInstr, tag) and
-    not Construction::isInCycle(useInstr)
+    not Construction::isInCycle(useInstr) and
+    strictcount(Construction::getRegisterOperandDefinition(useInstr, tag)) = 1
   } or
   TNonPhiMemoryOperand(
     Instruction useInstr, MemoryOperandTag tag, Instruction defInstr, Overlap overlap
   ) {
     defInstr = Construction::getMemoryOperandDefinition(useInstr, tag, overlap) and
-    not Construction::isInCycle(useInstr)
+    not Construction::isInCycle(useInstr) and
+    (
+      strictcount(Construction::getMemoryOperandDefinition(useInstr, tag, _)) = 1
+      or
+      tag instanceof UnmodeledUseOperandTag
+    )
   } or
   TPhiOperand(
     PhiInstruction useInstr, Instruction defInstr, IRBlock predecessorBlock, Overlap overlap

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/internal/ValueNumberingInternal.qll

@@ -2,10 +2,10 @@ private import ValueNumberingImports
 private import cpp
 
 newtype TValueNumber =
-  TVariableAddressValueNumber(IRFunction irFunc, IRVariable var) {
-    variableAddressValueNumber(_, irFunc, var)
+  TVariableAddressValueNumber(IRFunction irFunc, Language::AST ast) {
+    variableAddressValueNumber(_, irFunc, ast)
   } or
-  TInitializeParameterValueNumber(IRFunction irFunc, IRVariable var) {
+  TInitializeParameterValueNumber(IRFunction irFunc, Language::AST var) {
     initializeParameterValueNumber(_, irFunc, var)
   } or
   TInitializeThisValueNumber(IRFunction irFunc) { initializeThisValueNumber(_, irFunc) } or
@@ -100,17 +100,23 @@ private predicate numberableInstruction(Instruction instr) {
 }
 
 private predicate variableAddressValueNumber(
-  VariableAddressInstruction instr, IRFunction irFunc, IRVariable var
+  VariableAddressInstruction instr, IRFunction irFunc, Language::AST ast
 ) {
   instr.getEnclosingIRFunction() = irFunc and
-  instr.getIRVariable() = var
+  // The underlying AST element is used as value-numbering key instead of the
+  // `IRVariable` to work around a problem where a variable or expression with
+  // multiple types gives rise to multiple `IRVariable`s.
+  instr.getIRVariable().getAST() = ast
 }
 
 private predicate initializeParameterValueNumber(
-  InitializeParameterInstruction instr, IRFunction irFunc, IRVariable var
+  InitializeParameterInstruction instr, IRFunction irFunc, Language::AST var
 ) {
   instr.getEnclosingIRFunction() = irFunc and
-  instr.getIRVariable() = var
+  // The underlying AST element is used as value-numbering key instead of the
+  // `IRVariable` to work around a problem where a variable or expression with
+  // multiple types gives rise to multiple `IRVariable`s.
+  instr.getIRVariable().getAST() = var
 }
 
 private predicate initializeThisValueNumber(InitializeThisInstruction instr, IRFunction irFunc) {
@@ -236,12 +242,12 @@ private TValueNumber nonUniqueValueNumber(Instruction instr) {
   exists(IRFunction irFunc |
     irFunc = instr.getEnclosingIRFunction() and
     (
-      exists(IRVariable var |
-        variableAddressValueNumber(instr, irFunc, var) and
-        result = TVariableAddressValueNumber(irFunc, var)
+      exists(Language::AST ast |
+        variableAddressValueNumber(instr, irFunc, ast) and
+        result = TVariableAddressValueNumber(irFunc, ast)
       )
       or
-      exists(IRVariable var |
+      exists(Language::AST var |
         initializeParameterValueNumber(instr, irFunc, var) and
         result = TInitializeParameterValueNumber(irFunc, var)
       )

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasedSSA.qll

@@ -220,9 +220,12 @@ class VariableMemoryLocation extends TVariableMemoryLocation, AllocationMemoryLo
   /**
    * Holds if this memory location covers the entire variable.
    */
-  final predicate coversEntireVariable() {
-    startBitOffset = 0 and
-    endBitOffset = var.getIRType().getByteSize() * 8
+  final predicate coversEntireVariable() { varIRTypeHasBitRange(startBitOffset, endBitOffset) }
+
+  pragma[noinline]
+  private predicate varIRTypeHasBitRange(int start, int end) {
+    start = 0 and
+    end = var.getIRType().getByteSize() * 8
   }
 }
 

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Operand.qll

@@ -11,13 +11,19 @@ cached
 private newtype TOperand =
   TRegisterOperand(Instruction useInstr, RegisterOperandTag tag, Instruction defInstr) {
     defInstr = Construction::getRegisterOperandDefinition(useInstr, tag) and
-    not Construction::isInCycle(useInstr)
+    not Construction::isInCycle(useInstr) and
+    strictcount(Construction::getRegisterOperandDefinition(useInstr, tag)) = 1
   } or
   TNonPhiMemoryOperand(
     Instruction useInstr, MemoryOperandTag tag, Instruction defInstr, Overlap overlap
   ) {
     defInstr = Construction::getMemoryOperandDefinition(useInstr, tag, overlap) and
-    not Construction::isInCycle(useInstr)
+    not Construction::isInCycle(useInstr) and
+    (
+      strictcount(Construction::getMemoryOperandDefinition(useInstr, tag, _)) = 1
+      or
+      tag instanceof UnmodeledUseOperandTag
+    )
   } or
   TPhiOperand(
     PhiInstruction useInstr, Instruction defInstr, IRBlock predecessorBlock, Overlap overlap