Merge pull request github#6335 from geoffw0/toctou2

C++: Improvements to the cpp/toctou-race-condition query

Author: Robert Marsh

cpp/change-notes/2021-07-20-toctou-race-condition.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/ql/src/Security/CWE/CWE-367/TOCTOUFilesystemRace.ql

@@ -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/test/query-tests/Security/CWE/CWE-367/semmle/TOCTOUFilesystemRace.expected

@@ -1,14 +1,13 @@
-| test2.cpp:39:7:39:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:39:13:39:16 | path | filename | test2.cpp:34:6:34:10 | call to fopen | checked |
-| test2.cpp:52:7:52:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:52:13:52:16 | path | filename | test2.cpp:52:7:52:11 | call to fopen | checked |
 | test2.cpp:69:7:69:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:69:13:69:16 | path | filename | test2.cpp:67:6:67:9 | call to stat | checked |
-| test2.cpp:98:7:98:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:98:13:98:16 | path | filename | test2.cpp:96:15:96:17 | foo | checked |
+| test2.cpp:83:7:83:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:83:13:83:16 | path | filename | test2.cpp:81:6:81:8 | buf | checked |
+| test2.cpp:98:7:98:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:98:13:98:16 | path | filename | test2.cpp:96:6:96:12 | buf_ptr | checked |
+| test2.cpp:115:7:115:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:115:13:115:16 | path | filename | test2.cpp:113:22:113:24 | buf | checked |
+| 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:255:3:255:8 | call to remove | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:255:10:255:14 | path1 | filename | test2.cpp:253:6:253:11 | call to rename | 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 |
-| test.cpp:21:3:21:8 | call to remove | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test.cpp:21:10:21:14 | file1 | filename | test.cpp:19:7:19:12 | call to rename | checked |
-| test.cpp:35:3:35:8 | call to remove | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test.cpp:35:10:35:14 | file1 | filename | test.cpp:32:7:32:12 | call to rename | checked |
-| test.cpp:49:3:49:8 | call to remove | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test.cpp:49:10:49:14 | file1 | filename | test.cpp:47:7:47:12 | call to rename | 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/test.cpp

@@ -18,7 +18,7 @@ void test1()
 	create(file1);
 	if (!rename(file1, file2))
 	{
-		remove(file1); // DUBIOUS (bad but perhaps not exploitable) [REPORTED]
+		remove(file1); // DUBIOUS (bad but perhaps not exploitable)
 	}
 }
 
@@ -32,7 +32,7 @@ void test2()
 	if (!rename(file1, file2))
 	{
 		file1.set("d.txt");
-		remove(file1); // GOOD [FALSE POSITIVE]
+		remove(file1); // GOOD
 	}
 }
 
@@ -46,6 +46,6 @@ void test3()
 	create(file1);
 	if (!rename(file1, file2))
 	{
-		remove(file1); // DUBIOUS (bad but perhaps not exploitable) [REPORTED]
+		remove(file1); // DUBIOUS (bad but perhaps not exploitable)
 	}
 }

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

@@ -36,7 +36,7 @@ void test1_1(const char *path)
 	if (f == NULL)
 	{
 		// retry
-		f = fopen(path, "r"); // GOOD (this is just trying again) [FALSE POSITIVE]
+		f = fopen(path, "r"); // GOOD (this is just trying again)
 	}
 
 	// ...
@@ -49,7 +49,7 @@ void test1_2(const char *path)
 	// try until we succeed
 	while (f == NULL)
 	{
-		f = fopen(path, "r"); // GOOD (this is just trying again) [FALSE POSITIVE]
+		f = fopen(path, "r"); // GOOD (this is just trying again)
 
 		// ...
 	}
@@ -80,7 +80,7 @@ void test2_2(const char *path)
 	stat(path, &buf);
 	if (buf.foo > 0)
 	{
-		f = fopen(path, "r"); // BAD [NOT DETECTED]
+		f = fopen(path, "r"); // BAD
 	}
 
 	// ...
@@ -112,7 +112,7 @@ void test2_4(const char *path)
 	stat(path, &buf);
 	if (stat_condition(&buf))
 	{
-		f = fopen(path, "r"); // BAD [NOT DETECTED]
+		f = fopen(path, "r"); // BAD
 	}
 
 	// ...
@@ -127,7 +127,7 @@ void test2_5(const char *path)
 	stat(path, buf_ptr);
 	if (stat_condition(buf_ptr))
 	{
-		f = fopen(path, "r"); // BAD [NOT DETECTED]
+		f = fopen(path, "r"); // BAD
 	}
 
 	// ...
@@ -242,7 +242,7 @@ void test4_1(const char *path)
 
 		fclose(f);
 
-		chmod(path, 0); // DUBIOUS (bad but perhaps not exploitable) [REPORTED]
+		chmod(path, 0); // BAD
 	}
 }
 
@@ -252,7 +252,7 @@ void test5_1(const char *path1, const char *path2)
 {
 	if (rename(path1, path2))
 	{
-		remove(path1); // DUBIOUS (bad but perhaps not exploitable) [REPORTED]
+		remove(path1); // DUBIOUS (bad but perhaps not exploitable)
 	}
 }
 
@@ -331,3 +331,28 @@ void test6_5(const char *path1, const char *path2)
 		// ...
 	}
 }
+
+// --- open / rename -> chmod ---
+
+void test7_1(const char *path)
+{
+	FILE *f;
+
+	f = fopen(path, "wt");
+	if (f != 0)
+	{
+		// ...
+	
+		fclose(f);
+
+		chmod(path, 1234); // BAD
+	}
+}
+
+void test7_1(const char *path1, const char *path2)
+{
+	if (!rename(path1, path2))
+	{
+		chmod(path2, 1234); // BAD
+	}
+}