Merge branch 'main' into post-release-prep/codeql-cli-2.8.0

cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql

@@ -14,6 +14,9 @@
  */
 
 import cpp
+// We don't actually use the global value numbering library in this query, but without it we end up
+// recomputing the IR.
+private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import semmle.code.cpp.ir.IR
 import semmle.code.cpp.ir.dataflow.DataFlow::DataFlow
 

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

@@ -12,23 +12,33 @@
  */
 
 import cpp
-import semmle.code.cpp.security.BufferWrite
-import semmle.code.cpp.security.TaintTracking
+import semmle.code.cpp.security.BufferWrite as BufferWrite
 import semmle.code.cpp.security.SensitiveExprs
-import TaintedWithPath
+import semmle.code.cpp.security.FlowSources
+import semmle.code.cpp.ir.dataflow.TaintTracking
+import DataFlow::PathGraph
 
-class Configuration extends TaintTrackingConfiguration {
-  override predicate isSink(Element tainted) { exists(BufferWrite w | w.getASource() = tainted) }
+/**
+ * Taint flow from user input to a buffer write.
+ */
+class ToBufferConfiguration extends TaintTracking::Configuration {
+  ToBufferConfiguration() { this = "ToBufferConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof FlowSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(BufferWrite::BufferWrite w | w.getASource() = sink.asExpr())
+  }
 }
 
 from
-  BufferWrite w, Expr taintedArg, Expr taintSource, PathNode sourceNode, PathNode sinkNode,
-  string taintCause, SensitiveExpr dest
+  ToBufferConfiguration config, BufferWrite::BufferWrite w, DataFlow::PathNode sourceNode,
+  DataFlow::PathNode sinkNode, FlowSource source, SensitiveExpr dest
 where
-  taintedWithPath(taintSource, taintedArg, sourceNode, sinkNode) and
-  isUserInput(taintSource, taintCause) and
-  w.getASource() = taintedArg and
+  config.hasFlowPath(sourceNode, sinkNode) and
+  sourceNode.getNode() = source and
+  w.getASource() = sinkNode.getNode().asExpr() and
   dest = w.getDest()
 select w, sourceNode, sinkNode,
-  "This write into buffer '" + dest.toString() + "' may contain unencrypted data from $@",
-  taintSource, "user input (" + taintCause + ")"
+  "This write into buffer '" + dest.toString() + "' may contain unencrypted data from $@", source,
+  "user input (" + source.getSourceType() + ")"

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

@@ -65,6 +65,7 @@ where
   midNode.getNode().asExpr() = mid and
   mid = w.getASource() and
   dest = w.getDest() and
+  not dest.(VariableAccess).getTarget().getName() = ["stdin", "stdout", "stderr"] and // exclude calls with standard streams
   not isFileName(globalValueNumber(source)) and // file names are not passwords
   not exists(string convChar | convChar = w.getSourceConvChar(mid) | not convChar = ["s", "S"]) // ignore things written with other conversion characters
 select w, sourceNode, midNode,

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

@@ -5,7 +5,7 @@
  * @kind path-problem
  * @problem.severity warning
  * @security-severity 7.5
- * @precision medium
+ * @precision high
  * @id cpp/cleartext-transmission
  * @tags security
  *       external/cwe/cwe-319
@@ -14,8 +14,8 @@
 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 semmle.code.cpp.commons.File
 import DataFlow::PathGraph
 
 /**
@@ -27,6 +27,7 @@ class SensitiveNode extends DataFlow::Node {
     this.asExpr() = any(SensitiveVariable sv).getInitializer().getExpr() or
     this.asExpr().(VariableAccess).getTarget() =
       any(SensitiveVariable sv).(GlobalOrNamespaceVariable) or
+    this.asExpr().(VariableAccess).getTarget() = any(SensitiveVariable v | v instanceof Field) or
     this.asUninitialized() instanceof SensitiveVariable or
     this.asParameter() instanceof SensitiveVariable or
     this.asExpr().(FunctionCall).getTarget() instanceof SensitiveFunction
@@ -58,7 +59,10 @@ class Send extends SendRecv instanceof RemoteFlowSinkFunction {
     call.getTarget() = this and
     exists(FunctionInput input, int arg |
       super.hasSocketInput(input) and
-      input.isParameter(arg) and
+      (
+        input.isParameter(arg) or
+        input.isParameterDeref(arg)
+      ) and
       result = call.getArgument(arg)
     )
   }
@@ -81,7 +85,10 @@ class Recv extends SendRecv instanceof RemoteFlowSourceFunction {
     call.getTarget() = this and
     exists(FunctionInput input, int arg |
       super.hasSocketInput(input) and
-      input.isParameter(arg) and
+      (
+        input.isParameter(arg) or
+        input.isParameterDeref(arg)
+      ) and
       result = call.getArgument(arg)
     )
   }
@@ -114,24 +121,32 @@ abstract class NetworkSendRecv extends FunctionCall {
   NetworkSendRecv() {
     this.getTarget() = target and
     // exclude calls based on the socket...
-    not exists(GVN g |
-      g = globalValueNumber(target.getSocketExpr(this)) and
+    not exists(DataFlow::Node src, DataFlow::Node dest |
+      DataFlow::localFlow(src, dest) and
+      dest.asExpr() = target.getSocketExpr(this) and
       (
         // literal constant
-        globalValueNumber(any(Literal l)) = g
+        src.asExpr() instanceof Literal
         or
         // variable (such as a global) initialized to a literal constant
         exists(Variable v |
           v.getInitializer().getExpr() instanceof Literal and
-          g = globalValueNumber(v.getAnAccess())
+          src.asExpr() = v.getAnAccess()
         )
         or
         // result of a function call with literal inputs (likely constant)
+        forex(Expr arg | arg = src.asExpr().(FunctionCall).getAnArgument() | arg instanceof Literal)
+        or
+        // variable called `stdin`, `stdout` or `stderr`
+        src.asExpr().(VariableAccess).getTarget().getName() = ["stdin", "stdout", "stderr"]
+        or
+        // open of `"/dev/tty"`
         exists(FunctionCall fc |
-          forex(Expr arg | arg = fc.getAnArgument() | arg instanceof Literal) and
-          g = globalValueNumber(fc)
+          fopenCall(fc) and
+          fc.getAnArgument().getValue() = "/dev/tty" and
+          src.asExpr() = fc
         )
-        // (this is far from exhaustive)
+        // (this is not exhaustive)
       )
     )
   }

cpp/ql/src/Security/CWE/CWE-732/DoNotCreateWorldWritable.ql

@@ -12,17 +12,16 @@
 
 import cpp
 import FilePermissions
-import semmle.code.cpp.commons.unix.Constants
 
 predicate worldWritableCreation(FileCreationExpr fc, int mode) {
   mode = localUmask(fc).mask(fc.getMode()) and
-  sets(mode, s_iwoth())
+  setsAnyBits(mode, UnixConstants::s_iwoth())
 }
 
 predicate setWorldWritable(FunctionCall fc, int mode) {
   fc.getTarget().getName() = ["chmod", "fchmod", "_chmod", "_wchmod"] and
   mode = fc.getArgument(1).getValue().toInt() and
-  sets(mode, s_iwoth())
+  setsAnyBits(mode, UnixConstants::s_iwoth())
 }
 
 from Expr fc, int mode, string message

cpp/ql/src/Security/CWE/CWE-732/FilePermissions.qll

@@ -1,5 +1,49 @@
 import cpp
-import semmle.code.cpp.commons.unix.Constants
+import semmle.code.cpp.commons.unix.Constants as UnixConstants
+
+/**
+ * Gets the number corresponding to the contents of `input` in base-16.
+ * Note: the first two characters of `input` must be `0x`. For example:
+ * `parseHex("0x123abc") = 1194684`.
+ */
+bindingset[input]
+int parseHex(string input) {
+  exists(string lowerCaseInput | lowerCaseInput = input.toLowerCase() |
+    lowerCaseInput.regexpMatch("0x[0-9a-f]+") and
+    result =
+      strictsum(int ix |
+        ix in [2 .. input.length()]
+      |
+        16.pow(input.length() - (ix + 1)) * "0123456789abcdef".indexOf(lowerCaseInput.charAt(ix))
+      )
+  )
+}
+
+/**
+ * Gets the value defined by the `O_CREAT` macro if the macro
+ * exists and if every definition defines the same value.
+ */
+int o_creat() {
+  result =
+    unique(int v |
+      exists(Macro m | m.getName() = "O_CREAT" |
+        v = parseHex(m.getBody()) or v = UnixConstants::parseOctal(m.getBody())
+      )
+    )
+}
+
+/**
+ * Gets the value defined by the `O_TMPFILE` macro if the macro
+ * exists and if every definition defines the same value.
+ */
+int o_tmpfile() {
+  result =
+    unique(int v |
+      exists(Macro m | m.getName() = "O_TMPFILE" |
+        v = parseHex(m.getBody()) or v = UnixConstants::parseOctal(m.getBody())
+      )
+    )
+}
 
 bindingset[n, digit]
 private string octalDigit(int n, int digit) {
@@ -20,11 +64,17 @@ string octalFileMode(int mode) {
   else result = "[non-standard mode: decimal " + mode + "]"
 }
 
+/**
+ * Holds if the bitmask `value` sets the bits in `flag`.
+ */
+bindingset[value, flag]
+predicate setsFlag(int value, int flag) { value.bitAnd(flag) = flag }
+
 /**
  * Holds if the bitmask `mask` sets any of the bit fields in `fields`.
  */
 bindingset[mask, fields]
-predicate sets(int mask, int fields) { mask.bitAnd(fields) != 0 }
+predicate setsAnyBits(int mask, int fields) { mask.bitAnd(fields) != 0 }
 
 /**
  * Gets the value that `fc` sets the umask to, if `fc` is a call to
@@ -83,16 +133,24 @@ abstract class FileCreationExpr extends FunctionCall {
   abstract int getMode();
 }
 
-class OpenCreationExpr extends FileCreationExpr {
+abstract class FileCreationWithOptionalModeExpr extends FileCreationExpr {
+  abstract predicate hasModeArgument();
+}
+
+class OpenCreationExpr extends FileCreationWithOptionalModeExpr {
   OpenCreationExpr() {
-    this.getTarget().getName() = ["open", "_open", "_wopen"] and
-    sets(this.getArgument(1).getValue().toInt(), o_creat())
+    this.getTarget().hasGlobalOrStdName(["open", "_open", "_wopen"]) and
+    exists(int flag | flag = this.getArgument(1).getValue().toInt() |
+      setsFlag(flag, o_creat()) or setsFlag(flag, o_tmpfile())
+    )
   }
 
   override Expr getPath() { result = this.getArgument(0) }
 
+  override predicate hasModeArgument() { exists(this.getArgument(2)) }
+
   override int getMode() {
-    if exists(this.getArgument(2))
+    if this.hasModeArgument()
     then result = this.getArgument(2).getValue().toInt()
     else
       // assume anything is permitted
@@ -108,20 +166,35 @@ class CreatCreationExpr extends FileCreationExpr {
   override int getMode() { result = this.getArgument(1).getValue().toInt() }
 }
 
-class OpenatCreationExpr extends FileCreationExpr {
+class OpenatCreationExpr extends FileCreationWithOptionalModeExpr {
   OpenatCreationExpr() {
-    this.getTarget().getName() = "openat" and
-    this.getNumberOfArguments() = 4
+    this.getTarget().hasGlobalOrStdName("openat") and
+    exists(int flag | flag = this.getArgument(2).getValue().toInt() |
+      setsFlag(flag, o_creat()) or setsFlag(flag, o_tmpfile())
+    )
   }
 
   override Expr getPath() { result = this.getArgument(1) }
 
-  override int getMode() { result = this.getArgument(3).getValue().toInt() }
+  override predicate hasModeArgument() { exists(this.getArgument(3)) }
+
+  override int getMode() {
+    if this.hasModeArgument()
+    then result = this.getArgument(3).getValue().toInt()
+    else
+      // assume anything is permitted
+      result = 0.bitNot()
+  }
 }
 
 private int fopenMode() {
   result =
-    s_irusr().bitOr(s_irgrp()).bitOr(s_iroth()).bitOr(s_iwusr()).bitOr(s_iwgrp()).bitOr(s_iwoth())
+    UnixConstants::s_irusr()
+        .bitOr(UnixConstants::s_irgrp())
+        .bitOr(UnixConstants::s_iroth())
+        .bitOr(UnixConstants::s_iwusr())
+        .bitOr(UnixConstants::s_iwgrp())
+        .bitOr(UnixConstants::s_iwoth())
 }
 
 class FopenCreationExpr extends FileCreationExpr {
@@ -153,6 +226,6 @@ class FopensCreationExpr extends FileCreationExpr {
     // fopen_s has restrictive permissions unless you have "u" in the mode
     if this.getArgument(2).getValue().charAt(_) = "u"
     then result = fopenMode()
-    else result = s_irusr().bitOr(s_iwusr())
+    else result = UnixConstants::s_irusr().bitOr(UnixConstants::s_iwusr())
   }
 }

cpp/ql/src/Security/CWE/CWE-732/OpenCallMissingModeArgument.c

@@ -0,0 +1,9 @@
+int open_file_bad() {
+	// BAD - this uses arbitrary bytes from the stack as mode argument
+        return open(FILE, O_CREAT)
+}
+
+int open_file_good() {
+	// GOOD - the mode argument is supplied
+        return open(FILE, O_CREAT, S_IRUSR | S_IWUSR)
+}

cpp/ql/src/Security/CWE/CWE-732/OpenCallMissingModeArgument.qhelp

@@ -0,0 +1,31 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+When opening a file with the <code>O_CREAT</code> or <code>O_TMPFILE</code> flag, the <code>mode</code> must
+be supplied. If the <code>mode</code> argument is omitted, some arbitrary bytes from the stack will be used
+as the file mode. This leaks some bits from the stack into the permissions of the file.
+</p>
+</overview>
+
+<recommendation>
+<p>
+The <code>mode</code> must be supplied when <code>O_CREAT</code> or <code>O_TMPFILE</code> is specified.
+</p>
+</recommendation>
+
+<example>
+<p>
+The first example opens a file with the <code>O_CREAT</code> flag without supplying the <code>mode</code>
+argument. In this case arbitrary bytes from the stack will be used as <code>mode</code> argument. The
+second example correctly supplies the <code>mode</code> argument and creates a file that is user readable
+and writable.
+</p>
+
+<sample src="OpenCallMissingModeArgument.c" />
+
+</example>
+</qhelp>

cpp/ql/src/Security/CWE/CWE-732/OpenCallMissingModeArgument.ql

@@ -0,0 +1,19 @@
+/**
+ * @name File opened with O_CREAT flag but without mode argument
+ * @description Opening a file with the O_CREAT flag but without mode argument reads arbitrary bytes from the stack.
+ * @kind problem
+ * @problem.severity error
+ * @security-severity 7.8
+ * @precision high
+ * @id cpp/open-call-with-mode-argument
+ * @tags security
+ *       external/cwe/cwe-732
+ */
+
+import cpp
+import FilePermissions
+
+from FileCreationWithOptionalModeExpr fc
+where not fc.hasModeArgument()
+select fc,
+  "A file is created here without providing a mode argument, which may leak bits from the stack."

cpp/ql/src/change-notes/2022-01-22-cleartext-transmission.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The "Cleartext transmission of sensitive information" (`cpp/cleartext-transmission`) query now finds more results, where a password is stored in a struct field or class member variable.

cpp/ql/src/change-notes/2022-01-25-cleartext-storage-buffer.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `cpp/cleartext-storage-buffer` query has been updated to use the `semmle.code.cpp.dataflow.TaintTracking` library.

cpp/ql/src/change-notes/2022-01-28-cleartext-storage-file.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `cpp/cleartext-storage-file` query has been improved, removing false positives where data is written to a standard output stream.

cpp/ql/src/change-notes/2022-02-22-cleartext-transmission.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The "Cleartext transmission of sensitive information" (`cpp/cleartext-transmission`) query has been further improved to reduce false positive results, and upgraded from `medium` to `high` precision.

cpp/ql/src/experimental/Best Practices/UselessTest.cpp

@@ -0,0 +1,9 @@
+void test(){
+    int a = 8;
+    int b = 9;
+
+    //Useless NonEquals
+    if(a==8 && a != 7) {}
+
+    while(a==8 && a!=7){}
+}

cpp/ql/src/experimental/Best Practices/UselessTest.qhelp

@@ -0,0 +1,18 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>Comparison operations like <code>a==8 &amp;&amp; a!=7</code> contain a useless part : the non-equal part. This rule finds tests of this kind within an <code>if</code> or a <code>while</code> statement</p>
+</overview>
+
+<recommendation>
+<p>Remove the useless comparisons</p>
+</recommendation>
+
+<example>
+<sample src="UselessTest.cpp" />
+</example>
+
+</qhelp>

cpp/ql/src/experimental/Best Practices/UselessTest.ql

@@ -0,0 +1,45 @@
+/**
+ * @name Useless Test
+ * @description A boolean condition that is guaranteed to never be evaluated should be deleted.
+ * @kind problem
+ * @problem.severity warning
+ * @id cpp/uselesstest
+ * @tags reliability
+ *       readability
+ */
+
+import cpp
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+
+predicate sameExpr(Expr e1, Expr e2) { globalValueNumber(e1).getAnExpr() = e2 }
+
+Element nearestParent(Expr e) {
+  if
+    e.getParent().(Expr).getConversion*() instanceof ParenthesisExpr or
+    e.getParent() instanceof IfStmt or
+    e.getParent() instanceof WhileStmt
+  then result = e.getParent()
+  else result = nearestParent(e.getParent())
+}
+
+from LogicalAndExpr b, EQExpr eq, NEExpr ne
+where
+  (
+    b.getAChild*() = eq and
+    b.getAChild*() = ne and
+    eq.getParent() instanceof LogicalAndExpr and
+    ne.getParent() instanceof LogicalAndExpr
+  ) and
+  (
+    eq.getLeftOperand() instanceof VariableAccess and ne.getLeftOperand() instanceof VariableAccess
+    or
+    eq.getLeftOperand() instanceof PointerDereferenceExpr and
+    ne.getLeftOperand() instanceof PointerDereferenceExpr
+  ) and
+  eq.getRightOperand() instanceof Literal and
+  ne.getRightOperand() instanceof Literal and
+  eq.getLeftOperand().getFullyConverted().getUnspecifiedType() =
+    ne.getLeftOperand().getFullyConverted().getUnspecifiedType() and
+  nearestParent(eq) = nearestParent(ne) and
+  sameExpr(eq.getLeftOperand(), ne.getLeftOperand())
+select ne, "Useless Test"

cpp/ql/src/qlpack.yml

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