Merge branch 'main' into pathinjectionsinks

Author: geoffw0

cpp/ql/lib/change-notes/2023-11-08-strsafe-models.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Added models for the `sprintf` variants from the `StrSafe.h` header.

cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll

@@ -31,6 +31,11 @@ abstract class MustFlowConfiguration extends string {
    */
   abstract predicate isSink(Operand sink);
 
+  /**
+   * Holds if data flow through `instr` is prohibited.
+   */
+  predicate isBarrier(Instruction instr) { none() }
+
   /**
    * Holds if the additional flow step from `node1` to `node2` must be taken
    * into account in the analysis.
@@ -48,18 +53,21 @@ abstract class MustFlowConfiguration extends string {
    */
   final predicate hasFlowPath(MustFlowPathNode source, MustFlowPathSink sink) {
     this.isSource(source.getInstruction()) and
-    source.getASuccessor+() = sink
+    source.getASuccessor*() = sink
   }
 }
 
 /** Holds if `node` flows from a source. */
 pragma[nomagic]
 private predicate flowsFromSource(Instruction node, MustFlowConfiguration config) {
-  config.isSource(node)
-  or
-  exists(Instruction mid |
-    step(mid, node, config) and
-    flowsFromSource(mid, pragma[only_bind_into](config))
+  not config.isBarrier(node) and
+  (
+    config.isSource(node)
+    or
+    exists(Instruction mid |
+      step(mid, node, config) and
+      flowsFromSource(mid, pragma[only_bind_into](config))
+    )
   )
 }
 

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

@@ -632,20 +632,20 @@ predicate jumpStep(Node n1, Node n2) {
       v = globalUse.getVariable() and
       n1.(FinalGlobalValue).getGlobalUse() = globalUse
     |
-      globalUse.getIndirectionIndex() = 1 and
+      globalUse.getIndirection() = 1 and
       v = n2.asVariable()
       or
-      v = n2.asIndirectVariable(globalUse.getIndirectionIndex())
+      v = n2.asIndirectVariable(globalUse.getIndirection())
     )
     or
     exists(Ssa::GlobalDef globalDef |
       v = globalDef.getVariable() and
       n2.(InitialGlobalValue).getGlobalDef() = globalDef
     |
-      globalDef.getIndirectionIndex() = 1 and
+      globalDef.getIndirection() = 1 and
       v = n1.asVariable()
       or
-      v = n1.asIndirectVariable(globalDef.getIndirectionIndex())
+      v = n1.asIndirectVariable(globalDef.getIndirection())
     )
   )
 }

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll

@@ -113,22 +113,12 @@ private newtype TDefOrUseImpl =
   TGlobalUse(GlobalLikeVariable v, IRFunction f, int indirectionIndex) {
     // Represents a final "use" of a global variable to ensure that
     // the assignment to a global variable isn't ruled out as dead.
-    exists(VariableAddressInstruction vai, int defIndex |
-      vai.getEnclosingIRFunction() = f and
-      vai.getAstVariable() = v and
-      isDef(_, _, _, vai, _, defIndex) and
-      indirectionIndex = [0 .. defIndex] + 1
-    )
+    isGlobalUse(v, f, _, indirectionIndex)
   } or
   TGlobalDefImpl(GlobalLikeVariable v, IRFunction f, int indirectionIndex) {
     // Represents the initial "definition" of a global variable when entering
     // a function body.
-    exists(VariableAddressInstruction vai |
-      vai.getEnclosingIRFunction() = f and
-      vai.getAstVariable() = v and
-      isUse(_, _, vai, _, indirectionIndex) and
-      not isDef(_, _, vai.getAUse(), _, _, _)
-    )
+    isGlobalDefImpl(v, f, _, indirectionIndex)
   } or
   TIteratorDef(
     Operand iteratorDerefAddress, BaseSourceVariableInstruction container, int indirectionIndex
@@ -150,6 +140,27 @@ private newtype TDefOrUseImpl =
     )
   }
 
