Merge branch 'main' into promote-arithmetic-uncontrolled

cpp/ql/src/CHANGELOG.md

@@ -0,0 +1,12 @@
+## 0.0.5
+
+### New Queries
+
+* A new query `cpp/certificate-not-checked` has been added for C/C++. The query flags unsafe use of OpenSSL and similar libraries.
+* A new query `cpp/certificate-result-conflation` has been added for C/C++. The query flags unsafe use of OpenSSL and similar libraries.
+
+## 0.0.4
+
+### New Queries
+
+* A new query `cpp/non-https-url` has been added for C/C++. The query flags uses of `http` URLs that might be better replaced with `https`.

cpp/ql/src/Likely Bugs/AmbiguouslySignedBitField.ql

@@ -26,6 +26,8 @@ where
   // At least for C programs on Windows, BOOL is a common typedef for a type
   // representing BoolType.
   not bf.getType().hasName("BOOL") and
+  // GLib's gboolean is a typedef for a type representing BoolType.
+  not bf.getType().hasName("gboolean") and
   // If this is true, then there cannot be unsigned sign extension or overflow.
   not bf.getDeclaredNumBits() = bf.getType().getSize() * 8 and
   not bf.isAnonymous() and

cpp/ql/src/Likely Bugs/Arithmetic/ComparisonWithCancelingSubExpr.ql

@@ -85,7 +85,8 @@ private predicate cancelingSubExprs(ComparisonOperation cmp, VariableAccess a1,
   exists(Variable v |
     exists(float m | m < 0 and cmpLinearSubVariable(cmp, v, a1, m)) and
     exists(float m | m > 0 and cmpLinearSubVariable(cmp, v, a2, m))
-  )
+  ) and
+  not any(ClassTemplateInstantiation inst).getATemplateArgument() = cmp.getParent*()
 }
 
 from ComparisonOperation cmp, VariableAccess a1, VariableAccess a2

cpp/ql/src/Likely Bugs/Arithmetic/PointlessSelfComparison.qll

@@ -29,7 +29,9 @@ predicate pointlessSelfComparison(ComparisonOperation cmp) {
     not exists(lhs.getQualifier()) and // Avoid structure fields
     not exists(rhs.getQualifier()) and // Avoid structure fields
     not convertedExprMightOverflow(lhs) and
-    not convertedExprMightOverflow(rhs)
+    not convertedExprMightOverflow(rhs) and
+    // Don't warn if the comparison is part of a template argument.
+    not any(ClassTemplateInstantiation inst).getATemplateArgument() = cmp.getParent*()
   )
 }
 

cpp/ql/src/Likely Bugs/Format/SnprintfOverflow.qhelp

@@ -3,7 +3,7 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>The return value of a call to <code>snprintf</code> is the number of characters that <i>would have</i> been written to the buffer assuming there was sufficient space.  In the event that the operation reaches the end of the buffer and more than one character is discarded, the return value will be greater than the buffer size.  This can cause incorrect behaviour, for example:
+<p>The return value of a call to <code>snprintf</code> is the number of characters that <i>would have</i> been written to the buffer assuming there was sufficient space.  In the event that the operation reaches the end of the buffer and more than one character is discarded, the return value will be greater than the buffer size.  This can cause incorrect behavior, for example:
 </p>
 </overview>
 

cpp/ql/src/Security/CWE/CWE-120/OverrunWrite.ql

@@ -21,14 +21,15 @@ import semmle.code.cpp.commons.Alloc
  * See CWE-120/UnboundedWrite.ql for a summary of CWE-120 alert cases.
  */
 
-from BufferWrite bw, Expr dest, int destSize
+from BufferWrite bw, Expr dest, int destSize, int estimated
 where
   not bw.hasExplicitLimit() and // has no explicit size limit
   dest = bw.getDest() and
   destSize = getBufferSize(dest, _) and
+  estimated = bw.getMaxDataLimited(_) and
   // we can deduce that too much data may be copied (even without
   // long '%f' conversions)
-  bw.getMaxDataLimited() > destSize
+  estimated > destSize
 select bw,
-  "This '" + bw.getBWDesc() + "' operation requires " + bw.getMaxData() +
+  "This '" + bw.getBWDesc() + "' operation requires " + estimated +
     " bytes but the destination is only " + destSize + " bytes."

cpp/ql/src/Security/CWE/CWE-120/OverrunWriteFloat.ql

@@ -21,14 +21,15 @@ import semmle.code.cpp.security.BufferWrite
  * See CWE-120/UnboundedWrite.ql for a summary of CWE-120 alert cases.
  */
 
-from BufferWrite bw, int destSize
+from BufferWrite bw, int destSize, int estimated, BufferWriteEstimationReason reason
 where
   not bw.hasExplicitLimit() and
   // has no explicit size limit
   destSize = getBufferSize(bw.getDest(), _) and
-  bw.getMaxData() > destSize and
+  estimated = bw.getMaxData(reason) and
+  estimated > destSize and
   // and we can deduce that too much data may be copied
-  bw.getMaxDataLimited() <= destSize // but it would fit without long '%f' conversions
+  bw.getMaxDataLimited(reason) <= destSize // but it would fit without long '%f' conversions
 select bw,
