JS: remove speculative property access sink from js/server-crash

Author: esbena

javascript/ql/src/Security/CWE-730/ServerCrash.ql

@@ -36,15 +36,6 @@ predicate isHeaderValue(HTTP::ExplicitHeaderDefinition def, DataFlow::Node node)
   def.definesExplicitly(_, node.asExpr())
 }
 
-predicate isDangerousPropertyRead(DataFlow::PropRead read) {
-  // TODO use flow labels
-  exists(DataFlow::PropRead base |
-    base = read.getBase() and
-    not read instanceof RemoteFlowSource and
-    not exists(read.getACall())
-  )
-}
-
 class Configuration extends TaintTracking::Configuration {
   Configuration() { this = "Configuration" }
 
@@ -53,18 +44,11 @@ class Configuration extends TaintTracking::Configuration {
   override predicate isSink(DataFlow::Node node) {
     // using control characters in a header value will cause an exception
     isHeaderValue(_, node)
-    or
-    // accessing a property of undefined will cause an exception
-    isDangerousPropertyRead(node)
   }
 }
 
 predicate isLikelyToThrow(DataFlow::Node crash) {
-  exists(Configuration cfg, DataFlow::Node sink | cfg.hasFlow(_, sink) |
-    isHeaderValue(crash, sink)
-    or
-    isDangerousPropertyRead(sink) and sink = crash
-  )
+  exists(Configuration cfg, DataFlow::Node sink | cfg.hasFlow(_, sink) | isHeaderValue(crash, sink))
 }
 
 /**

javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.expected

@@ -4,4 +4,3 @@
 | server-crash.js:28:5:28:14 | throw err; | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:27:28:29:3 | (err, x ...  OK\\n  } | an asynchronous function | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
 | server-crash.js:33:5:33:14 | throw err; | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:32:28:34:3 | (err, x ...  OK\\n  } | an asynchronous function | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
 | server-crash.js:41:5:41:48 | res.set ... header) | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:40:28:42:3 | (err, x ...  OK\\n  } | an asynchronous function | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
-| server-crash.js:68:5:68:21 | req.query.foo.bar | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:67:28:69:3 | (err, x ...  OK\\n  } | an asynchronous function | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |

javascript/ql/test/query-tests/Security/CWE-730/server-crash.js

@@ -65,7 +65,7 @@ app.get("/async-throw", (req, res) => {
   });
 
   fs.readFile("/WHATEVER", (err, x) => {
-    req.query.foo.bar; // NOT OK
+    req.query.foo.bar; // NOT OK [INCONSISTENCY]: need to add property reads as sinks
   });
   fs.readFile("/WHATEVER", (err, x) => {
     res.setHeader("reflected", unknown); // OK