Merge remote-tracking branch 'upstream/master' into dbartol/Indirections

Author: Dave Bartolomeo

change-notes/1.24/analysis-cpp.md

@@ -40,4 +40,4 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 * The taint tracking library (`semmle.code.cpp.dataflow.TaintTracking`) has had
   the following improvements:
   * The library now models data flow through `strdup` and similar functions.
-  
+  * The library now models data flow through formatting functions such as `sprintf`.

change-notes/1.24/analysis-csharp.md

@@ -6,8 +6,12 @@ The following changes in version 1.24 affect C# analysis in all applications.
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
+| Assembly path injection (`cs/assembly-path-injection`) | security, external/cwe/cwe-114 | Finds user-controlled data used to load an assembly. |
 | Insecure configuration for ASP.NET requestValidationMode (`cs/insecure-request-validation-mode`) | security, external/cwe/cwe-016 | Finds where this attribute has been set to a value less than 4.5, which turns off some validation features and makes the application less secure. |
-| Page request validation is disabled (`cs/web/request-validation-disabled`) | security, frameworks/asp.net, external/cwe/cwe-016 | Finds where ASP.NET page request validation has been disabled, which could makes the application less secure. |
+| Insecure SQL connection (`cs/insecure-sql-connection`) | security, external/cwe/cwe-327 | Finds unencrypted SQL connection strings. |
+| Page request validation is disabled (`cs/web/request-validation-disabled`) | security, frameworks/asp.net, external/cwe/cwe-016 | Finds where ASP.NET page request validation has been disabled, which could make the application less secure. |
+| Serialization check bypass (`cs/serialization-check-bypass`) | security, external/cwe/cwe-20 | Finds where data is not validated in a deserialization method. |
+| XML injection (`cs/xml-injection`) | security, external/cwe/cwe-091 | Finds user-controlled data that is used to write directly to an XML document. |
 
 ## Changes to existing queries
 
@@ -30,4 +34,3 @@ The following changes in version 1.24 affect C# analysis in all applications.
 * Expression nullability flow state is given by the predicates `Expr.hasNotNullFlowState()` and `Expr.hasMaybeNullFlowState()`.
 
 ## Changes to autobuilder
-

change-notes/1.24/analysis-javascript.md

@@ -38,6 +38,7 @@
 | Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | This query now recognizes additional ways event handler receivers can be bound. | 
 | 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. |
 
 ## Changes to libraries
 

cpp/ql/src/Critical/DescriptorNeverClosed.qhelp

@@ -6,15 +6,15 @@
 
 <overview>
 <p>
-This rule finds calls to <code>open</code> or <code>socket</code> where there is no corresponding <code>close</code> call in the program analyzed.
+This rule finds calls to <code>socket</code> where there is no corresponding <code>close</code> call in the program analyzed.
 Leaving descriptors open will cause a resource leak that will persist even after the program terminates.
 </p>
 
 <include src="aliasAnalysisWarning.qhelp" />
 </overview>
 
 <recommendation>
-<p>Ensure that all file or socket descriptors allocated by the program are freed before it terminates.</p>
+<p>Ensure that all socket descriptors allocated by the program are freed before it terminates.</p>
 </recommendation>
 
 <example>

cpp/ql/src/Critical/DescriptorNeverClosed.ql

@@ -1,6 +1,6 @@
 /**
  * @name Open descriptor never closed
- * @description Functions that always return before closing the socket or file they opened leak resources.
+ * @description Functions that always return before closing the socket they opened leak resources.
  * @kind problem
  * @id cpp/descriptor-never-closed
  * @problem.severity warning

cpp/ql/src/semmle/code/cpp/Parameter.qll

@@ -163,5 +163,8 @@ class Parameter extends LocalScopeVariable, @parameter {
  * An `int` that is a parameter index for some function.  This is needed for binding in certain cases.
  */
 class ParameterIndex extends int {
-  ParameterIndex() { exists(Parameter p | this = p.getIndex()) }
+  ParameterIndex() {
+    exists(Parameter p | this = p.getIndex()) or
+    exists(Call c | exists(c.getArgument(this))) // permit indexing varargs
+  }
 }

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

@@ -149,6 +149,9 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   or
   i2.(UnaryInstruction).getUnary() = i1
   or