-  "This '" + bw.getBWDesc() + "' operation may require " + bw.getMaxData() +
+  "This '" + bw.getBWDesc() + "' operation may require " + estimated +
     " bytes because of float conversions, but the target is only " + destSize + " bytes."

cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql

@@ -44,7 +44,7 @@ import TaintedWithPath
 
 predicate isUnboundedWrite(BufferWrite bw) {
   not bw.hasExplicitLimit() and // has no explicit size limit
-  not exists(bw.getMaxData()) // and we can't deduce an upper bound to the amount copied
+  not exists(bw.getMaxData(_)) // and we can't deduce an upper bound to the amount copied
 }
 
 /*

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 that sends data over a network.
+ */
+class Send extends SendRecv instanceof RemoteFlowSinkFunction {
+  override Expr getSocketExpr(Call call) {
+    call.getTarget() = this and
+    exists(FunctionInput input, int arg |
+      super.hasSocketInput(input) and
+      input.isParameter(arg) and
+      result = call.getArgument(arg)
+    )
+  }
+
+  override Expr getDataExpr(Call call) {
+    call.getTarget() = this and
+    exists(FunctionInput input, int arg |
+      super.hasRemoteFlowSink(input, _) and
+      input.isParameterDeref(arg) and
+      result = call.getArgument(arg)
+    )
+  }
+}
+
+/**
+ * A function that receives data over a network.
+ */
+class Recv extends SendRecv instanceof RemoteFlowSourceFunction {
+  override Expr getSocketExpr(Call call) {
+    call.getTarget() = this and
+    exists(FunctionInput input, int arg |
+      super.hasSocketInput(input) and
+      input.isParameter(arg) and
+      result = call.getArgument(arg)
+    )
+  }
+
+  override Expr getDataExpr(Call call) {
+    call.getTarget() = this and
+    exists(FunctionOutput output, int arg |
+      super.hasRemoteFlowSource(output, _) and
+      output.isParameterDeref(arg) and
+      result = call.getArgument(arg)
+    )
+  }
+}
+
+/**
+ * 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.
+ */
+abstract class NetworkSendRecv extends FunctionCall {
+  SendRecv target;
+
+  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())
+        )
+      )
+    )
+  }
+
+  final Expr getDataExpr() { result = target.getDataExpr(this) }
 }
 
 /**
  * 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)
-    )
-  }
+  override Send target;
 }
 
 /**
  * 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)
-    )
-  }
+  override Recv target;
 }
 
 /**
- * Taint flow from a sensitive expression to a network operation with data
- * tainted by that expression.
+ * An expression that is an argument or return value from an encryption or
+ * decryption call.
  */
-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()))
+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()

cpp/ql/src/Security/CWE/CWE-319/UseOfHttp.ql

@@ -28,6 +28,11 @@ class PrivateHostName extends string {
   }
 }
 
+pragma[nomagic]
+predicate privateHostNameFlowsToExpr(Expr e) {
+  TaintTracking::localExprTaint(any(StringLiteral p | p.getValue() instanceof PrivateHostName), e)
+}
+
 /**
  * A string containing an HTTP URL not in a private domain.
  */
@@ -38,11 +43,9 @@ class HttpStringLiteral extends StringLiteral {
       or
       exists(string tail |
         tail = s.regexpCapture("http://(.*)", 1) and not tail instanceof PrivateHostName
-      ) and
-      not TaintTracking::localExprTaint(any(StringLiteral p |
-          p.getValue() instanceof PrivateHostName
-        ), this.getParent*())
-    )
+      )
+    ) and
+    not privateHostNameFlowsToExpr(this.getParent*())
   }
 }
 

cpp/ql/src/change-notes/2021-12-30-ambiguously-signed-bit-field.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* Added exception for GLib's gboolean to cpp/ambiguously-signed-bit-field.
+  This change reduces the number of false positives in the query.

cpp/ql/src/change-notes/released/0.0.4.md

@@ -0,0 +1,5 @@
+## 0.0.4
+
+### New Queries
+
+* A new query `cpp/non-https-url` has been added for C/C++. The query flags uses of `http` URLs that might be better replaced with `https`.

cpp/ql/src/change-notes/released/0.0.5.md

@@ -0,0 +1,6 @@
+## 0.0.5
+
+### New Queries
+
+* A new query `cpp/certificate-not-checked` has been added for C/C++. The query flags unsafe use of OpenSSL and similar libraries.
+* A new query `cpp/certificate-result-conflation` has been added for C/C++. The query flags unsafe use of OpenSSL and similar libraries.

cpp/ql/src/codeql-pack.release.yml

@@ -0,0 +1,2 @@
+---
+lastReleaseVersion: 0.0.5

cpp/ql/src/experimental/Security/CWE/CWE-266/IncorrectPrivilegeAssignment.cpp

