improve and fix bugs and add Form Flow Sources test files

Author: am0o0

javascript/ql/lib/semmle/javascript/frameworks/FormParsers.qll

@@ -20,7 +20,7 @@ module BusBoy {
         // Files
         busboyOnEvent.getParameter(0).asSink().mayHaveStringValue("file") and
         // second param of 'file' event is a Readable stream
-        this = readableStreamDataNode(busboyOnEvent.getParameter(1).getParameter(1)).asSource()
+        this = readableStreamDataNode(busboyOnEvent.getParameter(1).getParameter(1))
         or
         // Fields
         busboyOnEvent.getParameter(0).asSink().mayHaveStringValue(["file", "field"]) and
@@ -53,6 +53,15 @@ module BusBoy {
   }
 }
 
+predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+  exists(API::Node busboyOnEvent |
+    busboyOnEvent = API::moduleImport("busboy").getReturn().getMember("on")
+  |
+    busboyOnEvent.getParameter(0).asSink().mayHaveStringValue("file") and
+    customStreamPipeAdditionalTaintStep(busboyOnEvent.getParameter(1).getParameter(1), pred, succ)
+  )
+}
+
 /**
  * A module for modeling [formidable](https://www.npmjs.com/package/formidable) package
  */
@@ -112,7 +121,7 @@ module Multiparty {
             this = on.getParameter(1).getParameter([0, 1]).asSource()
             or
             on.getParameter(0).asSink().mayHaveStringValue("part") and
-            this = readableStreamDataNode(on.getParameter(1).getParameter(0)).asSource()
+            this = readableStreamDataNode(on.getParameter(1).getParameter(0))
           )
         )
       )
