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

Author: Dave Bartolomeo

.github/workflows/labeler.yml

@@ -13,6 +13,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| 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. |
 
 ## Changes to libraries

change-notes/1.24/analysis-cpp.md

@@ -21,6 +21,7 @@ The following changes in version 1.24 affect C# analysis in all applications.
 ## Changes to libraries
 
 * 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.
 
 ## Changes to autobuilder
 

change-notes/1.24/analysis-csharp.md

@@ -3,7 +3,7 @@
  * @description A function that uses more functions and variables from another file than functions and variables from its own file. This function might be better placed in the other file, to avoid exposing internals of the file it depends on.
  * @kind problem
  * @problem.severity recommendation
- * @precision high
+ * @precision medium
  * @id cpp/feature-envy
  * @tags maintainability
  *       modularity

cpp/ql/src/Architecture/FeatureEnvy.ql

@@ -3,7 +3,7 @@
  * @description Two files share too much information about each other (accessing many operations or variables in both directions). It would be better to invert some of the dependencies to reduce the coupling between the two files.
  * @kind problem
  * @problem.severity recommendation
- * @precision high
+ * @precision medium
  * @id cpp/file-intimacy
  * @tags maintainability
  *       modularity

cpp/ql/src/Architecture/InappropriateIntimacy.ql

@@ -3,7 +3,7 @@
  * @description Finds classes with many fields; they could probably be refactored by breaking them down into smaller classes, and using composition.
  * @kind problem
  * @problem.severity recommendation
- * @precision high
+ * @precision medium
  * @id cpp/class-many-fields
  * @tags maintainability
  *       statistical

cpp/ql/src/Architecture/Refactoring Opportunities/ClassesWithManyFields.ql

@@ -4,7 +4,7 @@
  * @kind problem
  * @problem.severity warning
  * @id cpp/japanese-era/exact-era-date
- * @precision medium
+ * @precision low
  * @tags reliability
  *       japanese-era
  */

cpp/ql/src/Best Practices/Magic Constants/JapaneseEraDate.ql

