Merge pull request #7773 from erik-krogh/CWE-367

JS: add a js/file-system-race query

Author: erik-krogh

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

@@ -0,0 +1,61 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Often it is necessary to check the state of a file before using it. These checks usually take a file name
+to be checked, and if the check returns positively, then the file is opened or otherwise operated upon.
+</p>
+
+<p>
+However, in the time between the check and the operation, the underlying file referenced by the
+file name could be changed by an attacker, causing unexpected behavior.
+</p>
+
+</overview>
+
+<recommendation>
+<p>
+Use file descriptors instead of file names whenever possible.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows a case where the code checks whether a file inside the <code>/tmp/</code> folder exists, 
+and if it doesn't, the file is written to that location.
+</p>
+
+<sample src="examples/file-race-bad.js" />
+
+<p>
+However, in a multi-user environment the file might be created by another user between the existence check and the write. 
+</p>
+
+<p>
+This can be avoided by using <code>fs.open</code> to get a file descriptor, and then use that file descriptor in the write operation.
+</p>
+
+<sample src="examples/file-race-good.js" />
+
+</example>
+
+<references>
+<li>
+Wikipedia: <a href="https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use">Time-of-check to time-of-use</a>.
+</li>
+
+<li>
+  The CERT Oracle Secure Coding Standard for C:
+  <a href="https://www.securecoding.cert.org/confluence/display/c/FIO01-C.+Be+careful+using+functions+that+use+file+names+for+identification">
+    FIO01-C. Be careful using functions that use file names for identification
+  </a>.
+</li>
+<li>
+NodeJS: 
+  <a href="https://nodejs.org/api/fs.html">The FS module</a>.
+</li>
+</references>
+</qhelp>

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

@@ -0,0 +1,105 @@
+/**
+ * @name Potential file system race condition
+ * @description Separately checking the state of a file before operating
+ *              on it may allow an attacker to modify the file between
+ *              the two operations.
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity 7.7
+ * @precision medium
+ * @id js/file-system-race
+ * @tags security
+ *       external/cwe/cwe-367
+ */
+
+import javascript
+
+/**
+ * A call that checks a property of some file.
+ */
+class FileCheck extends DataFlow::CallNode {
+  FileCheck() {
+    this =
+      NodeJSLib::FS::moduleMember([
+          "open", "openSync", "exists", "existsSync", "stat", "statSync", "lstat", "lstatSync",
+          "fstat", "fstatSync", "access", "accessSync"
+        ]).getACall()
+  }
+
+  DataFlow::Node getPathArgument() { result = this.getArgument(0) }
+}
+
+/**
+ * A call that modifies or otherwise interacts with a file.
+ */
+class FileUse extends DataFlow::CallNode {
+  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()
+  }
+
+  DataFlow::Node getPathArgument() { result = this.getArgument(0) }
+}
+
+/**
+ * Gets a reference to a file-handle from a call to `open` or `openSync`.
+ */
+DataFlow::SourceNode getAFileHandle(DataFlow::TypeTracker t) {
+  t.start() and
+  (
+    result = NodeJSLib::FS::moduleMember("openSync").getACall()
+    or
+    result =
+      NodeJSLib::FS::moduleMember("open")
+          .getACall()
+          .getLastArgument()
+          .getAFunctionValue()
+          .getParameter(1)
+  )
+  or
+  exists(DataFlow::TypeTracker t2 | result = getAFileHandle(t2).track(t2, t))
+}
+
+/**
+ * Holds if `check` and `use` operate on the same file.
+ */
+predicate checkAndUseOnSame(FileCheck check, FileUse use) {
+  exists(string path |
+    check.getPathArgument().mayHaveStringValue(path) and
+    use.getPathArgument().mayHaveStringValue(path)
+  )
+  or
+  AccessPath::getAnAliasedSourceNode(check.getPathArgument()).flowsTo(use.getPathArgument())
+}
+
+/**
+ * Holds if `check` happens before `use`.
+ */
+pragma[inline]
+predicate useAfterCheck(FileCheck check, FileUse use) {
+  check.getCallback(_).getFunction() = use.getContainer()
+  or
+  exists(BasicBlock bb |
+    check.getBasicBlock() = bb and
+    use.getBasicBlock() = bb and
+    exists(int i, int j |
+      bb.getNode(i) = check.asExpr() and
+      bb.getNode(j) = use.asExpr() and
+      i < j
+    )
+  )
+  or
+  check.getBasicBlock().getASuccessor+() = use.getBasicBlock()
+}
+
+from FileCheck check, FileUse use
+where
+  checkAndUseOnSame(check, use) and
+  useAfterCheck(check, use) and
+  not getAFileHandle(DataFlow::TypeTracker::end()).flowsTo(use.getPathArgument())
+select use, "The file may have changed since it $@.", check, "was checked"

