Merge pull request #7338 from geoffw0/clrtxt2

C++: Improvements to cpp/cleartext-transmission

Author: MathiasVP

cpp/change-notes/2021-10-07-cleartext-transmission.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The "Cleartext transmission of sensitive information" (`cpp/cleartext-transmission`) query has been improved, reducing the number of false positive results when encryption is present.

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

@@ -14,105 +14,188 @@
 import cpp
 import semmle.code.cpp.security.SensitiveExprs
 import semmle.code.cpp.dataflow.TaintTracking
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import semmle.code.cpp.models.interfaces.FlowSource
 import DataFlow::PathGraph
 
 /**
- * A function call that sends or receives data over a network.
+ * A DataFlow node corresponding to a variable or function call that
+ * might contain or return a password or other sensitive information.
  */
-abstract class NetworkSendRecv extends FunctionCall {
+class SensitiveNode extends DataFlow::Node {
+  SensitiveNode() {
+    this.asExpr() = any(SensitiveVariable sv).getInitializer().getExpr() or
+    this.asExpr().(VariableAccess).getTarget() =
+      any(SensitiveVariable sv).(GlobalOrNamespaceVariable) or
+    this.asUninitialized() instanceof SensitiveVariable or
+    this.asParameter() instanceof SensitiveVariable or
+    this.asExpr().(FunctionCall).getTarget() instanceof SensitiveFunction
+  }
+}
+
+/**
+ * A function that sends or receives data over a network.
+ */
+abstract class SendRecv extends Function {
   /**
    * Gets the expression for the socket or similar object used for sending or
-   * receiving data (if any).
+   * receiving data through the function call `call` (if any).
    */
-  abstract Expr getSocketExpr();
+  abstract Expr getSocketExpr(Call call);
 
   /**
-   * Gets the expression for the buffer to be sent from / received into.
+   * Gets the expression for the buffer to be sent from / received into through
+   * the function call `call`.
    */
-  abstract Expr getDataExpr();
+  abstract Expr getDataExpr(Call call);
 }
 
 /**
- * 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.
+ * A function that sends data over a network.
  */
-class NetworkSend extends NetworkSendRecv {
-  RemoteFlowSinkFunction target;
-
-  NetworkSend() { target = this.getTarget() }
-
-  override Expr getSocketExpr() {
+class Send extends SendRecv instanceof RemoteFlowSinkFunction {
+  override Expr getSocketExpr(Call call) {
+    call.getTarget() = this and
     exists(FunctionInput input, int arg |
-      target.hasSocketInput(input) and
+      super.hasSocketInput(input) and
       input.isParameter(arg) and
-      result = this.getArgument(arg)
+      result = call.getArgument(arg)
     )
   }
 
-  override Expr getDataExpr() {
+  override Expr getDataExpr(Call call) {
+    call.getTarget() = this and
     exists(FunctionInput input, int arg |
-      target.hasRemoteFlowSink(input, _) and
+      super.hasRemoteFlowSink(input, _) and
       input.isParameterDeref(arg) and
-      result = this.getArgument(arg)
+      result = call.getArgument(arg)
     )
   }
 }
 
 /**
- * A function call that receives data over a network.
+ * A function that receives data over a network.
  */
-class NetworkRecv extends NetworkSendRecv {
-  RemoteFlowSourceFunction target;
-
-  NetworkRecv() { target = this.getTarget() }
-
-  override Expr getSocketExpr() {
+class Recv extends SendRecv instanceof RemoteFlowSourceFunction {
+  override Expr getSocketExpr(Call call) {
+    call.getTarget() = this and
     exists(FunctionInput input, int arg |
-      target.hasSocketInput(input) and
+      super.hasSocketInput(input) and
       input.isParameter(arg) and
-      result = this.getArgument(arg)
+      result = call.getArgument(arg)
     )
   }
 
-  override Expr getDataExpr() {
+  override Expr getDataExpr(Call call) {
+    call.getTarget() = this and
     exists(FunctionOutput output, int arg |
-      target.hasRemoteFlowSource(output, _) and
+      super.hasRemoteFlowSource(output, _) and
       output.isParameterDeref(arg) and
-      result = this.getArgument(arg)
+      result = call.getArgument(arg)
     )
   }
 }
 
 /**
- * Taint flow from a sensitive expression to a network operation with data
- * tainted by that expression.
+ * A function call that sends or receives data over a network.
+ *
+ * note: function calls 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.
+ *
+ * We do exclude function calls that specify a constant socket, which is
+ * likely to mean standard input, standard output or a similar channel.
  */
-class SensitiveSendRecvConfiguration extends TaintTracking::Configuration {
-  SensitiveSendRecvConfiguration() { this = "SensitiveSendRecvConfiguration" }
+abstract class NetworkSendRecv extends FunctionCall {
+  SendRecv target;
 
-  override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof SensitiveExpr }
+  NetworkSendRecv() {
+    this.getTarget() = target and
+    // exclude calls based on the socket...
+    not exists(GVN g |
+      g = globalValueNumber(target.getSocketExpr(this)) and
+      (
+        // literal constant
+        globalValueNumber(any(Literal l)) = g
+        or
+        // variable (such as a global) initialized to a literal constant
+        exists(Variable v |
+          v.getInitializer().getExpr() instanceof Literal and
+          g = globalValueNumber(v.getAnAccess())
+        )
+      )
+    )
+  }
 
-  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()))
+  final Expr getDataExpr() { result = target.getDataExpr(this) }
+}
+
+/**
+ * A function call that sends data over a network.
+ */
+class NetworkSend extends NetworkSendRecv {
+  override Send target;
+}
+
+/**
+ * A function call that receives data over a network.
+ */
+class NetworkRecv extends NetworkSendRecv {
+  override Recv target;
+}
+
+/**
+ * An expression that is an argument or return value from an encryption or
+ * decryption call.
+ */
+class Encrypted extends Expr {
+  Encrypted() {
+    exists(FunctionCall fc |
+      fc.getTarget().getName().toLowerCase().regexpMatch(".*(crypt|encode|decode).*") and
+      (
+        this = fc or
+        this = fc.getAnArgument()
       )
     )
   }
 }
 
+/**
+ * Taint flow from a sensitive expression.
+ */
+class FromSensitiveConfiguration extends TaintTracking::Configuration {
+  FromSensitiveConfiguration() { this = "FromSensitiveConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof SensitiveNode }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink.asExpr() = any(NetworkSendRecv nsr).getDataExpr()
+    or
+    sink.asExpr() instanceof Encrypted
+  }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    // flow through encryption functions to the return value (in case we can reach other sinks)
+    node2.asExpr().(Encrypted).(FunctionCall).getAnArgument() = node1.asExpr()
+  }
+}
+
 from
-  SensitiveSendRecvConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink,
-  NetworkSendRecv transmission, string msg
+  FromSensitiveConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink,
+  NetworkSendRecv networkSendRecv, string msg
 where
+  // flow from sensitive -> network data
   config.hasFlowPath(source, sink) and
-  sink.getNode().asExpr() = transmission.getDataExpr() and
-  if transmission instanceof NetworkSend
+  sink.getNode().asExpr() = networkSendRecv.getDataExpr() and
+  // no flow from sensitive -> evidence of encryption
+  not exists(DataFlow::Node encrypted |
+    config.hasFlow(source.getNode(), encrypted) and
+    encrypted.asExpr() instanceof Encrypted
+  ) and
+  // construct result
+  if networkSendRecv instanceof NetworkSend
   then
     msg =
       "This operation transmits '" + sink.toString() +
@@ -121,4 +204,4 @@ where
     msg =
       "This operation receives into '" + sink.toString() +
         "', which may put unencrypted sensitive data into $@"
-select transmission, source, sink, msg, source, source.getNode().asExpr().toString()
+select networkSendRecv, source, sink, msg, source, source.getNode().toString()