Merge pull request #10471 from erik-krogh/tooRacy

JS: filter out "file read after existence check" from js/file-system-race

Author: erik-krogh

javascript/ql/src/Security/CWE-367/FileSystemRace.ql

@@ -18,32 +18,44 @@ import javascript
  * A call that checks a property of some file.
  */
 class FileCheck extends DataFlow::CallNode {
+  string member;
+
   FileCheck() {
-    this =
-      NodeJSLib::FS::moduleMember([
-          "open", "openSync", "exists", "existsSync", "stat", "statSync", "lstat", "lstatSync",
-          "fstat", "fstatSync", "access", "accessSync"
-        ]).getACall()
+    member =
+      [
+        "open", "openSync", "exists", "existsSync", "stat", "statSync", "lstat", "lstatSync",
+        "fstat", "fstatSync", "access", "accessSync"
+      ] and
+    this = NodeJSLib::FS::moduleMember(member).getACall()
   }
 
   DataFlow::Node getPathArgument() { result = this.getArgument(0) }
+
+  /** Holds if this call is a simple existence check for a file. */
+  predicate isExistsCheck() { member = ["exists", "existsSync"] }
 }
 
 /**
  * A call that modifies or otherwise interacts with a file.
  */
 class FileUse extends DataFlow::CallNode {
+  string member;
+
   FileUse() {
-    this =
-      NodeJSLib::FS::moduleMember([
-          // these are the six methods that accept file paths and file descriptors
-          "readFile", "readFileSync", "writeFile", "writeFileSync", "appendFile", "appendFileSync",
-          // don't use "open" after e.g. "access"
-          "open", "openSync"
-        ]).getACall()
+    member =
+      [
+        // these are the six methods that accept file paths and file descriptors
+        "readFile", "readFileSync", "writeFile", "writeFileSync", "appendFile", "appendFileSync",
+        // don't use "open" after e.g. "access"
+        "open", "openSync"
+      ] and
+    this = NodeJSLib::FS::moduleMember(member).getACall()
   }
 
   DataFlow::Node getPathArgument() { result = this.getArgument(0) }
+
+  /** Holds if this call reads from a file. */
+  predicate isFileRead() { member = ["readFile", "readFileSync"] }
 }
 
 /**
@@ -101,5 +113,6 @@ from FileCheck check, FileUse use
 where
   checkAndUseOnSame(check, use) and
   useAfterCheck(check, use) and
+  not (check.isExistsCheck() and use.isFileRead()) and // a read after an exists check is fine
   not getAFileHandle(DataFlow::TypeTracker::end()).flowsTo(use.getPathArgument())
 select use, "The file may have changed since it $@.", check, "was checked"

javascript/ql/test/query-tests/Security/CWE-367/tst.js

@@ -36,3 +36,8 @@ fs.access("myfile", (err) => {
     // ....
   });
 });
+
+const filePath3 = createFile();
+if (fs.existsSync(filePath3)) {
+  fs.readFileSync(filePath3); // OK - a read after an existence check is OK 
+}