+private predicate isGlobalUse(
+  GlobalLikeVariable v, IRFunction f, int indirection, int indirectionIndex
+) {
+  exists(VariableAddressInstruction vai |
+    vai.getEnclosingIRFunction() = f and
+    vai.getAstVariable() = v and
+    isDef(_, _, _, vai, indirection, indirectionIndex)
+  )
+}
+
+private predicate isGlobalDefImpl(
+  GlobalLikeVariable v, IRFunction f, int indirection, int indirectionIndex
+) {
+  exists(VariableAddressInstruction vai |
+    vai.getEnclosingIRFunction() = f and
+    vai.getAstVariable() = v and
+    isUse(_, _, vai, indirection, indirectionIndex) and
+    not isDef(_, _, _, vai, _, indirectionIndex)
+  )
+}
+
 private predicate unspecifiedTypeIsModifiableAt(Type unspecified, int indirectionIndex) {
   indirectionIndex = [1 .. getIndirectionForUnspecifiedType(unspecified).getNumberOfIndirections()] and
   exists(CppType cppType |
@@ -438,7 +449,7 @@ class GlobalUse extends UseImpl, TGlobalUse {
 
   override FinalGlobalValue getNode() { result.getGlobalUse() = this }
 
-  override int getIndirection() { result = ind + 1 }
+  override int getIndirection() { isGlobalUse(global, f, result, ind) }
 
   /** Gets the global variable associated with this use. */
   GlobalLikeVariable getVariable() { result = global }
@@ -460,7 +471,9 @@ class GlobalUse extends UseImpl, TGlobalUse {
     )
   }
 
-  override SourceVariable getSourceVariable() { sourceVariableIsGlobal(result, global, f, ind) }
+  override SourceVariable getSourceVariable() {
+    sourceVariableIsGlobal(result, global, f, this.getIndirection())
+  }
 
   final override Cpp::Location getLocation() { result = f.getLocation() }
 
@@ -501,16 +514,18 @@ class GlobalDefImpl extends DefOrUseImpl, TGlobalDefImpl {
 
   /** Gets the global variable associated with this definition. */
   override SourceVariable getSourceVariable() {
-    sourceVariableIsGlobal(result, global, f, indirectionIndex)
+    sourceVariableIsGlobal(result, global, f, this.getIndirection())
   }
 
+  int getIndirection() { result = indirectionIndex }
+
   /**
    * Gets the type of this use after specifiers have been deeply stripped
    * and typedefs have been resolved.
    */
   Type getUnspecifiedType() { result = global.getUnspecifiedType() }
 
-  override string toString() { result = "GlobalDef" }
+  override string toString() { result = "Def of " + this.getSourceVariable() }
 
   override Location getLocation() { result = f.getLocation() }
 
@@ -980,7 +995,7 @@ class GlobalDef extends TGlobalDef, SsaDefOrUse {
   final override Location getLocation() { result = global.getLocation() }
 
   /** Gets a textual representation of this definition. */
-  override string toString() { result = "GlobalDef" }
+  override string toString() { result = global.toString() }
 
   /**
    * Holds if this definition has index `index` in block `block`, and
@@ -990,6 +1005,9 @@ class GlobalDef extends TGlobalDef, SsaDefOrUse {
     global.hasIndexInBlock(block, index, sv)
   }
 
+  /** Gets the indirection index of this definition. */
+  int getIndirection() { result = global.getIndirection() }
+
   /** Gets the indirection index of this definition. */
   int getIndirectionIndex() { result = global.getIndirectionIndex() }
 

cpp/ql/lib/semmle/code/cpp/models/implementations/Gets.qll

@@ -49,10 +49,11 @@ private class FgetsFunction extends DataFlowFunction, TaintFunction, ArrayFuncti
   }
 
   override predicate hasRemoteFlowSource(FunctionOutput output, string description) {
-    output.isParameterDeref(0) and
-    description = "string read by " + this.getName()
-    or
-    output.isReturnValue() and
+    (
+      output.isParameterDeref(0) or
+      output.isReturnValue() or
+      output.isReturnValueDeref()
+    ) and
     description = "string read by " + this.getName()
   }
 

cpp/ql/lib/semmle/code/cpp/models/implementations/Inet.qll

@@ -157,7 +157,7 @@ private class Getaddrinfo extends TaintFunction, ArrayFunction, RemoteFlowSource
   override predicate hasArrayWithNullTerminator(int bufParam) { bufParam in [0, 1] }
 
   override predicate hasRemoteFlowSource(FunctionOutput output, string description) {
-    output.isParameterDeref(3) and
+    output.isParameterDeref(3, 2) and
     description = "address returned by " + this.getName()
   }
 }

cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll

@@ -147,19 +147,32 @@ private class SnprintfImpl extends Snprintf {
 
 /**
  * The Microsoft `StringCchPrintf` function and variants.
+ * See: https://learn.microsoft.com/en-us/windows/win32/api/strsafe/
+ *      and
+ *      https://learn.microsoft.com/en-us/previous-versions/windows/embedded/ms860435(v=msdn.10)
  */
 private class StringCchPrintf extends FormattingFunction {
   StringCchPrintf() {
     this instanceof TopLevelFunction and
-    this.hasGlobalName([
-        "StringCchPrintf", "StringCchPrintfEx", "StringCchPrintf_l", "StringCchPrintf_lEx",
-        "StringCbPrintf", "StringCbPrintfEx", "StringCbPrintf_l", "StringCbPrintf_lEx"
-      ]) and
+    exists(string baseName |
+      baseName in [
+          "StringCchPrintf", //StringCchPrintf(pszDest, cchDest, pszFormat, ...)
+          "StringCchPrintfEx", //StringCchPrintfEx(pszDest,cchDest, ppszDestEnd, pcchRemaining, dwFlags, pszFormat, ...)
+          "StringCchPrintf_l", //StringCchPrintf_l(pszDest, cbDest, pszFormat, locale, ...)
+          "StringCchPrintf_lEx", //StringCchPrintf_lEx(pszDest, cchDest, ppszDestEnd, pcchRemaining, dwFlags, pszFormat, locale, ...)
+          "StringCbPrintf", //StringCbPrintf(pszDest, cbDest, pszFormat, ...)
+          "StringCbPrintfEx", //StringCbPrintfEx(pszDest, cbDest, ppszDestEnd, pcbRemaining, dwFlags, pszFormat, ...)
+          "StringCbPrintf_l", //StringCbPrintf_l(pszDest, cbDest, pszFormat, locale, ...)
+          "StringCbPrintf_lEx" //StringCbPrintf_lEx(pszDest, cbDest, ppszDestEnd, pcbRemaining, dwFlags, pszFormat, locale, ...)
+        ]
+    |
+      this.hasGlobalName(baseName + ["", "A", "W"])
+    ) and
     not exists(this.getDefinition().getFile().getRelativePath())
   }
 
   override int getFormatParameterIndex() {
-    if this.getName().matches("%Ex") then result = 5 else result = 2
+    if this.getName().matches("%Ex" + ["", "A", "W"]) then result = 5 else result = 2
   }
 
   override int getOutputParameterIndex(boolean isStream) { result = 0 and isStream = false }

cpp/ql/lib/semmle/code/cpp/models/implementations/Send.qll

@@ -58,7 +58,7 @@ private class Send extends AliasFunction, ArrayFunction, SideEffectFunction, Rem
   override ParameterIndex getParameterSizeIndex(ParameterIndex i) { i = 1 and result = 2 }
 
   override predicate hasRemoteFlowSink(FunctionInput input, string description) {
-    input.isParameterDeref(1) and description = "buffer sent by " + this.getName()
+    input.isParameterDeref(1, 1) and description = "buffer sent by " + this.getName()
   }
 
   override predicate hasSocketInput(FunctionInput input) { input.isParameter(0) }

cpp/ql/lib/semmle/code/cpp/models/interfaces/FunctionInputsAndOutputs.qll

@@ -8,7 +8,7 @@ import semmle.code.cpp.Parameter
 
 private newtype TFunctionInput =
   TInParameter(ParameterIndex i) or
-  TInParameterDeref(ParameterIndex i) or
+  TInParameterDeref(ParameterIndex i, int indirectionIndex) { indirectionIndex = [1, 2] } or
   TInQualifierObject() or
   TInQualifierAddress() or
   TInReturnValueDeref()
@@ -245,15 +245,18 @@ class InParameter extends FunctionInput, TInParameter {
  */
 class InParameterDeref extends FunctionInput, TInParameterDeref {
   ParameterIndex index;
+  int indirectionIndex;
 
-  InParameterDeref() { this = TInParameterDeref(index) }
+  InParameterDeref() { this = TInParameterDeref(index, indirectionIndex) }
 
   override string toString() { result = "InParameterDeref " + index.toString() }
 
   /** Gets the zero-based index of the parameter. */
   ParameterIndex getIndex() { result = index }
 
-  override predicate isParameterDeref(ParameterIndex i) { i = index }
+  override predicate isParameterDeref(ParameterIndex i, int indirection) {
+    i = index and indirectionIndex = indirection
+  }
 }
 
 /**
@@ -321,10 +324,10 @@ class InReturnValueDeref extends FunctionInput, TInReturnValueDeref {
 }
 
 private newtype TFunctionOutput =
-  TOutParameterDeref(ParameterIndex i) or
+  TOutParameterDeref(ParameterIndex i, int indirectionIndex) { indirectionIndex = [1, 2] } or
   TOutQualifierObject() or
   TOutReturnValue() or
-  TOutReturnValueDeref()
+  TOutReturnValueDeref(int indirections) { indirections = [1, 2] }
 
 /**
  * An output from a function. This can be:
@@ -498,17 +501,16 @@ class FunctionOutput extends TFunctionOutput {
  */
 class OutParameterDeref extends FunctionOutput, TOutParameterDeref {
   ParameterIndex index;
+  int indirectionIndex;
 
-  OutParameterDeref() { this = TOutParameterDeref(index) }
+  OutParameterDeref() { this = TOutParameterDeref(index, indirectionIndex) }
 
   override string toString() { result = "OutParameterDeref " + index.toString() }
 
   ParameterIndex getIndex() { result = index }
 
-  override predicate isParameterDeref(ParameterIndex i) { i = index }
-
   override predicate isParameterDeref(ParameterIndex i, int ind) {
-    this.isParameterDeref(i) and ind = 1
+    i = index and ind = indirectionIndex
   }
 }
 
@@ -572,4 +574,8 @@ class OutReturnValueDeref extends FunctionOutput, TOutReturnValueDeref {
   override string toString() { result = "OutReturnValueDeref" }
 
   override predicate isReturnValueDeref() { any() }
+
+  override predicate isReturnValueDeref(int indirectionIndex) {
+    this = TOutReturnValueDeref(indirectionIndex)
+  }
 }

cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/RangeAnalysis.qll

@@ -17,9 +17,7 @@ private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
  * `upper` is true, and can be traced back to a guard represented by `reason`.
  */
 predicate bounded(Expr e, Bound b, float delta, boolean upper, Reason reason) {
-  exists(SemanticExprConfig::Expr semExpr |
-    semExpr.getUnconverted().getUnconvertedResultExpression() = e
-  |
+  exists(SemanticExprConfig::Expr semExpr | semExpr.getUnconvertedResultExpression() = e |
     semBounded(semExpr, b, delta, upper, reason)
   )
 }
@@ -30,9 +28,7 @@ predicate bounded(Expr e, Bound b, float delta, boolean upper, Reason reason) {
  * The `Expr` may be a conversion.
  */
 predicate convertedBounded(Expr e, Bound b, float delta, boolean upper, Reason reason) {
-  exists(SemanticExprConfig::Expr semExpr |
-    semExpr.getConverted().getConvertedResultExpression() = e
-  |
+  exists(SemanticExprConfig::Expr semExpr | semExpr.getConvertedResultExpression() = e |
     semBounded(semExpr, b, delta, upper, reason)
   )
 }