Merge branch 'main' into impropnullfp

cpp/ql/src/AlertSuppression.ql

@@ -68,7 +68,7 @@ class SuppressionScope extends ElementBase {
    * The location spans column `startcolumn` of line `startline` to
    * column `endcolumn` of line `endline` in file `filepath`.
    * For more information, see
-   * [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+   * [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
    */
   predicate hasLocationInfo(
     string filepath, int startline, int startcolumn, int endline, int endcolumn

cpp/ql/src/Critical/OverflowStatic.ql

@@ -5,7 +5,7 @@
  * @kind problem
  * @problem.severity warning
  * @security-severity 9.3
- * @precision medium
+ * @precision high
  * @id cpp/static-buffer-overflow
  * @tags reliability
  *       security
@@ -55,6 +55,8 @@ predicate overflowOffsetInLoop(BufferAccess bufaccess, string msg) {
     loop.counter().getAnAccess() = bufaccess.getArrayOffset() and
     // Ensure that we don't have an upper bound on the array index that's less than the buffer size.
     not upperBound(bufaccess.getArrayOffset().getFullyConverted()) < bufaccess.bufferSize() and
+    // The upper bounds analysis must not have been widended
+    not upperBoundMayBeWidened(bufaccess.getArrayOffset().getFullyConverted()) and
     msg =
       "Potential buffer-overflow: counter '" + loop.counter().toString() + "' <= " +
         loop.limit().toString() + " but '" + bufaccess.buffer().getName() + "' has " +
@@ -130,11 +132,13 @@ predicate outOfBounds(BufferAccess bufaccess, string msg) {
     (
       access > size
       or
-      access = size and not exists(AddressOfExpr addof | bufaccess = addof.getOperand())
+      access = size and
+      not exists(AddressOfExpr addof | bufaccess = addof.getOperand()) and
+      not exists(BuiltInOperationBuiltInOffsetOf offsetof | offsetof.getAChild() = bufaccess)
     ) and
     msg =
       "Potential buffer-overflow: '" + buf + "' has size " + size.toString() + " but '" + buf + "[" +
-        access.toString() + "]' is accessed here."
+        access.toString() + "]' may be accessed here."
   )
 }
 

cpp/ql/src/Documentation/CommentedOutCode.qll

@@ -198,7 +198,7 @@ class CommentBlock extends Comment {
    * The location spans column `startcolumn` of line `startline` to
    * column `endcolumn` of line `endline` in file `filepath`.
    * For more information, see
-   * [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+   * [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
    */
   predicate hasLocationInfo(
     string filepath, int startline, int startcolumn, int endline, int endcolumn

cpp/ql/src/Metrics/Internal/CallableExtents.ql

@@ -18,7 +18,7 @@ class RangeFunction extends Function {
    * The location spans column `startcolumn` of line `startline` to
    * column `endcolumn` of line `endline` in file `filepath`.
    * For more information, see
-   * [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+   * [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
    */
   predicate hasLocationInfo(string path, int sl, int sc, int el, int ec) {
     super.getLocation().hasLocationInfo(path, sl, sc, _, _) and

cpp/ql/src/Security/CWE/CWE-078/ExecTainted.c

@@ -9,7 +9,7 @@ int main(int argc, char** argv) {
     system(command1);
   }
 
-  {  
+  {
     // GOOD: the user string is encoded by a library routine.
     char userNameQuoted[1000] = {0};
     encodeShellString(userNameQuoted, 1000, userName); 

cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql

@@ -3,10 +3,10 @@
  * @description Using user-supplied data in an OS command, without
  *              neutralizing special elements, can make code vulnerable
  *              to command injection.
- * @kind problem
+ * @kind path-problem
  * @problem.severity error
  * @security-severity 9.8
- * @precision low
+ * @precision high
  * @id cpp/command-line-injection
  * @tags security
  *       external/cwe/cwe-078
@@ -16,13 +16,204 @@
 import cpp
 import semmle.code.cpp.security.CommandExecution
 import semmle.code.cpp.security.Security
-import semmle.code.cpp.security.TaintTracking
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+import semmle.code.cpp.ir.IR
+import semmle.code.cpp.ir.dataflow.TaintTracking
+import semmle.code.cpp.ir.dataflow.TaintTracking2
+import semmle.code.cpp.security.FlowSources
+import semmle.code.cpp.models.implementations.Strcat
 
-from Expr taintedArg, Expr taintSource, string taintCause, string callChain
+Expr sinkAsArgumentIndirection(DataFlow::Node sink) {
+  result =
+    sink.asOperand()
+        .(SideEffectOperand)
+        .getAddressOperand()
+        .getAnyDef()
+        .getUnconvertedResultExpression()
+}
+
+/**
+ * Holds if `fst` is a string that is used in a format or concatenation function resulting in `snd`,
+ * and is *not* placed at the start of the resulting string. This indicates that the author did not
+ * expect `fst` to control what program is run if the resulting string is eventually interpreted as
+ * a command line, for example as an argument to `system`.
+ */
+predicate interestingConcatenation(DataFlow::Node fst, DataFlow::Node snd) {
+  exists(FormattingFunctionCall call, int index, FormatLiteral literal |
+    sinkAsArgumentIndirection(fst) = call.getConversionArgument(index) and
+    snd.asDefiningArgument() = call.getOutputArgument(false) and
+    literal = call.getFormat() and
+    not literal.getConvSpecOffset(index) = 0 and
+    literal.getConversionChar(index) = ["s", "S"]
+  )
+  or
+  // strcat and friends
+  exists(StrcatFunction strcatFunc, CallInstruction call, ReadSideEffectInstruction rse |
+    call.getStaticCallTarget() = strcatFunc and
+    rse.getArgumentDef() = call.getArgument(strcatFunc.getParamSrc()) and
+    fst.asOperand() = rse.getSideEffectOperand() and
+    snd.asInstruction().(WriteSideEffectInstruction).getDestinationAddress() =
+      call.getArgument(strcatFunc.getParamDest())
+  )
+  or
+  exists(CallInstruction call, Operator op, ReadSideEffectInstruction rse |
+    call.getStaticCallTarget() = op and
+    op.hasQualifiedName("std", "operator+") and
+    op.getType().(UserType).hasQualifiedName("std", "basic_string") and
+    call.getArgument(1) = rse.getArgumentOperand().getAnyDef() and // left operand
+    fst.asOperand() = rse.getSideEffectOperand() and
+    call = snd.asInstruction()
+  )
+}
+
+class TaintToConcatenationConfiguration extends TaintTracking::Configuration {
+  TaintToConcatenationConfiguration() { this = "TaintToConcatenationConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof FlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { interestingConcatenation(sink, _) }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    node.asInstruction().getResultType() instanceof IntegralType
+    or
+    node.asInstruction().getResultType() instanceof FloatingPointType
+  }
+}
+
+class ExecTaintConfiguration extends TaintTracking2::Configuration {
+  ExecTaintConfiguration() { this = "ExecTaintConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) {
+    exists(DataFlow::Node prevSink, TaintToConcatenationConfiguration conf |
+      conf.hasFlow(_, prevSink) and
+      interestingConcatenation(prevSink, source)
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    shellCommand(sinkAsArgumentIndirection(sink), _)
+  }
+
+  override predicate isSanitizerOut(DataFlow::Node node) {
+    isSink(node) // Prevent duplicates along a call chain, since `shellCommand` will include wrappers
+  }
+}
+
+module StitchedPathGraph {
+  // There's a different PathNode class for each DataFlowImplN.qll, so we can't simply combine the
+  // PathGraph predicates directly. Instead, we use a newtype so there's a single type that
+  // contains both sets of PathNodes.
+  newtype TMergedPathNode =
+    TPathNode1(DataFlow::PathNode node) or
+    TPathNode2(DataFlow2::PathNode node)
+
+  // this wraps the toString and location predicates so we can use the merged node type in a
+  // selection
+  class MergedPathNode extends TMergedPathNode {
+    string toString() {
+      exists(DataFlow::PathNode n |
+        this = TPathNode1(n) and
+        result = n.toString()
+      )
+      or
+      exists(DataFlow2::PathNode n |
+        this = TPathNode2(n) and
+        result = n.toString()
+      )
+    }
+
+    DataFlow::Node getNode() {
+      exists(DataFlow::PathNode n |
+        this = TPathNode1(n) and
+        result = n.getNode()
+      )
+      or
+      exists(DataFlow2::PathNode n |
+        this = TPathNode2(n) and
+        result = n.getNode()
+      )
+    }
+
+    DataFlow::PathNode getPathNode1() { this = TPathNode1(result) }
+
+    DataFlow2::PathNode getPathNode2() { this = TPathNode2(result) }
+
+    predicate hasLocationInfo(
+      string filepath, int startline, int startcolumn, int endline, int endcolumn
+    ) {
+      exists(DataFlow::PathNode n |
+        this = TPathNode1(n) and
+        n.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+      )
+      or
+      exists(DataFlow2::PathNode n |
+        this = TPathNode2(n) and
+        n.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+      )
+    }
+  }
+
+  query predicate edges(MergedPathNode a, MergedPathNode b) {
+    exists(DataFlow::PathNode an, DataFlow::PathNode bn |
+      a = TPathNode1(an) and
+      b = TPathNode1(bn) and
+      DataFlow::PathGraph::edges(an, bn)
+    )
+    or
+    exists(DataFlow2::PathNode an, DataFlow2::PathNode bn |
+      a = TPathNode2(an) and
+      b = TPathNode2(bn) and
+      DataFlow2::PathGraph::edges(an, bn)
+    )
+    or
+    // This is where paths from the two configurations are connected. `interestingConcatenation`
+    // is the only thing in this module that's actually specific to the query - everything else is
+    // just using types and predicates from the DataFlow library.
+    interestingConcatenation(a.getNode(), b.getNode()) and
+    a instanceof TPathNode1 and
+    b instanceof TPathNode2
+  }
+
+  query predicate nodes(MergedPathNode mpn, string key, string val) {
+    // here we just need the union of the underlying `nodes` predicates
+    exists(DataFlow::PathNode n |
+      mpn = TPathNode1(n) and
+      DataFlow::PathGraph::nodes(n, key, val)
+    )
+    or
+    exists(DataFlow2::PathNode n |
+      mpn = TPathNode2(n) and
+      DataFlow2::PathGraph::nodes(n, key, val)
+    )
+  }
+
+  query predicate subpaths(
+    MergedPathNode arg, MergedPathNode par, MergedPathNode ret, MergedPathNode out
+  ) {
+    // just forward subpaths from the underlying libraries. This might be slightly awkward when
+    // the concatenation is deep in a call chain.
+    DataFlow::PathGraph::subpaths(arg.getPathNode1(), par.getPathNode1(), ret.getPathNode1(),
+      out.getPathNode1())
+    or
+    DataFlow2::PathGraph::subpaths(arg.getPathNode2(), par.getPathNode2(), ret.getPathNode2(),
+      out.getPathNode2())
+  }
+}
+
+import StitchedPathGraph
+
+from
+  DataFlow::PathNode sourceNode, DataFlow::PathNode concatSink, DataFlow2::PathNode concatSource,
+  DataFlow2::PathNode sinkNode, string taintCause, string callChain,
+  TaintToConcatenationConfiguration conf1, ExecTaintConfiguration conf2
 where
-  shellCommand(taintedArg, callChain) and
-  tainted(taintSource, taintedArg) and
-  isUserInput(taintSource, taintCause)
-select taintedArg,
-  "This argument to an OS command is derived from $@ and then passed to " + callChain, taintSource,
-  "user input (" + taintCause + ")"
+  taintCause = sourceNode.getNode().(FlowSource).getSourceType() and
+  conf1.hasFlowPath(sourceNode, concatSink) and
+  interestingConcatenation(concatSink.getNode(), concatSource.getNode()) and // this loses call context
+  conf2.hasFlowPath(concatSource, sinkNode) and
+  shellCommand(sinkAsArgumentIndirection(sinkNode.getNode()), callChain)
+select sinkAsArgumentIndirection(sinkNode.getNode()), TPathNode1(sourceNode).(MergedPathNode),
+  TPathNode2(sinkNode).(MergedPathNode),
+  "This argument to an OS command is derived from $@, dangerously concatenated into $@, and then passed to "
+    + callChain, sourceNode, "user input (" + taintCause + ")", concatSource,
+  concatSource.toString()

cpp/ql/src/Security/CWE/CWE-311/CleartextStorage.inc.qhelp

@@ -9,7 +9,7 @@ storage.</p>
 </overview>
 <recommendation>
 
-<p>Ensure that sensitive information is always encrypted before being stored, especially before writing to a file.
+<p>Ensure that sensitive information is always encrypted before being stored to a file or transmitted over the network.
 It may be wise to encrypt information before it is put into a buffer that may be readable in memory.</p>
 
 <p>In general, decrypt sensitive information only at the point where it is necessary for it to be used in

cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.qhelp

@@ -0,0 +1,5 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<include src="CleartextStorage.inc.qhelp" /></qhelp>

cpp/ql/src/Security/CWE/CWE-311/CleartextTransmission.ql

@@ -0,0 +1,124 @@
+/**
+ * @name Cleartext transmission of sensitive information
+ * @description Transmitting sensitive information across a network in
+ *              cleartext can expose it to an attacker.
+ * @kind path-problem
+ * @problem.severity warning
+ * @security-severity 7.5
+ * @precision medium
+ * @id cpp/cleartext-transmission
+ * @tags security
+ *       external/cwe/cwe-319
+ */
+
+import cpp
+import semmle.code.cpp.security.SensitiveExprs
+import semmle.code.cpp.dataflow.TaintTracking
+import semmle.code.cpp.models.interfaces.FlowSource
+import DataFlow::PathGraph
+
+/**
+ * A function call that sends or receives data over a network.
+ */
+abstract class NetworkSendRecv extends FunctionCall {
+  /**
+   * Gets the expression for the socket or similar object used for sending or
+   * receiving data (if any).
+   */
+  abstract Expr getSocketExpr();
+
+  /**
+   * Gets the expression for the buffer to be sent from / received into.
+   */
+  abstract Expr getDataExpr();
+}
+
+/**
+ * A function call that sends data over a network.
+ *
+ * note: functions such as `write` may be writing to a network source or a file. We could attempt to determine which, and sort results into `cpp/cleartext-transmission` and perhaps `cpp/cleartext-storage-file`. In practice it usually isn't very important which query reports a result as long as its reported exactly once.
+ */
+class NetworkSend extends NetworkSendRecv {
+  RemoteFlowSinkFunction target;
+
+  NetworkSend() { target = this.getTarget() }
+
+  override Expr getSocketExpr() {
+    exists(FunctionInput input, int arg |
+      target.hasSocketInput(input) and
+      input.isParameter(arg) and
+      result = this.getArgument(arg)
+    )
+  }
+
+  override Expr getDataExpr() {
+    exists(FunctionInput input, int arg |
+      target.hasRemoteFlowSink(input, _) and
+      input.isParameterDeref(arg) and
+      result = this.getArgument(arg)
+    )
+  }
+}
+
+/**
+ * A function call that receives data over a network.
+ */
+class NetworkRecv extends NetworkSendRecv {
+  RemoteFlowSourceFunction target;
+
+  NetworkRecv() { target = this.getTarget() }
+
+  override Expr getSocketExpr() {
+    exists(FunctionInput input, int arg |
+      target.hasSocketInput(input) and
+      input.isParameter(arg) and
+      result = this.getArgument(arg)
+    )
+  }
+
+  override Expr getDataExpr() {
+    exists(FunctionOutput output, int arg |
+      target.hasRemoteFlowSource(output, _) and
+      output.isParameterDeref(arg) and
+      result = this.getArgument(arg)
+    )
+  }
+}
+
+/**
+ * Taint flow from a sensitive expression to a network operation with data
+ * tainted by that expression.
+ */
+class SensitiveSendRecvConfiguration extends TaintTracking::Configuration {
+  SensitiveSendRecvConfiguration() { this = "SensitiveSendRecvConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof SensitiveExpr }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(NetworkSendRecv transmission |
+      sink.asExpr() = transmission.getDataExpr() and
+      // a zero socket descriptor is standard input, which is not interesting for this query.
+      not exists(Zero zero |
+        DataFlow::localFlow(DataFlow::exprNode(zero),
+          DataFlow::exprNode(transmission.getSocketExpr()))
+      )
+    )
+  }
+}
+
+from
+  SensitiveSendRecvConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink,
+  NetworkSendRecv transmission, string msg
+where
+  config.hasFlowPath(source, sink) and
+  sink.getNode().asExpr() = transmission.getDataExpr() and
+  if transmission instanceof NetworkSend
+  then
+    msg =
+      "This operation transmits '" + sink.toString() +
+        "', which may contain unencrypted sensitive data from $@"
+  else
+    msg =
+      "This operation receives into '" + sink.toString() +
+        "', which may put unencrypted sensitive data into $@"
+select transmission, source, sink, msg, source, source.getNode().asExpr().toString()

cpp/ql/src/Security/CWE/CWE-570/IncorrectAllocationErrorHandling.ql

@@ -4,10 +4,13 @@
  * @kind problem
  * @id cpp/incorrect-allocation-error-handling
  * @problem.severity warning
+ * @security-severity 7.5
  * @precision medium
  * @tags correctness
  *       security
  *       external/cwe/cwe-570
+ *       external/cwe/cwe-252
+ *       external/cwe/cwe-755
  */
 
 import cpp

cpp/ql/src/definitions.qll

@@ -24,7 +24,7 @@ class Top extends Element {
    * The location spans column `startcolumn` of line `startline` to
    * column `endcolumn` of line `endline` in file `filepath`.
    * For more information, see
-   * [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+   * [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
    */
   pragma[noopt]
   final predicate hasLocationInfo(

cpp/ql/src/external/CodeDuplication.qll

@@ -61,7 +61,7 @@ class Copy extends @duplication_or_similarity {
    * The location spans column `startcolumn` of line `startline` to
    * column `endcolumn` of line `endline` in file `filepath`.
    * For more information, see
-   * [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+   * [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
    */
   predicate hasLocationInfo(
     string filepath, int startline, int startcolumn, int endline, int endcolumn

cpp/ql/src/external/DefectFilter.qll

@@ -8,7 +8,7 @@ import cpp
  * column `startcolumn` of line `startline` to column `endcolumn` of line `endline`
  * in file `filepath`.
  *
- * For more information, see [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+ * For more information, see [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
  */
 external predicate defectResults(
   int id, string queryPath, string file, int startline, int startcol, int endline, int endcol,

cpp/ql/src/external/MetricFilter.qll

@@ -8,7 +8,7 @@ import cpp
  * column `startcolumn` of line `startline` to column `endcolumn` of line `endline`
  * in file `filepath`.
  *
- * For more information, see [Locations](https://help.semmle.com/QL/learn-ql/ql/locations.html).
+ * For more information, see [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
  */
 external predicate metricResults(
   int id, string queryPath, string file, int startline, int startcol, int endline, int endcol,