Merge branch 'rc/1.24' into update-references

Author: felicitymay

cpp/ql/src/Likely Bugs/Protocols/TlsSettingsMisconfiguration.ql

@@ -3,7 +3,7 @@
  * @description Using the TLS or SSLv23 protocol from the boost::asio library, but not disabling deprecated protocols, or disabling minimum-recommended protocols.
  * @kind problem
  * @problem.severity error
- * @id cpp/boost/tls_settings_misconfiguration
+ * @id cpp/boost/tls-settings-misconfiguration
  * @tags security
  */
 

cpp/ql/src/qlpack.yml

@@ -2,3 +2,4 @@ name: codeql-cpp
 version: 0.0.0
 dbscheme: semmlecode.cpp.dbscheme
 suites: codeql-suites
+extractor: cpp

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

@@ -199,7 +199,27 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
   // Flow through pointer dereference
   i2.(LoadInstruction).getSourceAddress() = i1
   or
-  i2.(UnaryInstruction).getUnary() = i1
+  // Flow through partial reads of arrays and unions
+  i2.(LoadInstruction).getSourceValueOperand().getAnyDef() = i1 and
+  not i1.isResultConflated() and
+  (
+    i1.getResultType() instanceof ArrayType or
+    i1.getResultType() instanceof Union
+  )
+  or
+  // Unary instructions tend to preserve enough information in practice that we
+  // want taint to flow through.
+  // The exception is `FieldAddressInstruction`. Together with the rule for
+  // `LoadInstruction` above and for `ChiInstruction` below, flow through
+  // `FieldAddressInstruction` could cause flow into one field to come out an
+  // unrelated field. This would happen across function boundaries, where the IR
+  // would not be able to match loads to stores.
+  i2.(UnaryInstruction).getUnary() = i1 and
+  (
+    not i2 instanceof FieldAddressInstruction
+    or
+    i2.(FieldAddressInstruction).getField().getDeclaringType() instanceof Union
+  )
   or
   // Flow out of definition-by-reference
   i2.(ChiInstruction).getPartial() = i1.(WriteSideEffectInstruction) and
@@ -213,7 +233,7 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
     or
     t instanceof ArrayType
     or
-    // Buffers or unknown size
+    // Buffers of unknown size
     t instanceof UnknownType
   )
   or

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

@@ -70,8 +70,7 @@ private module VirtualDispatch {
         // Call return
         exists(DataFlowCall call, ReturnKind returnKind |
           other = getAnOutNode(call, returnKind) and
-          src.(ReturnNode).getKind() = returnKind and
-          call.getStaticCallTarget() = src.getEnclosingCallable()
+          returnNodeWithKindAndEnclosingCallable(src, returnKind, call.getStaticCallTarget())
         ) and
         allowFromArg = false
         or
@@ -125,6 +124,18 @@ private module VirtualDispatch {
     }
   }
 