javascript/ql/src/Security/CWE-367/examples/file-race-bad.js

@@ -0,0 +1,9 @@
+const fs = require("fs");
+const os = require("os");
+const path = require("path");
+
+const filePath = path.join(os.tmpdir(), "my-temp-file.txt");
+
+if (!fs.existsSync(filePath)) {
+  fs.writeFileSync(filePath, "Hello", { mode: 0o600 });
+}

javascript/ql/src/Security/CWE-367/examples/file-race-good.js

@@ -0,0 +1,13 @@
+const fs = require("fs");
+const os = require("os");
+const path = require("path");
+
+const filePath = path.join(os.tmpdir(), "my-temp-file.txt");
+
+try {
+  const fd = fs.openSync(filePath, fs.O_CREAT | fs.O_EXCL | fs.O_RDWR, 0o600);
+
+  fs.writeFileSync(fd, "Hello");
+} catch (e) {
+  // file existed
+}

javascript/ql/src/change-notes/2022-01-27-file-system-race.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* A new query `js/file-system-race` has been added. The query detects when there is time between a file being checked and used. The query is not run by default.

javascript/ql/test/query-tests/Security/CWE-367/FileSystemRace.expected

@@ -0,0 +1,4 @@
+| tst.js:8:3:8:54 | fs.writ ... o600 }) | The file may have changed since it $@. | tst.js:7:6:7:28 | fs.exis ... lePath) | was checked |
+| tst.js:14:3:14:40 | fs.writ ... ntent") | The file may have changed since it $@. | tst.js:12:15:12:36 | fs.stat ... ePath2) | was checked |
+| tst.js:18:3:18:40 | fs.writ ... ntent") | The file may have changed since it $@. | tst.js:17:1:19:2 | fs.acce ... T OK\\n}) | was checked |
+| tst.js:33:3:37:4 | fs.open ... ..\\n  }) | The file may have changed since it $@. | tst.js:27:1:38:2 | fs.acce ...  });\\n}) | was checked |

javascript/ql/test/query-tests/Security/CWE-367/FileSystemRace.qlref

@@ -0,0 +1 @@
+Security/CWE-367/FileSystemRace.ql

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

@@ -0,0 +1,38 @@
+const fs = require("fs");
+const os = require("os");
+const path = require("path");
+
+const filePath = path.join(os.tmpdir(), "my-temp-file.txt");
+
+if (!fs.existsSync(filePath)) {
+  fs.writeFileSync(filePath, "Hello", { mode: 0o600 }); // NOT OK
+}
+
+const filePath2 = createFile();
+const stats = fs.statSync(filePath2);
+if (doSomethingWith(stats)) {
+  fs.writeFileSync(filePath2, "content"); // NOT OK
+}
+
+fs.access(filePath2, fs.constants.F_OK, (err) => {
+  fs.writeFileSync(filePath2, "content"); // NOT OK
+});
+
+fs.open("myFile", "rw", (err, fd) => {
+  fs.writeFileSync(fd, "content"); // OK
+});
+
+import { open, close } from "fs";
+
+fs.access("myfile", (err) => {
+  if (!err) {
+    console.error("myfile already exists");
+    return;
+  }
+
+  fs.open("myfile", "wx", (err, fd) => { // NOT OK
+    if (err) throw err;
+
+    // ....
+  });
+});