@@ -31,6 +31,12 @@ abstract class XMLLocatable extends @xmllocatable {
  * both of which can contain other elements.
  */
 class XMLParent extends @xmlparent {
+  XMLParent() {
+    // explicitly restrict `this` to be either an `XMLElement` or an `XMLFile`;
+    // the type `@xmlparent` currently also includes non-XML files
+    this instanceof @xmlelement or xmlEncoding(this, _)
+  }
+
   /**
    * Gets a printable representation of this XML parent.
    * (Intended to be overridden in subclasses.)

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

@@ -51,7 +51,11 @@ predicate allocationFunction(Function f) {
       name = "HeapReAlloc" or
       name = "VirtualAlloc" or
       name = "CoTaskMemAlloc" or
-      name = "CoTaskMemRealloc"
+      name = "CoTaskMemRealloc" or
+      name = "kmem_alloc" or
+      name = "kmem_zalloc" or
+      name = "pool_get" or
+      name = "pool_cache_get"
     )
   )
 }
@@ -77,6 +81,12 @@ predicate freeFunction(Function f, int argNum) {
       name = "free" and argNum = 0
       or
       name = "realloc" and argNum = 0
+      or
+      name = "kmem_free" and argNum = 0
+      or
+      name = "pool_put" and argNum = 1
+      or
+      name = "pool_cache_put" and argNum = 1
     )
     or
     f.hasGlobalOrStdName(name) and

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

@@ -1,6 +1,7 @@
 import cpp
 import semmle.code.cpp.security.Security
 private import semmle.code.cpp.ir.dataflow.DataFlow
+private import semmle.code.cpp.ir.dataflow.DataFlow2
 private import semmle.code.cpp.ir.IR
 private import semmle.code.cpp.ir.dataflow.internal.DataFlowDispatch as Dispatch
 
@@ -29,20 +30,60 @@ private class DefaultTaintTrackingCfg extends DataFlow::Configuration {
     instructionTaintStep(n1.asInstruction(), n2.asInstruction())
   }
 
-  override predicate isBarrier(DataFlow::Node node) {
-    exists(Variable checkedVar |
-      accessesVariable(node.asInstruction(), checkedVar) and
-      hasUpperBoundsCheck(checkedVar)
+  override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }
+}
+
+private class ToGlobalVarTaintTrackingCfg extends DataFlow::Configuration {
+  ToGlobalVarTaintTrackingCfg() { this = "GlobalVarTaintTrackingCfg" }
+
+  override predicate isSource(DataFlow::Node source) { isUserInput(source.asExpr(), _) }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(GlobalOrNamespaceVariable gv | writesVariable(sink.asInstruction(), gv))
+  }
+
+  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()
     )
   }
+
+  override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }
 }
 
-private predicate accessesVariable(CopyInstruction copy, Variable var) {
-  exists(VariableAddressInstruction va | va.getASTVariable() = var |
-    copy.(StoreInstruction).getDestinationAddress() = va
-    or
-    copy.(LoadInstruction).getSourceAddress() = va
-  )
+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)
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) { any() }
+
+  override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
+    instructionTaintStep(n1.asInstruction(), n2.asInstruction())
+  }
+
+  override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }
+}
+
+private predicate readsVariable(LoadInstruction load, Variable var) {
+  load.getSourceAddress().(VariableAddressInstruction).getASTVariable() = var
+}
+
+private predicate writesVariable(StoreInstruction store, Variable var) {
+  store.getDestinationAddress().(VariableAddressInstruction).getASTVariable() = var
 }
 
 /**
@@ -62,6 +103,13 @@ private predicate hasUpperBoundsCheck(Variable var) {
   )
 }
 
+private predicate nodeIsBarrier(DataFlow::Node node) {
+  exists(Variable checkedVar |
+    readsVariable(node.asInstruction(), checkedVar) and
+    hasUpperBoundsCheck(checkedVar)
+  )
+}
+
 private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   // Expressions computed from tainted data are also tainted
   i2 = any(CallInstruction call |
@@ -99,51 +147,73 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   // addition expression.
 }
 
+private Element adjustedSink(DataFlow::Node sink) {
+  // TODO: is it more appropriate to use asConvertedExpr here and avoid
+  // `getConversion*`? Or will that cause us to miss some cases where there's
+  // flow to a conversion (like a `ReferenceDereferenceExpr`) and we want to
+  // pretend there was flow to the converted `Expr` for the sake of
+  // compatibility.
+  sink.asExpr().getConversion*() = result
+  or
+  // For compatibility, send flow from arguments to parameters, even for
+  // functions with no body.
+  exists(FunctionCall call, int i |
+    sink.asExpr() = call.getArgument(i) and
+    result = resolveCall(call).getParameter(i)
+  )
+  or
+  // For compatibility, send flow into a `Variable` if there is flow to any
+  // Load or Store of that variable.
+  exists(CopyInstruction copy |
+    copy.getSourceValue() = sink.asInstruction() and
+    (
+      readsVariable(copy, result) or
+      writesVariable(copy, result)
+    ) and
+    not hasUpperBoundsCheck(result)
+  )
+  or
+  // 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()
+}
+
 predicate tainted(Expr source, Element tainted) {
   exists(DefaultTaintTrackingCfg cfg, DataFlow::Node sink |
     cfg.hasFlow(DataFlow::exprNode(source), sink)
   |
-    // TODO: is it more appropriate to use asConvertedExpr here and avoid
-    // `getConversion*`? Or will that cause us to miss some cases where there's
-    // flow to a conversion (like a `ReferenceDereferenceExpr`) and we want to
-    // pretend there was flow to the converted `Expr` for the sake of
-    // compatibility.
-    sink.asExpr().getConversion*() = tainted
-    or
-    // For compatibility, send flow from arguments to parameters, even for
-    // functions with no body.
-    exists(FunctionCall call, int i |
-      sink.asExpr() = call.getArgument(i) and
-      tainted = resolveCall(call).getParameter(i)
-    )
-    or
-    // For compatibility, send flow into a `Variable` if there is flow to any
-    // Load or Store of that variable.
-    exists(CopyInstruction copy |
-      copy.getSourceValue() = sink.asInstruction() and
-      accessesVariable(copy, tainted) and
-      not hasUpperBoundsCheck(tainted)
-    )
-    or
-    // For compatibility, send flow into a `NotExpr` even if it's part of a
-    // short-circuiting condition and thus might get skipped.
-    tainted.(NotExpr).getOperand() = sink.asExpr()
+    tainted = adjustedSink(sink)
   )
 }
 
 predicate taintedIncludingGlobalVars(Expr source, Element tainted, string globalVar) {
   tainted(source, tainted) and
-  // TODO: Find a way to emulate how `security.TaintTracking` reports the last
-  // global variable that taint has passed through. Also make sure we emulate
-  // its behavior for interprocedural flow through globals.
   globalVar = ""
+  or
+  exists(
+    ToGlobalVarTaintTrackingCfg toCfg, FromGlobalVarTaintTrackingCfg fromCfg, DataFlow::Node store,
+    GlobalOrNamespaceVariable global, DataFlow::Node load, DataFlow::Node sink
+  |
+    toCfg.hasFlow(DataFlow::exprNode(source), store) and
+    store
+        .asInstruction()
+        .(StoreInstruction)
+        .getDestinationAddress()
+        .(VariableAddressInstruction)
+        .getASTVariable() = global and
+    load
+        .asInstruction()
+        .(LoadInstruction)
+        .getSourceAddress()
+        .(VariableAddressInstruction)
+        .getASTVariable() = global and
+    fromCfg.hasFlow(load, sink) and
+    tainted = adjustedSink(sink) and
+    global = globalVarFromId(globalVar)
+  )
 }
 
-GlobalOrNamespaceVariable globalVarFromId(string id) {
-  // TODO: Implement this when `taintedIncludingGlobalVars` has support for
-  // global variables.
-  none()
-}
+GlobalOrNamespaceVariable globalVarFromId(string id) { id = result.getQualifiedName() }
 
 Function resolveCall(Call call) {
   exists(CallInstruction callInstruction |