Merge pull request github#6150 from geoffw0/toctou

C++: Tests for cpp/toctou-race-condition

Author: MathiasVP

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

@@ -16,10 +16,13 @@ import cpp
 import semmle.code.cpp.controlflow.Guards
 
 /**
- * An operation on a filename.
+ * An operation on a filename that is likely to modify the corresponding file
+ * and may return an indication of success.
  *
- * Note: we're not interested in operations on file descriptors, as they
- * are better behaved.
+ * Note: we're not interested in operations where the file is specified by a
+ * descriptor, rather than a filename, as they are better behaved. We are
+ * interested in functions that take a filename and return a file descriptor,
+ * however.
  */
 FunctionCall filenameOperation(Expr path) {
   exists(string name | name = result.getTarget().getName() |
@@ -48,7 +51,8 @@ FunctionCall filenameOperation(Expr path) {
 }
 
 /**
- * A use of `access` (or similar) on a filename.
+ * An operation on a filename that returns information in the return value but
+ * does not modify the corresponding file.  For example, `access`.
  */
 FunctionCall accessCheck(Expr path) {
   exists(string name | name = result.getTarget().getName() |
@@ -62,7 +66,9 @@ FunctionCall accessCheck(Expr path) {
 }
 
 /**
- * A use of `stat` (or similar) on a filename.
+ * An operation on a filename that returns information via a pointer argument
+ * and any return value, but does not modify the corresponding file.  For
+ * example, `stat`.
  */
 FunctionCall stat(Expr path, Expr buf) {
   exists(string name | name = result.getTarget().getName() |
@@ -77,7 +83,7 @@ FunctionCall stat(Expr path, Expr buf) {
 }
 
 /**
- * Holds if `use` points to `source`, either by being the same or by
+ * Holds if `use` refers to `source`, either by being the same or by
  * one step of variable indirection.
  */
 predicate referenceTo(Expr source, Expr use) {
@@ -88,36 +94,38 @@ predicate referenceTo(Expr source, Expr use) {
   )
 }
 
-from FunctionCall fc, Expr check, Expr checkUse, Expr opUse
+from Expr check, Expr checkPath, FunctionCall use, Expr usePath
 where
-  // checkUse looks like a check on a filename
+  // `check` looks like a check on a filename
   (
     // either:
     // an access check
-    check = accessCheck(checkUse)
+    check = accessCheck(checkPath)
     or
     // a stat
-    check = stat(checkUse, _)
+    check = stat(checkPath, _)
     or
     // another filename operation (null pointers can indicate errors)
-    check = filenameOperation(checkUse)
+    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(checkUse, buf.getAnAccess())) |
+    exists(Variable buf | exists(stat(checkPath, buf.getAnAccess())) |
       check.(VariableAccess).getQualifier() = buf.getAnAccess()
     )
   ) and
-  // checkUse and opUse refer to the same SSA variable
-  exists(SsaDefinition def, StackVariable v | def.getAUse(v) = checkUse and def.getAUse(v) = opUse) and
-  // opUse looks like an operation on a filename
-  fc = filenameOperation(opUse) and
-  // the return value of check is used (possibly with one step of
-  // variable indirection) in a guard which controls fc
+  // `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*()) |
-    guard.controls(fc.(ControlFlowNode).getBasicBlock(), _)
+    guard.controls(use.(ControlFlowNode).getBasicBlock(), _)
   )
-select fc,
+select use,
   "The $@ being operated upon was previously $@, but the underlying file may have been changed since then.",
-  opUse, "filename", check, "checked"
+  usePath, "filename", check, "checked"

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

@@ -1,3 +1,14 @@
+| 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: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 |

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); // BAD
+		remove(file1); // DUBIOUS (bad but perhaps not exploitable) [REPORTED]
 	}
 }
 
@@ -46,6 +46,6 @@ void test3()
 	create(file1);
 	if (!rename(file1, file2))
 	{
-		remove(file1); // BAD
+		remove(file1); // DUBIOUS (bad but perhaps not exploitable) [REPORTED]
 	}
 }