Merge branch 'main' into atorralba/promote-ognl-injection

Author: atorralba

cpp/change-notes/2021-06-15-path-sensitive-stack-reachability.md

@@ -0,0 +1,3 @@
+lgtm,codescanning
+* The "Cleartext storage of sensitive information in file" (cpp/cleartext-storage-file) query now uses dataflow to produce additional results.
+* Heuristics in the SensitiveExprs.qll library have been improved, making the "Cleartext storage of sensitive information in file" (cpp/cleartext-storage-file), "Cleartext storage of sensitive information in buffer" (cpp/cleartext-storage-buffer) and "Cleartext storage of sensitive information in an SQLite" (cpp/cleartext-storage-database) queries more accurate.

cpp/change-notes/2021-07-13-cleartext-storage-file.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Improvements have been made to the `cpp/toctou-race-condition` query, both to find more correct results and fewer false positive results.

cpp/change-notes/2021-07-20-toctou-race-condition.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Virtual function specifiers are now accessible via the new predicates on `Function` (`.isDeclaredVirtual`, `.isOverride`, and `.isFinal`).

cpp/change-notes/2021-07-29-virtual-function-declaration-specifiers.md

@@ -41,7 +41,7 @@ DeclStmt declWithNoInit(LocalVariable v) {
   result.getADeclaration() = v and
   not exists(v.getInitializer()) and
   /* The type of the variable is not stack-allocated. */
-  not allocatedType(v.getType())
+  exists(Type t | t = v.getType() | not allocatedType(t))
 }
 
 class UninitialisedLocalReachability extends StackVariableReachability {

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

@@ -5,7 +5,7 @@
  * @kind problem
  * @problem.severity warning
  * @security-severity 7.5
- * @precision medium
+ * @precision high
  * @id cpp/cleartext-storage-file
  * @tags security
  *       external/cwe/cwe-313
@@ -14,10 +14,40 @@
 import cpp
 import semmle.code.cpp.security.SensitiveExprs
 import semmle.code.cpp.security.FileWrite
+import semmle.code.cpp.dataflow.DataFlow
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 
-from FileWrite w, SensitiveExpr source, Expr dest
+/**
+ * An operation on a filename.
+ */
+predicate filenameOperation(FunctionCall op, Expr path) {
+  exists(string name | name = op.getTarget().getName() |
+    name =
+      [
+        "remove", "unlink", "rmdir", "rename", "fopen", "open", "freopen", "_open", "_wopen",
+        "_wfopen", "_fsopen", "_wfsopen", "chmod", "chown", "stat", "lstat", "fstat", "access",
+        "_access", "_waccess", "_access_s", "_waccess_s"
+      ] and
+    path = op.getArgument(0)
+    or
+    name = ["fopen_s", "wfopen_s", "rename"] and
+    path = op.getArgument(1)
+  )
+}
+
+predicate isFileName(GVN gvn) {
+  exists(FunctionCall op, Expr path |
+    filenameOperation(op, path) and
+    gvn = globalValueNumber(path)
+  )
+}
+
+from FileWrite w, SensitiveExpr source, Expr mid, Expr dest
 where
-  source = w.getASource() and
-  dest = w.getDest()
+  DataFlow::localFlow(DataFlow::exprNode(source), DataFlow::exprNode(mid)) and
+  mid = w.getASource() and
+  dest = w.getDest() and
+  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, "This write into file '" + dest.toString() + "' may contain unencrypted data from $@",
   source, "this source."

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

@@ -6,7 +6,7 @@
  * @kind problem
  * @problem.severity warning
  * @security-severity 7.7
- * @precision medium
+ * @precision high
  * @id cpp/toctou-race-condition
  * @tags security
  *       external/cwe/cwe-367
@@ -26,28 +26,29 @@ import semmle.code.cpp.controlflow.Guards
  */
 FunctionCall filenameOperation(Expr path) {
   exists(string name | name = result.getTarget().getName() |
-    (
-      name = "remove" or
-      name = "unlink" or
-      name = "rmdir" or
-      name = "rename" or
-      name = "chmod" or
-      name = "chown" or
-      name = "fopen" or
-      name = "open" or
-      name = "freopen" or
-      name = "_open" or
-      name = "_wopen" or
-      name = "_wfopen"
-    ) and
+    name =
+      [
+        "remove", "unlink", "rmdir", "rename", "fopen", "open", "freopen", "_open", "_wopen",
+        "_wfopen", "_fsopen", "_wfsopen"
+      ] and
     result.getArgument(0) = path
     or
-    (
-      name = "fopen_s" or
-      name = "wfopen_s"
-    ) and
+    name = ["fopen_s", "wfopen_s", "rename"] and
     result.getArgument(1) = path
   )
+  or
+  result = sensitiveFilenameOperation(path)
+}
+
+/**
+ * An operation on a filename that is likely to modify the security properties
+ * of the corresponding file and may return an indication of success.
+ */
+FunctionCall sensitiveFilenameOperation(Expr path) {
+  exists(string name | name = result.getTarget().getName() |
+    name = ["chmod", "chown"] and
+    result.getArgument(0) = path
+  )
 }
 
 /**
@@ -56,11 +57,7 @@ FunctionCall filenameOperation(Expr path) {
  */
 FunctionCall accessCheck(Expr path) {
   exists(string name | name = result.getTarget().getName() |
-    name = "access" or
-    name = "_access" or
-    name = "_waccess" or
-    name = "_access_s" or
-    name = "_waccess_s"
+    name = ["access", "_access", "_waccess", "_access_s", "_waccess_s"]
   ) and
   path = result.getArgument(0)
 }
@@ -72,9 +69,7 @@ FunctionCall accessCheck(Expr path) {
  */
 FunctionCall stat(Expr path, Expr buf) {
   exists(string name | name = result.getTarget().getName() |
-    name = "stat" or
-    name = "lstat" or
-    name = "fstat" or
+    name = ["stat", "lstat", "fstat"] or
     name.matches("\\_stat%") or
     name.matches("\\_wstat%")
   ) and
@@ -98,29 +93,36 @@ from Expr check, Expr checkPath, FunctionCall use, Expr usePath
 where
   // `check` looks like a check on a filename
   (
-    // either:
-    // an access check
-    check = accessCheck(checkPath)
-    or
-    // a stat
-    check = stat(checkPath, _)
+    (
+      // either:
+      // an access check
+      check = accessCheck(checkPath)
+      or
+      // a stat
+      check = stat(checkPath, _)
+      or
+      // access to a member variable on the stat buf
+      // (morally, this should be a use-use pair, but it seems unlikely
+      // that this variable will get reused in practice)
+      exists(Expr call, Expr e, Variable v |
+        call = stat(checkPath, e) and
+        e.getAChild*().(VariableAccess).getTarget() = v and
+        check.(VariableAccess).getTarget() = v and
+        not e.getAChild*() = check // the call that writes to the pointer is not where the pointer is checked.
+      )
+    ) and
+    // `op` looks like an operation on a filename
+    use = filenameOperation(usePath)
     or
     // another filename operation (null pointers can indicate errors)
-    check = filenameOperation(checkPath)
-    or
-    // access to a member variable on the stat buf
-    // (morally, this should be a use-use pair, but it seems unlikely
-    // that this variable will get reused in practice)
-    exists(Variable buf | exists(stat(checkPath, buf.getAnAccess())) |
-      check.(VariableAccess).getQualifier() = buf.getAnAccess()
-    )
+    check = filenameOperation(checkPath) and
+    // `op` looks like a sensitive operation on a filename
+    use = sensitiveFilenameOperation(usePath)
   ) and
   // `checkPath` and `usePath` refer to the same SSA variable
   exists(SsaDefinition def, StackVariable v |
     def.getAUse(v) = checkPath and def.getAUse(v) = usePath
   ) and
-  // `op` looks like an operation on a filename
-  use = filenameOperation(usePath) and
   // the return value of `check` is used (possibly with one step of
   // variable indirection) in a guard which controls `use`
   exists(GuardCondition guard | referenceTo(check, guard.getAChild*()) |

cpp/ql/src/Security/CWE/CWE-367/TOCTOUFilesystemRace.ql

@@ -82,9 +82,23 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function {
   /** Holds if this function is inline. */
   predicate isInline() { this.hasSpecifier("inline") }
 
-  /** Holds if this function is virtual. */
+  /**
+   * Holds if this function is virtual.
+   *
+   * Unlike `isDeclaredVirtual()`, `isVirtual()` holds even if the function
+   * is not explicitly declared with the `virtual` specifier.
+   */
   predicate isVirtual() { this.hasSpecifier("virtual") }
 
+  /** Holds if this function is declared with the `virtual` specifier. */
+  predicate isDeclaredVirtual() { this.hasSpecifier("declared_virtual") }
+
+  /** Holds if this function is declared with the `override` specifier. */
+  predicate isOverride() { this.hasSpecifier("override") }
+
+  /** Holds if this function is declared with the `final` specifier. */
+  predicate isFinal() { this.hasSpecifier("final") }
+
   /**
    * Holds if this function is deleted.
    * This may be because it was explicitly deleted with an `= delete`

cpp/ql/src/semmle/code/cpp/Function.qll

@@ -208,7 +208,7 @@ private predicate bbSuccessorEntryReachesDefOrUse(
   boolean skipsFirstLoopAlwaysTrueUponEntry
 ) {
   exists(BasicBlock succ, boolean succSkipsFirstLoopAlwaysTrueUponEntry |
-    bbSuccessorEntryReachesLoopInvariant0(bb, succ, skipsFirstLoopAlwaysTrueUponEntry,
+    bbSuccessorEntryReachesLoopInvariant(bb, succ, skipsFirstLoopAlwaysTrueUponEntry,
       succSkipsFirstLoopAlwaysTrueUponEntry)
   |
     bbEntryReachesDefOrUseLocally(succ, v, defOrUse) and