+  i2.(ChiInstruction).getPartial() = i1 and
+  not isChiForAllAliasedMemory(i2)
+  or
   exists(BinaryInstruction bin |
     bin = i2 and
     predictableInstruction(i2.getAnOperand().getDef()) and
@@ -199,12 +202,29 @@ private Instruction getACallArgumentOrIndirection(CallInstruction call, int argu
 
 private predicate modelTaintToParameter(Function f, int parameterIn, int parameterOut) {
   exists(FunctionInput modelIn, FunctionOutput modelOut |
-    f.(TaintFunction).hasTaintFlow(modelIn, modelOut) and
+    (
+      f.(DataFlowFunction).hasDataFlow(modelIn, modelOut)
+      or
+      f.(TaintFunction).hasTaintFlow(modelIn, modelOut)
+    ) and
     (modelIn.isParameter(parameterIn) or modelIn.isParameterDeref(parameterIn)) and
     modelOut.isParameterDeref(parameterOut)
   )
 }
 
+/**
+ * Holds if `chi` is on the chain of chi-instructions for all aliased memory.
+ * Taint shoud not pass through these instructions since they tend to mix up
+ * unrelated objects.
+ */
+private predicate isChiForAllAliasedMemory(Instruction instr) {
+  instr.(ChiInstruction).getTotal() instanceof AliasedDefinitionInstruction
+  or
+  isChiForAllAliasedMemory(instr.(ChiInstruction).getTotal())
+  or
+  isChiForAllAliasedMemory(instr.(PhiInstruction).getAnInput())
+}
+
 private predicate modelTaintToReturnValue(Function f, int parameterIn) {
   // Taint flow from parameter to return value
   exists(FunctionInput modelIn, FunctionOutput modelOut |

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

@@ -264,11 +264,17 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
 }
 
 private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction iTo) {
-  iTo.(CopyInstruction).getSourceValue() = iFrom or
-  iTo.(PhiInstruction).getAnOperand().getDef() = iFrom or
+  iTo.(CopyInstruction).getSourceValue() = iFrom
+  or
+  iTo.(PhiInstruction).getAnOperand().getDef() = iFrom
+  or
   // Treat all conversions as flow, even conversions between different numeric types.
-  iTo.(ConvertInstruction).getUnary() = iFrom or
-  iTo.(InheritanceConversionInstruction).getUnary() = iFrom or
+  iTo.(ConvertInstruction).getUnary() = iFrom
+  or
+  iTo.(CheckedConvertOrNullInstruction).getUnary() = iFrom
+  or
+  iTo.(InheritanceConversionInstruction).getUnary() = iFrom
+  or
   // A chi instruction represents a point where a new value (the _partial_
   // operand) may overwrite an old value (the _total_ operand), but the alias
   // analysis couldn't determine that it surely will overwrite every bit of it or
@@ -278,10 +284,8 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   // due to shortcomings of the alias analysis. We may get false flow in cases
   // where the data is indeed overwritten.
   //
-  // Allowing flow through the partial operand would be more noisy, especially
-  // for variables that have escaped: for soundness, the IR has to assume that
-  // every write to an unknown address can affect every escaped variable, and
-  // this assumption shows up as data flowing through partial chi operands.
+  // Flow through the partial operand belongs in the taint-tracking libraries
+  // for now.
   iTo.getAnOperand().(ChiTotalOperand).getDef() = iFrom
 }
 

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

@@ -947,6 +947,10 @@ class ConvertInstruction extends UnaryInstruction {
   ConvertInstruction() { getOpcode() instanceof Opcode::Convert }
 }
 
+class CheckedConvertOrNullInstruction extends UnaryInstruction {
+  CheckedConvertOrNullInstruction() { getOpcode() instanceof Opcode::CheckedConvertOrNull }
+}
+
 /**
  * Represents an instruction that converts between two addresses
  * related by inheritance.
@@ -987,7 +991,7 @@ class InheritanceConversionInstruction extends UnaryInstruction {
 
 /**
  * Represents an instruction that converts from the address of a derived class
- * to the address of a direct non-virtual base class.
+ * to the address of a base class.
  */
 class ConvertToBaseInstruction extends InheritanceConversionInstruction {
   ConvertToBaseInstruction() { getOpcode() instanceof ConvertToBaseOpcode }

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

@@ -84,6 +84,10 @@ private predicate operandIsPropagated(Operand operand, IntValue bitOffset) {
         bitOffset = Ints::mul(convert.getDerivation().getByteOffset(), 8)
       )
       or
+      // Conversion using dynamic_cast results in an unknown offset
+      instr instanceof CheckedConvertOrNullInstruction and
+      bitOffset = Ints::unknown()
+      or
       // Converting to a derived class subtracts the offset of the base class.
       exists(ConvertToDerivedInstruction convert |
         convert = instr and