C++: Restrict the 'check' to stat / access only as these are by far the more reliable results.

Author: geoffw0

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

@@ -87,9 +87,6 @@ where
     // a stat
     check = stat(checkPath, _)
     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)

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

@@ -1,18 +1,10 @@
-| 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: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:265:7:265:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:265:13:265:17 | path2 | filename | test2.cpp:263:7:263:12 | 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); // 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)
 
 		// ...
 	}
@@ -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); // DUBIOUS (bad but perhaps not exploitable)
 	}
 }
 
@@ -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)
 	}
 }
 
@@ -262,7 +262,7 @@ void test5_2(const char *path1, const char *path2)
 
 	if (!rename(path1, path2))
 	{
-		f = fopen(path2, "r"); // BAD
+		f = fopen(path2, "r"); // BAD [NOT DETECTED]
 	}
 }