@@ -0,0 +1,16 @@
+...
+  umask(0); // BAD
+...
+  maskOut = S_IRWXG | S_IRWXO;
+  umask(maskOut); // GOOD
+  ...
+  fchmod(fileno(fp), 0555 - maskOut); // BAD 
+  ...
+  fchmod(fileno(fp), 0555 & ~maskOut); // GOOD
+...
+  umask(0666);
+  chmod(pathname, 0666); // BAD
+...
+  umask(0022);
+  chmod(pathname, 0666); // GOOD
+...

cpp/ql/src/experimental/Security/CWE/CWE-266/IncorrectPrivilegeAssignment.qhelp

@@ -0,0 +1,23 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Finding  for function calls that set file permissions that may have errors in use. Incorrect arithmetic for calculating the resolution mask, using the same mask in opposite functions, using a mask that is too wide.</p>
+
+</overview>
+
+<example>
+<p>The following example demonstrates erroneous and fixed ways to use functions.</p>
+<sample src="IncorrectPrivilegeAssignment.cpp" />
+
+</example>
+<references>
+
+<li>
+  CERT C Coding Standard:
+  <a href="https://wiki.sei.cmu.edu/confluence/display/c/FIO06-C.+Create+files+with+appropriate+access+permissions">FIO06-C. Create files with appropriate access permissions</a>.
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-266/IncorrectPrivilegeAssignment.ql

@@ -0,0 +1,87 @@
+/**
+ * @name Find the wrong use of the umask function.
+ * @description Incorrectly evaluated argument to the umask function may have security implications.
+ * @kind problem
+ * @id cpp/wrong-use-of-the-umask
+ * @problem.severity warning
+ * @precision medium
+ * @tags correctness
+ *       maintainability
+ *       security
+ *       external/cwe/cwe-266
+ *       external/cwe/cwe-264
+ *       external/cwe/cwe-200
+ *       external/cwe/cwe-560
+ *       external/cwe/cwe-687
+ */
+
+import cpp
+import semmle.code.cpp.exprs.BitwiseOperation
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+
+/**
+ * An expression that is either a `BinaryArithmeticOperation` or the result of one or more `BinaryBitwiseOperation`s on a `BinaryArithmeticOperation`. For example `1 | (2 + 3)`.
+ */
+class ContainsArithmetic extends Expr {
+  ContainsArithmetic() {
+    this instanceof BinaryArithmeticOperation
+    or
+    // recursive search into `Operation`s
+    this.(BinaryBitwiseOperation).getAnOperand() instanceof ContainsArithmetic
+  }
+}
+
+/** Holds for a function `f` that has an argument at index `apos` used to set file permissions. */
+predicate numberArgumentModFunctions(Function f, int apos) {
+  f.hasGlobalOrStdName("umask") and apos = 0
+  or
+  f.hasGlobalOrStdName("fchmod") and apos = 1
+  or
+  f.hasGlobalOrStdName("chmod") and apos = 1
+}
+
+from FunctionCall fc, string msg, FunctionCall fcsnd
+where
+  fc.getTarget().hasGlobalOrStdName("umask") and
+  fc.getArgument(0).getValue() = "0" and
+  not exists(FunctionCall fctmp |
+    fctmp.getTarget().hasGlobalOrStdName("umask") and
+    not fctmp.getArgument(0).getValue() = "0"
+  ) and
+  exists(FunctionCall fctmp |
+    (
+      fctmp.getTarget().hasGlobalOrStdName("fopen") or
+      fctmp.getTarget().hasGlobalOrStdName("open")
+    ) and
+    not fctmp.getArgument(1).getValue().matches("r%") and
+    fctmp.getNumberOfArguments() = 2 and
+    not fctmp.getArgument(0).getValue() = "/dev/null" and
+    fcsnd = fctmp
+  ) and
+  not exists(FunctionCall fctmp |
+    fctmp.getTarget().hasGlobalOrStdName("chmod") or
+    fctmp.getTarget().hasGlobalOrStdName("fchmod")
+  ) and
+  msg = "Using umask(0) may not be safe with call $@."
+  or
+  fc.getTarget().hasGlobalOrStdName("umask") and
+  exists(FunctionCall fctmp |
+    (
+      fctmp.getTarget().hasGlobalOrStdName("chmod") or
+      fctmp.getTarget().hasGlobalOrStdName("fchmod")
+    ) and
+    (
+      globalValueNumber(fc.getArgument(0)) = globalValueNumber(fctmp.getArgument(1)) and
+      fc.getArgument(0).getValue() != "0"
+    ) and
+    msg = "Not use equal argument in umask and $@ functions." and
+    fcsnd = fctmp
+  )
+  or
+  exists(ContainsArithmetic exptmp, int i |
+    numberArgumentModFunctions(fc.getTarget(), i) and
+    globalValueNumber(exptmp) = globalValueNumber(fc.getArgument(i)) and
+    msg = "Using arithmetic to compute the mask in $@ may not be safe." and
+    fcsnd = fc
+  )
+select fc, msg, fcsnd, fcsnd.getTarget().getName()

cpp/ql/src/qlpack.yml

@@ -1,5 +1,6 @@
 name: codeql/cpp-queries
-version: 0.0.2
+version: 0.0.6-dev
+groups: cpp
 dependencies:
     codeql/cpp-all: "*"
     codeql/suite-helpers: "*"