Merge pull request github#5529 from erik-krogh/socketInput

Approved by esbena

Author: codeql-ci

javascript/ql/src/semmle/javascript/frameworks/WebSocket.qll

@@ -200,27 +200,73 @@ module ServerWebSocket {
     result = DataFlow::moduleImport("sockjs").getAMemberCall("createServer")
   }
 
+  /**
+   * Gets a `socket.on("connection", (msg, req) => {})` call.
+   */
+  private DataFlow::CallNode getAConnectionCall(LibraryName library) {
+    result = getAServer(library).getAMemberCall(EventEmitter::on()) and
+    result.getArgument(0).mayHaveStringValue("connection")
+  }
+
   /**
    * A server WebSocket instance.
    */
   class ServerSocket extends EventEmitter::Range, DataFlow::SourceNode {
     LibraryName library;
 
-    ServerSocket() {
-      exists(DataFlow::CallNode onCall |
-        onCall = getAServer(library).getAMemberCall(EventEmitter::on()) and
-        onCall.getArgument(0).mayHaveStringValue("connection")
-      |
-        this = onCall.getCallback(1).getParameter(0)
-      )
-    }
+    ServerSocket() { this = getAConnectionCall(library).getCallback(1).getParameter(0) }
 
     /**
      * Gets the name of the library that created this server socket.
      */
     LibraryName getLibrary() { result = library }
   }
 
+  /**
+   * A `socket.on("connection", (msg, req) => {})` call seen as a HTTP route handler.
+   * `req` is a `HTTP::IncomingMessage` instance.
+   */
+  class ConnectionCallAsRouteHandler extends HTTP::RouteHandler, DataFlow::CallNode {
+    ConnectionCallAsRouteHandler() { this = getAConnectionCall(_) }
+
+    override HTTP::HeaderDefinition getAResponseHeader(string name) { none() }
+  }
+
+  /**
+   * The `req` parameter of a `socket.on("connection", (msg, req) => {})` call.
+   */
+  class ServerHTTPRequest extends HTTP::Servers::RequestSource {
+    ConnectionCallAsRouteHandler handler;
+
+    ServerHTTPRequest() { this = handler.getCallback(1).getParameter(1) }
+
+    override HTTP::RouteHandler getRouteHandler() { result = handler }
+  }
+
+  /**
+   * An access user-controlled HTTP request input in a request to a WebSocket server.
+   */
+  class WebSocketRequestInput extends HTTP::RequestInputAccess {
+    ServerHTTPRequest request;
+    string kind;
+
+    WebSocketRequestInput() {
+      kind = "url" and
+      this = request.ref().getAPropertyRead("url")
+      or
+      kind = "header" and
+      this = request.ref().getAPropertyRead(["headers", "rawHeaders"]).getAPropertyRead()
+      or
+      // req.headers.cookie
+      kind = "cookie" and
+      this = request.ref().getAPropertyRead("headers").getAPropertyRead("cookie")
+    }
+
+    override string getKind() { result = kind }
+
+    override HTTP::RouteHandler getRouteHandler() { result = request.getRouteHandler() }
+  }
+
   /**
    * A message sent from a WebSocket server.
    */

javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected

@@ -77,6 +77,11 @@ nodes
 | tst.js:98:29:98:35 | req.url |
 | tst.js:100:19:100:25 | tainted |
 | tst.js:100:19:100:25 | tainted |
+| tst.js:108:11:108:27 | url |
+| tst.js:108:17:108:27 | request.url |
+| tst.js:108:17:108:27 | request.url |
+| tst.js:109:27:109:29 | url |
+| tst.js:109:27:109:29 | url |
 edges
 | tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
 | tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
@@ -152,6 +157,10 @@ edges
 | tst.js:98:19:98:52 | url.par ... ery.url | tst.js:98:9:98:52 | tainted |
 | tst.js:98:29:98:35 | req.url | tst.js:98:19:98:42 | url.par ... , true) |
 | tst.js:98:29:98:35 | req.url | tst.js:98:19:98:42 | url.par ... , true) |
+| tst.js:108:11:108:27 | url | tst.js:109:27:109:29 | url |
+| tst.js:108:11:108:27 | url | tst.js:109:27:109:29 | url |
+| tst.js:108:17:108:27 | request.url | tst.js:108:11:108:27 | url |
+| tst.js:108:17:108:27 | request.url | tst.js:108:11:108:27 | url |
 #select
 | tst.js:18:5:18:20 | request(tainted) | tst.js:14:29:14:35 | req.url | tst.js:18:13:18:19 | tainted | The $@ of this request depends on $@. | tst.js:18:13:18:19 | tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value |
 | tst.js:20:5:20:24 | request.get(tainted) | tst.js:14:29:14:35 | req.url | tst.js:20:17:20:23 | tainted | The $@ of this request depends on $@. | tst.js:20:17:20:23 | tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value |
@@ -173,3 +182,4 @@ edges
 | tst.js:90:5:90:33 | JSDOM.f ... ms.foo) | tst.js:90:19:90:28 | ctx.params | tst.js:90:19:90:32 | ctx.params.foo | The $@ of this request depends on $@. | tst.js:90:19:90:32 | ctx.params.foo | URL | tst.js:90:19:90:28 | ctx.params | a user-provided value |
 | tst.js:92:5:92:33 | JSDOM.f ... ms.foo) | tst.js:92:19:92:28 | ctx.params | tst.js:92:19:92:32 | ctx.params.foo | The $@ of this request depends on $@. | tst.js:92:19:92:32 | ctx.params.foo | URL | tst.js:92:19:92:28 | ctx.params | a user-provided value |
 | tst.js:100:5:100:26 | new Web ... ainted) | tst.js:98:29:98:35 | req.url | tst.js:100:19:100:25 | tainted | The $@ of this request depends on $@. | tst.js:100:19:100:25 | tainted | URL | tst.js:98:29:98:35 | req.url | a user-provided value |
+| tst.js:109:20:109:30 | new ws(url) | tst.js:108:17:108:27 | request.url | tst.js:109:27:109:29 | url | The $@ of this request depends on $@. | tst.js:109:27:109:29 | url | URL | tst.js:108:17:108:27 | request.url | a user-provided value |

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

@@ -99,3 +99,13 @@ var server = http.createServer(async function(req, res) {
 
     new WebSocket(tainted); // NOT OK
 });
+
+
+import * as ws from 'ws';
+
+new ws.Server({ port: 8080 }).on('connection', function(socket, request) {
+  socket.on('message', function(message) {
+    const url = request.url;
+    const socket = new ws(url);
+  });
+});