Merge branch 'main' of github.com:github/codeql into SharedDataflow_FieldFlow

Author: yoff

change-notes/1.26/analysis-javascript.md

@@ -4,8 +4,10 @@
 
 * Support for the following frameworks and libraries has been improved:
   - [bluebird](https://www.npmjs.com/package/bluebird)
+  - [express](https://www.npmjs.com/package/express)
   - [fast-json-stable-stringify](https://www.npmjs.com/package/fast-json-stable-stringify)
   - [fast-safe-stringify](https://www.npmjs.com/package/fast-safe-stringify)
+  - [http](https://nodejs.org/api/http.html)
   - [javascript-stringify](https://www.npmjs.com/package/javascript-stringify)
   - [js-stringify](https://www.npmjs.com/package/js-stringify)
   - [json-stable-stringify](https://www.npmjs.com/package/json-stable-stringify)

cpp/ql/src/semmle/code/cpp/controlflow/internal/ConstantExprs.qll

@@ -279,20 +279,62 @@ private predicate reachableRecursive(ControlFlowNode n) {
   reachableRecursive(n.getAPredecessor())
 }
 
+/** Holds if `e` is a compile time constant with integer value `val`. */
 private predicate compileTimeConstantInt(Expr e, int val) {
-  val = e.getFullyConverted().getValue().toInt() and
-  not e instanceof StringLiteral and
-  not exists(Expr e1 | e1.getConversion() = e) // only values for fully converted expressions
+  (
+    // If we have an integer value then we are done.
+    if exists(e.getValue().toInt())
+    then val = e.getValue().toInt()
+    else
+      // Otherwise, if we are a conversion of another expression with an
+      // integer value, and that value can be converted into our type,
+      // then we have that value.
+      exists(Expr x, int valx |
+        x.getConversion() = e and
+        compileTimeConstantInt(x, valx) and
+        val = convertIntToType(valx, e.getType().getUnspecifiedType())
+      )
+  ) and
+  // If our unconverted expression is a string literal `"123"`, then we
+  // do not have integer value `123`.
+  not e.getUnconverted() instanceof StringLiteral
 }
 
-library class CompileTimeConstantInt extends Expr {
-  CompileTimeConstantInt() { compileTimeConstantInt(this, _) }
+/**
+ * Get `val` represented as type `t`, if that is possible without
+ * overflow or underflows.
+ */
+bindingset[val, t]
+private int convertIntToType(int val, IntegralType t) {
+  if t instanceof BoolType
+  then if val = 0 then result = 0 else result = 1
+  else
+    if t.isUnsigned()
+    then if val >= 0 and val.bitShiftRight(t.getSize() * 8) = 0 then result = val else none()
+    else
+      if val >= 0 and val.bitShiftRight(t.getSize() * 8 - 1) = 0
+      then result = val
+      else
+        if (-(val + 1)).bitShiftRight(t.getSize() * 8 - 1) = 0
+        then result = val
+        else none()
+}
+
+/**
+ * INTERNAL: Do not use.
+ * An expression that has been found to have an integer value at compile
+ * time.
+ */
+class CompileTimeConstantInt extends Expr {
+  int val;
+
+  CompileTimeConstantInt() { compileTimeConstantInt(this.getFullyConverted(), val) }
 
-  int getIntValue() { compileTimeConstantInt(this, result) }
+  int getIntValue() { result = val }
 }
 
 library class CompileTimeVariableExpr extends Expr {
-  CompileTimeVariableExpr() { not compileTimeConstantInt(this, _) }
+  CompileTimeVariableExpr() { not this instanceof CompileTimeConstantInt }
 }
 
 /** A helper class for evaluation of expressions. */

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

@@ -211,10 +211,17 @@ private predicate fieldStoreStepNoChi(Node node1, FieldContent f, PostUpdateNode
   )
 }
 
+private FieldAddressInstruction getFieldInstruction(Instruction instr) {
+  result = instr or
+  result = instr.(CopyValueInstruction).getUnary()
+}
+
 pragma[noinline]
-private predicate getWrittenField(StoreInstruction store, Field f, Class c) {
+private predicate getWrittenField(Instruction instr, Field f, Class c) {
   exists(FieldAddressInstruction fa |
-    fa = store.getDestinationAddress() and
+    fa =
+      getFieldInstruction([instr.(StoreInstruction).getDestinationAddress(),
+            instr.(WriteSideEffectInstruction).getDestinationAddress()]) and
     f = fa.getField() and
     c = f.getDeclaringType()
   )
@@ -265,7 +272,23 @@ private predicate arrayStoreStepChi(Node node1, ArrayContent a, PostUpdateNode n
 predicate storeStep(Node node1, Content f, PostUpdateNode node2) {
   fieldStoreStepNoChi(node1, f, node2) or
   fieldStoreStepChi(node1, f, node2) or
-  arrayStoreStepChi(node1, f, node2)
+  arrayStoreStepChi(node1, f, node2) or
+  fieldStoreStepAfterArraySuppression(node1, f, node2)
+}
+
+// This predicate pushes the correct `FieldContent` onto the access path when the
+// `suppressArrayRead` predicate has popped off an `ArrayContent`.
+private predicate fieldStoreStepAfterArraySuppression(
+  Node node1, FieldContent f, PostUpdateNode node2
+) {
+  exists(BufferMayWriteSideEffectInstruction write, ChiInstruction chi, Class c |
+    not chi.isResultConflated() and
+    node1.asInstruction() = chi and
+    node2.asInstruction() = chi and
+    chi.getPartial() = write and
+    getWrittenField(write, f.getAField(), c) and
+    f.hasOffset(c, _, _)
+  )
 }
 
 bindingset[result, i]
@@ -302,11 +325,88 @@ private predicate fieldReadStep(Node node1, FieldContent f, Node node2) {
   )
 }
 
+/**
+ * When a store step happens in a function that looks like an array write such as:
+ * ```cpp
+ * void f(int* pa) {
+ *   pa = source();
+ * }
+ * ```
+ * it can be a write to an array, but it can also happen that `f` is called as `f(&a.x)`. If that is
+ * the case, the `ArrayContent` that was written by the call to `f` should be popped off the access
+ * path, and a `FieldContent` containing `x` should be pushed instead.
+ * So this case pops `ArrayContent` off the access path, and the `fieldStoreStepAfterArraySuppression`
+ * predicate in `storeStep` ensures that we push the right `FieldContent` onto the access path.
+ */
+predicate suppressArrayRead(Node node1, ArrayContent a, Node node2) {
+  a = TArrayContent() and
+  exists(BufferMayWriteSideEffectInstruction write, ChiInstruction chi |
+    node1.asInstruction() = write and
+    node2.asInstruction() = chi and
+    chi.getPartial() = write and
+    getWrittenField(write, _, _)
+  )
+}
+
+private class ArrayToPointerConvertInstruction extends ConvertInstruction {
+  ArrayToPointerConvertInstruction() {
+    this.getUnary().getResultType() instanceof ArrayType and
+    this.getResultType() instanceof PointerType
+  }
+}
+
+private Instruction skipOneCopyValueInstruction(Instruction instr) {
+  not instr instanceof CopyValueInstruction and result = instr
+  or
+  result = instr.(CopyValueInstruction).getUnary()
+}
+
+private Instruction skipCopyValueInstructions(Instruction instr) {
+  result = skipOneCopyValueInstruction*(instr) and not result instanceof CopyValueInstruction
+}
+
 private predicate arrayReadStep(Node node1, ArrayContent a, Node node2) {
   a = TArrayContent() and
-  exists(LoadInstruction load |
+  // Explicit dereferences such as `*p` or `p[i]` where `p` is a pointer or array.
+  exists(LoadInstruction load, Instruction address |
+    load.getSourceValueOperand().isDefinitionInexact() and
     node1.asInstruction() = load.getSourceValueOperand().getAnyDef() and
-    load = node2.asInstruction()
+    load = node2.asInstruction() and
+    address = skipCopyValueInstructions(load.getSourceAddress()) and
+    (
+      address instanceof LoadInstruction or
+      address instanceof ArrayToPointerConvertInstruction or
+      address instanceof PointerOffsetInstruction
+    )
+  )
+}
+
+/**
+ * In cases such as:
+ * ```cpp
+ * void f(int* pa) {
+ *   *pa = source();
+ * }
+ * ...
+ * int x;
+ * f(&x);
+ * use(x);
+ * ```
+ * the load on `x` in `use(x)` will exactly overlap with its definition (in this case the definition
+ * is a `BufferMayWriteSideEffect`). This predicate pops the `ArrayContent` (pushed by the store in `f`)
+ * from the access path.
+ */
+private predicate exactReadStep(Node node1, ArrayContent a, Node node2) {
+  a = TArrayContent() and
+  exists(BufferMayWriteSideEffectInstruction write, ChiInstruction chi |
+    not chi.isResultConflated() and
+    chi.getPartial() = write and
+    node1.asInstruction() = write and
+    node2.asInstruction() = chi and
+    // To distinquish this case from the `arrayReadStep` case we require that the entire variable was
+    // overwritten by the `BufferMayWriteSideEffectInstruction` (i.e., there is a load that reads the
+    // entire variable).
+    exists(LoadInstruction load | load.getSourceValue() = chi)
   )
 }
 
@@ -317,7 +417,9 @@ private predicate arrayReadStep(Node node1, ArrayContent a, Node node2) {
  */
 predicate readStep(Node node1, Content f, Node node2) {
   fieldReadStep(node1, f, node2) or
-  arrayReadStep(node1, f, node2)
+  arrayReadStep(node1, f, node2) or
+  exactReadStep(node1, f, node2) or
+  suppressArrayRead(node1, f, node2)
 }
 
 /**

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

@@ -389,6 +389,35 @@ private class ExplicitSingleFieldStoreQualifierNode extends PartialDefinitionNod
   }
 }
 
+private FieldAddressInstruction getFieldInstruction(Instruction instr) {
+  result = instr or
+  result = instr.(CopyValueInstruction).getUnary()
+}
+
+/**
+ * The target of a `fieldStoreStepAfterArraySuppression` store step, which is used to convert
+ * an `ArrayContent` to a `FieldContent` when the `BufferMayWriteSideEffect` instruction stores
+ * into a field. See the QLDoc for `suppressArrayRead` for an example of where such a conversion
+ * is inserted.
+ */
+private class BufferMayWriteSideEffectFieldStoreQualifierNode extends PartialDefinitionNode {
+  override ChiInstruction instr;
+  BufferMayWriteSideEffectInstruction write;
+  FieldAddressInstruction field;
+
+  BufferMayWriteSideEffectFieldStoreQualifierNode() {
+    not instr.isResultConflated() and
+    instr.getPartial() = write and
+    field = getFieldInstruction(write.getDestinationAddress())
+  }
+
+  override Node getPreUpdateNode() { result.asOperand() = instr.getTotalOperand() }
+
+  override Expr getDefinedExpr() {
+    result = field.getObjectAddress().getUnconvertedResultExpression()
+  }
+}
+
 /**
  * The `PostUpdateNode` that is the target of a `arrayStoreStepChi` store step. The overriden
  * `ChiInstruction` corresponds to the instruction represented by `node2` in `arrayStoreStepChi`.
@@ -717,7 +746,12 @@ private predicate modelFlow(Operand opFrom, Instruction iTo) {
         iTo = outNode and
         outNode = getSideEffectFor(call, index)
       )
-      // TODO: add write side effects for qualifiers
+      or
+      exists(WriteSideEffectInstruction outNode |
+        modelOut.isQualifierObject() and
+        iTo = outNode and
+        outNode = getSideEffectFor(call, -1)
+      )
     ) and
     (
       exists(int index |
@@ -733,7 +767,12 @@ private predicate modelFlow(Operand opFrom, Instruction iTo) {
       or
       modelIn.isQualifierAddress() and
       opFrom = call.getThisArgumentOperand()
-      // TODO: add read side effects for qualifiers
+      or
+      exists(ReadSideEffectInstruction read |
+        modelIn.isQualifierObject() and
+        read = getSideEffectFor(call, -1) and
+        opFrom = read.getSideEffectOperand()
+      )
     )
   )
 }

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

@@ -9,24 +9,31 @@ private import semmle.code.cpp.ir.dataflow.DataFlow
 /**
  * Gets the instruction that goes into `input` for `call`.
  */
-Instruction callInput(CallInstruction call, FunctionInput input) {
+DataFlow::Node callInput(CallInstruction call, FunctionInput input) {
   // A positional argument
   exists(int index |
-    result = call.getPositionalArgument(index) and
+    result.asInstruction() = call.getPositionalArgument(index) and
     input.isParameter(index)
   )
   or
   // A value pointed to by a positional argument
   exists(ReadSideEffectInstruction read |
-    result = read and
+    result.asOperand() = read.getSideEffectOperand() and
     read.getPrimaryInstruction() = call and
     input.isParameterDeref(read.getIndex())
   )
   or
   // The qualifier pointer
-  result = call.getThisArgument() and
+  result.asInstruction() = call.getThisArgument() and
   input.isQualifierAddress()
-  //TODO: qualifier deref
+  or
+  // The qualifier object
+  exists(ReadSideEffectInstruction read |
+    result.asOperand() = read.getSideEffectOperand() and
+    read.getPrimaryInstruction() = call and
+    read.getIndex() = -1 and
+    input.isQualifierObject()
+  )
 }
 
 /**
@@ -43,5 +50,13 @@ Instruction callOutput(CallInstruction call, FunctionOutput output) {
     effect.getPrimaryInstruction() = call and
     output.isParameterDeref(effect.getIndex())
   )
-  // TODO: qualifiers, return value dereference
+  or
+  // The side effect of a call on the qualifier object
+  exists(WriteSideEffectInstruction effect |
+    result = effect and
+    effect.getPrimaryInstruction() = call and
+    effect.getIndex() = -1 and
+    output.isQualifierObject()
+  )
+  // TODO: return value dereference
 }

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

@@ -19,8 +19,11 @@ predicate localTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
  * local data flow steps. That is, `nodeFrom` and `nodeTo` are likely to represent
  * different objects.
  */
+cached
 predicate localAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
   localInstructionTaintStep(nodeFrom.asInstruction(), nodeTo.asInstruction())
+  or
+  modeledTaintStep(nodeFrom, nodeTo)
 }
 
 /**
@@ -49,8 +52,6 @@ private predicate localInstructionTaintStep(Instruction nodeFrom, Instruction no
   or
   nodeTo.(LoadInstruction).getSourceAddress() = nodeFrom
   or
-  modeledInstructionTaintStep(nodeFrom, nodeTo)
-  or
   // Flow through partial reads of arrays and unions
   nodeTo.(LoadInstruction).getSourceValueOperand().getAnyDef() = nodeFrom and
   not nodeFrom.isResultConflated() and
@@ -109,10 +110,17 @@ predicate defaultTaintSanitizer(DataFlow::Node node) { none() }
  * Holds if taint can flow from `instrIn` to `instrOut` through a call to a
  * modeled function.
  */
-predicate modeledInstructionTaintStep(Instruction instrIn, Instruction instrOut) {
+predicate modeledTaintStep(DataFlow::Node nodeIn, DataFlow::Node nodeOut) {
   exists(CallInstruction call, TaintFunction func, FunctionInput modelIn, FunctionOutput modelOut |
-    instrIn = callInput(call, modelIn) and
-    instrOut = callOutput(call, modelOut) and
+    (
+      nodeIn = callInput(call, modelIn)
+      or
+      exists(int n |
+        modelIn.isParameterDeref(n) and
+        nodeIn = callInput(call, any(InParameter inParam | inParam.getIndex() = n))
+      )
+    ) and
+    nodeOut.asInstruction() = callOutput(call, modelOut) and
     call.getStaticCallTarget() = func and
     func.hasTaintFlow(modelIn, modelOut)
   )
@@ -126,8 +134,8 @@ predicate modeledInstructionTaintStep(Instruction instrIn, Instruction instrOut)
     CallInstruction call, Function func, FunctionInput modelIn, OutParameterDeref modelMidOut,
     int indexMid, InParameter modelMidIn, OutReturnValue modelOut
   |
-    instrIn = callInput(call, modelIn) and
-    instrOut = callOutput(call, modelOut) and
+    nodeIn = callInput(call, modelIn) and
+    nodeOut.asInstruction() = callOutput(call, modelOut) and
     call.getStaticCallTarget() = func and
     func.(TaintFunction).hasTaintFlow(modelIn, modelMidOut) and
     func.(DataFlowFunction).hasDataFlow(modelMidIn, modelOut) and