Merge pull request github#12026 from erik-krogh/nodePty

JS: add code-injection sink for node-pty

Author: yoff

javascript/ql/lib/change-notes/2023-01-31-node-pty.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Added sinks from the [`node-pty`](https://www.npmjs.com/package/node-pty) library to the `js/code-injection` query.

javascript/ql/lib/semmle/javascript/security/dataflow/CodeInjectionCustomizations.qll

@@ -294,6 +294,27 @@ module CodeInjection {
     }
   }
 
+  /**
+   * An execution of a terminal command via the `node-pty` library, seen as a code injection sink.
+   * Example:
+   * ```JS
+   * var pty = require('node-pty');
+   * var ptyProcess = pty.spawn("bash", [], {...});
+   * ptyProcess.write('ls\r');
+   * ```
+   */
+  class NodePty extends Sink {
+    NodePty() {
+      this =
+        API::moduleImport("node-pty")
+            .getMember("spawn")
+            .getReturn()
+            .getMember("write")
+            .getACall()
+            .getArgument(0)
+    }
+  }
+
   /** A sink for code injection via template injection. */
   abstract private class TemplateSink extends Sink {
     deprecated override string getMessageSuffix() {

javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected

@@ -84,6 +84,11 @@ nodes
 | express.js:26:17:26:35 | req.param("wobble") |
 | express.js:27:34:27:38 | taint |
 | express.js:27:34:27:38 | taint |
+| express.js:34:9:34:35 | taint |
+| express.js:34:17:34:35 | req.param("wobble") |
+| express.js:34:17:34:35 | req.param("wobble") |
+| express.js:43:15:43:19 | taint |
+| express.js:43:15:43:19 | taint |
 | module.js:9:16:9:29 | req.query.code |
 | module.js:9:16:9:29 | req.query.code |
 | module.js:9:16:9:29 | req.query.code |
@@ -216,6 +221,10 @@ edges
 | express.js:26:9:26:35 | taint | express.js:27:34:27:38 | taint |
 | express.js:26:17:26:35 | req.param("wobble") | express.js:26:9:26:35 | taint |
 | express.js:26:17:26:35 | req.param("wobble") | express.js:26:9:26:35 | taint |
+| express.js:34:9:34:35 | taint | express.js:43:15:43:19 | taint |
+| express.js:34:9:34:35 | taint | express.js:43:15:43:19 | taint |
+| express.js:34:17:34:35 | req.param("wobble") | express.js:34:9:34:35 | taint |
+| express.js:34:17:34:35 | req.param("wobble") | express.js:34:9:34:35 | taint |
 | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code |
 | module.js:11:17:11:30 | req.query.code | module.js:11:17:11:30 | req.query.code |
 | react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |
@@ -311,6 +320,7 @@ edges
 | express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") | express.js:19:37:19:70 | req.par ... odule") | This code execution depends on a $@. | express.js:19:37:19:70 | req.par ... odule") | user-provided value |
 | express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") | express.js:21:19:21:48 | req.par ... ntext") | This code execution depends on a $@. | express.js:21:19:21:48 | req.par ... ntext") | user-provided value |
 | express.js:27:34:27:38 | taint | express.js:26:17:26:35 | req.param("wobble") | express.js:27:34:27:38 | taint | This code execution depends on a $@. | express.js:26:17:26:35 | req.param("wobble") | user-provided value |
+| express.js:43:15:43:19 | taint | express.js:34:17:34:35 | req.param("wobble") | express.js:43:15:43:19 | taint | This code execution depends on a $@. | express.js:34:17:34:35 | req.param("wobble") | user-provided value |
 | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code | This code execution depends on a $@. | module.js:9:16:9:29 | req.query.code | user-provided value |
 | module.js:11:17:11:30 | req.query.code | module.js:11:17:11:30 | req.query.code | module.js:11:17:11:30 | req.query.code | This code execution depends on a $@. | module.js:11:17:11:30 | req.query.code | user-provided value |
 | react-native.js:8:32:8:38 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:8:32:8:38 | tainted | This code execution depends on a $@. | react-native.js:7:17:7:33 | req.param("code") | user-provided value |

javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected

@@ -88,6 +88,11 @@ nodes
 | express.js:26:17:26:35 | req.param("wobble") |
 | express.js:27:34:27:38 | taint |
 | express.js:27:34:27:38 | taint |
+| express.js:34:9:34:35 | taint |
+| express.js:34:17:34:35 | req.param("wobble") |
+| express.js:34:17:34:35 | req.param("wobble") |
+| express.js:43:15:43:19 | taint |
+| express.js:43:15:43:19 | taint |
 | module.js:9:16:9:29 | req.query.code |
 | module.js:9:16:9:29 | req.query.code |
 | module.js:9:16:9:29 | req.query.code |
@@ -224,6 +229,10 @@ edges
 | express.js:26:9:26:35 | taint | express.js:27:34:27:38 | taint |
 | express.js:26:17:26:35 | req.param("wobble") | express.js:26:9:26:35 | taint |
 | express.js:26:17:26:35 | req.param("wobble") | express.js:26:9:26:35 | taint |
+| express.js:34:9:34:35 | taint | express.js:43:15:43:19 | taint |
+| express.js:34:9:34:35 | taint | express.js:43:15:43:19 | taint |
+| express.js:34:17:34:35 | req.param("wobble") | express.js:34:9:34:35 | taint |
+| express.js:34:17:34:35 | req.param("wobble") | express.js:34:9:34:35 | taint |
 | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code |
 | module.js:11:17:11:30 | req.query.code | module.js:11:17:11:30 | req.query.code |
 | react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted |

javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/express.js

@@ -28,3 +28,18 @@ app.get('/other/path', function(req, res) {
 
   cp.execFileSync('node', ['-e', `console.log(${JSON.stringify(taint)})`]); // OK
 });
+
+const pty = require('node-pty');
+app.get('/terminal', function(req, res) {
+  const taint = req.param("wobble");
+  const shell = pty.spawn('bash', [], {
+    name: 'xterm-color',
+    cols: 80,
+    rows: 30,
+    cwd: process.env.HOME,
+    env: process.env
+  });
+
+  shell.write(taint); // NOT OK
+});
+