@@ -150,7 +159,7 @@ module Dicer {
       exists(API::Node dicer | dicer = API::moduleImport("dicer").getInstance() |
         exists(API::Node on | on = dicer.getMember("on") |
           on.getParameter(0).asSink().mayHaveStringValue("part") and
-          this = readableStreamDataNode(on.getParameter(1).getParameter(0)).asSource()
+          this = readableStreamDataNode(on.getParameter(1).getParameter(0))
           or
           exists(API::Node onPart | onPart = on.getParameter(1).getParameter(0).getMember("on") |
             onPart.getParameter(0).asSink().mayHaveStringValue("header") and
@@ -177,3 +186,14 @@ module Dicer {
     }
   }
 }
+
+/**
+ * An Additional taint step like `for (succ in pred)`
+ */
+private class AdditionalTaintStepForIn extends TaintTracking::SharedTaintStep {
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(ForInStmt fis, Variable v | v = fis.getAnIterationVariable() |
+      succ.asExpr() = v.getAnAccess() and pred.asExpr() = fis.getIterationDomain()
+    )
+  }
+}

javascript/ql/lib/semmle/javascript/frameworks/ReadableStream.qll

@@ -117,18 +117,20 @@ API::Node nodeJsStream() {
  * Gets a Readable Stream method(not a return value of the method)
  * and returns all nodes responsible for a data read access
  */
-API::Node readableStreamDataNode(API::Node stream) {
+DataFlow::Node readableStreamDataNode(API::Node stream) {
+  result = stream.asSource()
+  or
   // 'data' event
   exists(API::CallNode onEvent | onEvent = stream.getMember("on").getACall() |
-    result = onEvent.getParameter(1).getParameter(0) and
+    result = onEvent.getParameter(1).getParameter(0).asSource() and
     onEvent.getParameter(0).asSink().mayHaveStringValue("data")
   )
   or
   // 'Readable' event
   exists(API::CallNode onEvent | onEvent = stream.getMember("on").getACall() |
     (
-      result = onEvent.getParameter(1).getReceiver().getMember("read").getReturn() or
-      result = stream.getMember("read").getReturn()
+      result = onEvent.getParameter(1).getReceiver().getMember("read").getReturn().asSource() or
+      result = stream.getMember("read").getReturn().asSource()
     ) and
     onEvent.getParameter(0).asSink().mayHaveStringValue("readable")
   )

javascript/ql/test/library-tests/frameworks/FormParsers/RemoteFlowSource.expected

@@ -0,0 +1,33 @@
+/**
+ * @name Remote Form Flow Sources
+ * @description Using remote user controlled sources from Forms
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 5
+ * @precision high
+ * @id js/remote-flow-source
+ * @tags correctness
+ *       security
+ */
+
+import javascript
+import DataFlow::PathGraph
+
+/**
+ * A taint-tracking configuration for test
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "RemoteFlowSourcesOUserForm" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink = API::moduleImport("sink").getAParameter().asSink() or
+    sink = API::moduleImport("sink").getReturn().asSource()
+  }
+}
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "This entity depends on a $@.", source.getNode(),
+  "user-provided value"

javascript/ql/test/library-tests/frameworks/FormParsers/RemoteFlowSource.ql

@@ -0,0 +1,33 @@
+const http = require('http');
+const zlib = require('node:zlib');
+const busboy = require('busboy');
+const sink = require('sink');
+
+http.createServer((req, res) => {
+    if (req.method === 'POST') {
+        const bb = busboy({ headers: req.headers });
+        bb.on('file', (name, file, info) => {
+            const { filename, encoding, mimeType } = info;
+            const z = zlib.createGzip();
+            sink(filename, encoding, mimeType) // sink
+            file.pipe(z).pipe(sink())
+
+            file.on('data', (data) => {
+                sink(data)
+            })
+
+            file.on('readable', function () {
+                // There is some data to read now.
+                let data;
+                while ((data = this.read()) !== null) {
+                    sink(data)
+                }
+            });
+        });
+        bb.on('field', (name, val, info) => {
+            sink(name, val, info)
+        });
+    }
+}).listen(8000, () => {
+    console.log('Listening for requests');
+});

javascript/ql/test/library-tests/frameworks/FormParsers/busybus.js

@@ -0,0 +1,25 @@
+const { inspect } = require('util');
+const http = require('http');
+const Dicer = require('dicer');
+const sink = require('sink');
+
+const PORT = 8080;
+
+http.createServer((req, res) => {
+    let m;
+    const dicer = new Dicer({ boundary: m[1] || m[2] });
+
+    dicer.on('part', (part) => {
+        part.pipe(sink())
+        part.on('header', (header) => {
+            for (h in header) {
+                sink(h)
+            }
+        });
+        part.on('data', (data) => {
+            sink(data)
+        });
+    });
+}).listen(PORT, () => {
+    console.log(`Listening for requests on port ${PORT}`);
+});

javascript/ql/test/library-tests/frameworks/FormParsers/dicer.js

@@ -0,0 +1,22 @@
+import http from 'node:http';
+import formidable from 'formidable';
+const sink = require('sink');
+
+const server = http.createServer(async (req, res) => {
+    const form = formidable({});
+    const [fields, files] = await form.parse(req);
+    sink(fields, files)
+    form.on('fileBegin', (formname, file) => {
+        sink(formname, file)
+    });
+    form.on('file', (formname, file) => {
+        sink(formname, file)
+    });
+    form.on('field', (fieldName, fieldValue) => {
+        sink(fieldName, fieldValue)
+    });
+});
+
+server.listen(8080, () => {
+    console.log('Server listening on http://localhost:8080/ ...');
+});

javascript/ql/test/library-tests/frameworks/FormParsers/formidable.js

@@ -0,0 +1,19 @@
+var multiparty = require('multiparty');
+var http = require('http');
+var util = require('util');
+const sink = require('sink');
+
+http.createServer(function (req, res) {
+    var form = new multiparty.Form();
+    form.on('part', (part) => {
+        sink(part)
+        part.pipe(sink())
+    });
+
+    var form2 = new multiparty.Form();
+    form2.parse(req, function (err, fields, files) {
+        sink(fields, files)
+    });
+    form2.parse(req);
+
+}).listen(8080);