C++: Accept any check followed by a 'sensitive' use such as 'chmod'.

Author: geoffw0

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

@@ -28,14 +28,27 @@ FunctionCall filenameOperation(Expr path) {
   exists(string name | name = result.getTarget().getName() |
     name =
       [
-        "remove", "unlink", "rmdir", "rename", "chmod", "chown", "fopen", "open", "freopen",
-        "_open", "_wopen", "_wfopen", "_wfsopen"
+        "remove", "unlink", "rmdir", "rename", "fopen", "open", "freopen", "_open", "_wopen",
+        "_wfopen", "_wfsopen"
       ] and
     result.getArgument(0) = path
     or
     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
+  )
 }
 
 /**
@@ -80,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
-    // 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.
-    )
+    // another filename operation (null pointers can indicate errors)
+    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/test/query-tests/Security/CWE/CWE-367/semmle/TOCTOUFilesystemRace.expected

@@ -5,6 +5,9 @@
 | test2.cpp:130:7:130:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:130:13:130:16 | path | filename | test2.cpp:128:21:128:27 | buf_ptr | checked |
 | test2.cpp:157:7:157:10 | call to open | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:157:12:157:15 | path | filename | test2.cpp:155:6:155:9 | call to stat | checked |
 | test2.cpp:170:7:170:10 | call to open | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:170:12:170:15 | path | filename | test2.cpp:168:6:168:10 | call to lstat | checked |
+| test2.cpp:245:3:245:7 | call to chmod | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:245:9:245:12 | path | filename | test2.cpp:238:6:238:10 | call to fopen | checked |
 | test2.cpp:277:7:277:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:277:13:277:16 | path | filename | test2.cpp:275:6:275:11 | call to access | checked |
 | test2.cpp:303:7:303:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:303:13:303:16 | path | filename | test2.cpp:301:7:301:12 | call to access | checked |
 | test2.cpp:317:7:317:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:317:13:317:16 | path | filename | test2.cpp:313:6:313:11 | call to access | checked |
+| test2.cpp:348:3:348:7 | call to chmod | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:348:9:348:12 | path | filename | test2.cpp:341:6:341:10 | call to fopen | checked |
+| test2.cpp:356:3:356:7 | call to chmod | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:356:9:356:13 | path2 | filename | test2.cpp:354:7:354:12 | call to rename | checked |

cpp/ql/test/query-tests/Security/CWE/CWE-367/semmle/test2.cpp

@@ -242,7 +242,7 @@ void test4_1(const char *path)
 
 		fclose(f);
 
-		chmod(path, 0); // DUBIOUS (bad but perhaps not exploitable)
+		chmod(path, 0); // BAD
 	}
 }
 
@@ -345,14 +345,14 @@ void test7_1(const char *path)
 
 		fclose(f);
 
-		chmod(path, 1234); // BAD [NOT DETECTED]
+		chmod(path, 1234); // BAD
 	}
 }
 
 void test7_1(const char *path1, const char *path2)
 {
 	if (!rename(path1, path2))
 	{
-		chmod(path2, 1234); // BAD [NOT DETECTED]
+		chmod(path2, 1234); // BAD
 	}
 }