+  /**
+   * A ReturnNode with its ReturnKind and its enclosing callable.
+   *
+   * Used to fix a join ordering issue in flowsFrom.
+   */
+  private predicate returnNodeWithKindAndEnclosingCallable(
+    ReturnNode node, ReturnKind kind, DataFlowCallable callable
+  ) {
+    node.getKind() = kind and
+    node.getEnclosingCallable() = callable
+  }
+
   /** Call through a function pointer. */
   private class DataSensitiveExprCall extends DataSensitiveCall {
     DataSensitiveExprCall() { not exists(this.getStaticCallTarget()) }

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

@@ -24,31 +24,86 @@ class ArgumentNode extends InstructionNode {
   DataFlowCall getCall() { this.argumentOf(result, _) }
 }
 
-private newtype TReturnKind = TNormalReturnKind()
+private newtype TReturnKind =
+  TNormalReturnKind() or
+  TIndirectReturnKind(ParameterIndex index)
 
 /**
  * A return kind. A return kind describes how a value can be returned
  * from a callable. For C++, this is simply a function return.
  */
 class ReturnKind extends TReturnKind {
   /** Gets a textual representation of this return kind. */
-  string toString() { result = "return" }
+  abstract string toString();
+}
+
+private class NormalReturnKind extends ReturnKind, TNormalReturnKind {
+  override string toString() { result = "return" }
+}
+
+private class IndirectReturnKind extends ReturnKind, TIndirectReturnKind {
+  ParameterIndex index;
+
+  IndirectReturnKind() { this = TIndirectReturnKind(index) }
+
+  override string toString() { result = "outparam[" + index.toString() + "]" }
 }
 
 /** A data flow node that occurs as the result of a `ReturnStmt`. */
 class ReturnNode extends InstructionNode {
-  ReturnNode() { exists(ReturnValueInstruction ret | this.getInstruction() = ret.getReturnValue()) }
+  Instruction primary;
+
+  ReturnNode() {
+    exists(ReturnValueInstruction ret | instr = ret.getReturnValue() and primary = ret)
+    or
+    exists(ReturnIndirectionInstruction rii |
+      instr = rii.getSideEffectOperand().getAnyDef() and primary = rii
+    )
+  }
 
   /** Gets the kind of this returned value. */
-  ReturnKind getKind() { result = TNormalReturnKind() }
+  abstract ReturnKind getKind();
+}
+
+class ReturnValueNode extends ReturnNode {
+  override ReturnValueInstruction primary;
+
+  override ReturnKind getKind() { result = TNormalReturnKind() }
+}
+
+class ReturnIndirectionNode extends ReturnNode {
+  override ReturnIndirectionInstruction primary;
+
+  override ReturnKind getKind() { result = TIndirectReturnKind(primary.getParameter().getIndex()) }
 }
 
 /** A data flow node that represents the output of a call. */
 class OutNode extends InstructionNode {
-  override CallInstruction instr;
+  OutNode() {
+    instr instanceof CallInstruction or
+    instr instanceof WriteSideEffectInstruction
+  }
 
   /** Gets the underlying call. */
-  DataFlowCall getCall() { result = instr }
+  abstract DataFlowCall getCall();
+
+  abstract ReturnKind getReturnKind();
+}
+
+private class CallOutNode extends OutNode {
+  override CallInstruction instr;
+
+  override DataFlowCall getCall() { result = instr }
+
+  override ReturnKind getReturnKind() { result instanceof NormalReturnKind }
+}
+
+private class SideEffectOutNode extends OutNode {
+  override WriteSideEffectInstruction instr;
+
+  override DataFlowCall getCall() { result = instr.getPrimaryInstruction() }
+
+  override ReturnKind getReturnKind() { result = TIndirectReturnKind(instr.getIndex()) }
 }
 
 /**
@@ -57,7 +112,7 @@ class OutNode extends InstructionNode {
  */
 OutNode getAnOutNode(DataFlowCall call, ReturnKind kind) {
   result.getCall() = call and
-  kind = TNormalReturnKind()
+  result.getReturnKind() = kind
 }
 
 /**

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

@@ -525,7 +525,7 @@ class ReturnValueInstruction extends ReturnInstruction {
   final Instruction getReturnValue() { result = getReturnValueOperand().getDef() }
 }
 
-class ReturnIndirectionInstruction extends Instruction {
+class ReturnIndirectionInstruction extends VariableInstruction {
   ReturnIndirectionInstruction() { getOpcode() instanceof Opcode::ReturnIndirection }
 
   final SideEffectOperand getSideEffectOperand() { result = getAnOperand() }
@@ -535,6 +535,12 @@ class ReturnIndirectionInstruction extends Instruction {
   final AddressOperand getSourceAddressOperand() { result = getAnOperand() }
 
   final Instruction getSourceAddress() { result = getSourceAddressOperand().getDef() }
+
+  /**
+   * Gets the parameter for which this instruction reads the final pointed-to value within the
+   * function.
+   */
+  final Language::Parameter getParameter() { result = var.(IRUserVariable).getVariable() }
 }
 
 class CopyInstruction extends Instruction {

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

@@ -525,7 +525,7 @@ class ReturnValueInstruction extends ReturnInstruction {
   final Instruction getReturnValue() { result = getReturnValueOperand().getDef() }
 }
 
-class ReturnIndirectionInstruction extends Instruction {
+class ReturnIndirectionInstruction extends VariableInstruction {
   ReturnIndirectionInstruction() { getOpcode() instanceof Opcode::ReturnIndirection }
 
   final SideEffectOperand getSideEffectOperand() { result = getAnOperand() }
@@ -535,6 +535,12 @@ class ReturnIndirectionInstruction extends Instruction {
   final AddressOperand getSourceAddressOperand() { result = getAnOperand() }
 
   final Instruction getSourceAddress() { result = getSourceAddressOperand().getDef() }
+
+  /**
+   * Gets the parameter for which this instruction reads the final pointed-to value within the
+   * function.
+   */
+  final Language::Parameter getParameter() { result = var.(IRUserVariable).getVariable() }
 }
 
 class CopyInstruction extends Instruction {

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll

@@ -324,6 +324,16 @@ class TranslatedFunctionCall extends TranslatedCallExpr, TranslatedDirectCall {
   override predicate hasWriteSideEffect() {
     not expr.getTarget().(SideEffectFunction).hasOnlySpecificWriteSideEffects()
   }
+
+  override Instruction getQualifierResult() {
+    hasQualifier() and
+    result = getQualifier().getResult()
+  }
+
+  override predicate hasQualifier() {
+    exists(getQualifier()) and
+    not exists(MemberFunction func | expr.getTarget() = func and func.isStatic())
+  }
 }
 
 /**

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll

@@ -463,7 +463,9 @@ newtype TTranslatedElement =
       expr = call.getArgument(n).getFullyConverted()
       or
       expr = call.getQualifier().getFullyConverted() and
-      n = -1
+      n = -1 and
+      // Exclude calls to static member functions. They don't modify the qualifier
+      not exists(MemberFunction func | func = call.getTarget() and func.isStatic())
     ) and
     (
       call.getTarget().(SideEffectFunction).hasSpecificReadSideEffect(n, _) and

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedFunction.qll

@@ -744,4 +744,9 @@ class TranslatedReadEffect extends TranslatedElement, TTranslatedReadEffect {
     operandTag = sideEffectOperand() and
     result = getUnknownType()
   }
+
+  final override IRVariable getInstructionVariable(InstructionTag tag) {
+    tag = OnlyInstructionTag() and
+    result = getIRUserVariable(getFunction(), param)
+  }
 }