JavaScript: add support for NodeJS and Electron http client libraries #14

rdmarsh2 · 2018-08-03T18:19:28Z

This PR adds RemoteFlowSources for both the NodeJS http/https client library and the Electron http/https client library and adds CredentialExprs for auth responses. The majority of the work is in NodeJSLib.qll; the Electron QL library simply extends the ClientRequest class from the QL NodeJS library.

JavaScript: Further improve handling of destructuring patterns.

xiemaisi

A few suggestions and comments.

This adds a lot of new classes. Have you considered which of them could be made private?

I also dislike the amount of repetition. Is there no way of merging some of these definitions or pulling out shared bits of code?

Finally, as I commented on below, you seem to use TrackedNodes (inter-procedural flow) a lot. We have so far got along with SourceNodes (intra-procedural flow). I assume this is because you have seen examples where inter-procedural flow is needed?

xiemaisi · 2018-08-06T10:34:06Z

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

+  /**
+   * A data flow node that is a Node.js HTTP or HTTPS client request.
+   */
+  abstract class ClientRequest extends DataFlow::TrackedNode {


This seems to overlap with the existing class RequestExpr. What's the rationale for introducing this class, and why is it a TrackedNode? Have you seen cases where we need to track requests inter-procedurally?

RequestExpr is used in a NodeJS server handling an incoming request, while ClientRequest is when the NodeJS or Electron libraries are used to send a request, as in the call at line 18 of javascript/ql/test/library-tests/frameworks/NodeJSLib/src/http.js

I think I added that because I saw a case where we need to track them interprocedurally - I'll go back through the projects I was testing on and see if I actually needed it.

It looks like I added it in an attempt to handle a use of bind to create the request callback. It didn't work, so I'll remove it and see if there's a better way to handle bind.

OK, that's not clear from the description or the name. Please consider whether there is a better name, and extend the qldoc with an example.

bind is tricky; I'd say let's ignore it for now.

xiemaisi · 2018-08-06T10:34:26Z

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

+   * A data flow node that is a Node.js HTTP or HTTPS client request.
+   */
+  abstract class ClientRequest extends DataFlow::TrackedNode {
+    abstract DataFlow::Node getOptions();


Public predicate, so needs qldoc.

xiemaisi · 2018-08-06T10:35:24Z

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

+  /**
+   * A data flow node that is  the parameter of a result callback for an HTTP or HTTPS request made through Node.js.
+   */
+  abstract class RequestCallbackParam extends DataFlow::TrackedNode, RemoteFlowSource {


This overlaps with RequestSource; same questions as for ClientRequest above.

xiemaisi · 2018-08-06T10:36:49Z

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

+    RequestCallbackData() {
+      exists(RequestCallbackParam rcp, DataFlow::MethodCallNode mcn, DataFlow::FunctionNode fn |
+        rcp.flowsTo(mcn.getReceiver()) and
+        mcn.getArgument(0).mayHaveStringValue("data") and


mcn.getMethodName() = "on"?

xiemaisi · 2018-08-06T10:38:17Z

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

+  }
+
+  /**
+   * A data flow node that is a data callback for an HTTP or HTTPS request made through Node.js.


Here and elsewhere, could you please add a brief example code snippet to the qldoc to clarify what is being modelled?

xiemaisi · 2018-08-06T10:40:05Z

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

+      exists(DataFlow::MethodCallNode mcn |
+        mcn = this and
+        DataFlow::moduleMember("electron", "net").flowsTo(mcn.getReceiver()) and
+        mcn.getMethodName() = "request"


Does this = DataFlow::moduleMember("electron", "net").getAMemberCall("request") not do what you want?

xiemaisi · 2018-08-06T10:40:29Z

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

+    NewClientRequest() {
+      exists(DataFlow::NewNode nn |
+        this = nn and
+        DataFlow::moduleMember("electron", "ClientRequest").flowsTo(nn.getCallee())


this = DataFlow::moduleMember("electron", "ClientRequest").getAnInstantiation()

xiemaisi

Thanks for addressing the review comments! I've got a few more comments, some of them stylistic, others about opportunities for using the SourceNode fluent API to simplify things. I didn't point out all opportunities for doing this.

Overall, I'd encourage you to make as many of these classes private as possible. If you have two very similar private classes that only exist to extend at abstract class, consider merging them for simplicity.

xiemaisi · 2018-08-10T07:37:51Z

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

+
+
+  /**
+   * A Node.js-style HTTP or HTTPS request made using `Electron.client`, e.g. `new client(url)`.


You use electron.net above (lower-case electron), and Electron.client here (upper-case Electron). Is that intentional?

Here and below: we generally avoid "e.g." in favour of "for example" or similar.

xiemaisi · 2018-08-10T07:39:51Z

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

@@ -506,4 +506,270 @@ module NodeJSLib {
    }
  }

+  /**
+   * A data flow node that is an HTTP or HTTPS client request made by a Node server, e.g. `http.request(url)`


We generally use "Node.js" instead of "Node" (although we're not as consistent about it as I'd like). Also, missing a full stop at the end.

xiemaisi · 2018-08-10T07:45:58Z

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

+  /**
+   * A data flow node that is an HTTPS client request made by a Node process, e.g. `https.request(url)`.
+   */
+  class HttpsRequest extends ClientRequest {


Do we have a good use case for separating this class from HttpsRequest and exposing both of them as public API? If not, I'd prefer merging them and making them private.

xiemaisi · 2018-08-10T07:47:27Z

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

+  /**
+   * A data flow node that is the parameter of a result callback for an HTTPS request made by a Node process, e.g. `res` in `https.request(url, (res) => {})`.
+   */
+  class HttpsRequestCallbackParam extends RequestCallbackParam {


Same question as for HttpsRequest above.

xiemaisi · 2018-08-10T07:49:09Z

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

+  class RequestCallbackData extends RemoteFlowSource {
+    RequestCallbackData() {
+      exists(RequestCallbackParam rcp, DataFlow::MethodCallNode mcn, DataFlow::FunctionNode fn |
+        rcp.flowsTo(mcn.getReceiver()) and


mcn = rcp.getAMethodCall("on")

xiemaisi · 2018-08-10T07:53:12Z

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

+   */
+  class ClientRequestDataHandler extends DataFlow::FunctionNode {
+    ClientRequestDataHandler() {
+      exists(DataFlow::MethodCallNode mcn, ClientRequestResponseEvent cr |


Similar simplifications to above can be made here.

xiemaisi · 2018-08-10T07:55:49Z

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

+    ClientRequestLoginPassword() {
+      exists(ClientRequestLoginCallback callback, DataFlow::CallNode call |
+        callback.flowsTo(call.getCallee()) and
+        this = call.getArgument(1).asExpr()


this = callback.getACall().getArgument(1).asExpr()

xiemaisi · 2018-08-10T07:58:55Z

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

+  /**
+   * A data flow node that is an HTTP or HTTPS client request made by a Node server, e.g. `http.request(url)`
+   */
+  abstract class ClientRequest extends DataFlow::SourceNode {


It's generally safer to extend DataFlow::DefaultSourceNode, which has (almost) the same extent as SourceNode, but isn't abstract.

xiemaisi · 2018-08-10T07:59:14Z

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

+  /**
+   * A data flow node that is the parameter of a result callback for an HTTP or HTTPS request made by a Node process, e.g. `res` in `http.request(url, (res) => {})`.
+   */
+  abstract class RequestCallbackParam extends DataFlow::SourceNode, RemoteFlowSource {


As above, extend DefaultSourceNode instead.

xiemaisi · 2018-08-10T07:59:24Z

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

+  /**
+   * A data flow node that is the parameter of a response callback for an HTTP or HTTPS request made by a Node process, e.g. `res` in `http.request(url).on('response', (res) => {})`.
+   */
+  class ClientRequestResponseEvent extends RemoteFlowSource, DataFlow::SourceNode {


DefaultSourceNode

xiemaisi

Looking good; a couple more suggestions.

xiemaisi · 2018-08-13T09:53:40Z

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

@@ -33,4 +33,53 @@ module Electron {
      this = DataFlow::moduleMember("electron", "BrowserView").getAnInstantiation()
    }
  }
+    /**


Indentation; also, please add blank line above.

This still needs addressing.

xiemaisi · 2018-08-13T09:54:39Z

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

+
+
+  /**
+   * A data flow node that is the parameter of a redirect callback for an HTTP or HTTPS request made by a Node process, for example `res` in `net.request(url).on('redirect', (res) => {})`.


s/Node/Node.js/ for consistency.

xiemaisi · 2018-08-13T09:57:46Z

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

+  }
+
+  /**
+   * A data flow node that is an HTTPS client request made by a Node.js process, for example `https.get(url)`.


This has exactly the same qldoc as the previous class, apart from the code example; either merge (my preferred option) or refine.

It's also wrong, of course, in that the qldoc talks about HTTPS, while the implementation is about HTTP.

s/HTTPS/HTTP or HTTPS/

xiemaisi · 2018-08-13T09:59:34Z

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

+      exists(DataFlow::MethodCallNode mcn, DataFlow::FunctionNode fn |
+        mcn = req and
+        fn.flowsTo(mcn.getArgument(1)) and
+        this = fn.getParameter(0)


this = mcn.getCallback(1).getParameter(0)

xiemaisi · 2018-08-13T10:00:07Z

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

+        rcp.getAMethodCall("on") = mcn and
+        mcn.getArgument(0).mayHaveStringValue("data") and
+        fn.flowsTo(mcn.getArgument(1)) and
+        fn.getParameter(0) = this


this = mcn.getCallback(1).getParameter(0)

xiemaisi · 2018-08-13T10:06:37Z

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

+  /**
+   * A data flow node that is the parameter of a response callback for an HTTP or HTTPS request made by a Node.js process, for example `res` in `http.request(url).on('response', (res) => {})`.
+   */
+  class ClientRequestResponseEvent extends RemoteFlowSource, DataFlow::ParameterNode {


Could this class be private?

Could it be?

xiemaisi

A few more comments about inconsistent qldoc. Also, could you add the eslint-scope attack as a test (with appropriate attribution)? With the latest extractor changes, this should now work out of the box.

xiemaisi · 2018-08-14T08:45:52Z

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

+  }
+
+  /**
+   * A data flow node that is an HTTP client request made by a Node.js server, for example `http.request(url)`.


s/HTTP/HTTP or HTTPs/

xiemaisi · 2018-08-14T08:46:15Z

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

+  }
+
+  /**
+   * A data flow node that is an HTTPS client request made by a Node.js process, for example `https.get(url)`.


s/HTTPS/HTTP or HTTPS/

xiemaisi · 2018-08-14T08:46:49Z

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

+   */
+  private class ClientRequestCallbackData extends RemoteFlowSource {
+    ClientRequestCallbackData() {
+      exists(ClientRequestCallbackParam rcp, DataFlow::MethodCallNode mcn|


Missing space after mcn.

Still missing.

xiemaisi · 2018-08-14T08:49:43Z

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

+  }
+
+  /**
+   * A data flow node that is the parameter of a result callback for an HTTPS request made by a Node.js process, for example `res` in `https.request(url, (res) => {})`.


s/HTTPS/HTTP or HTTPS/

xiemaisi · 2018-08-14T08:51:06Z

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

+        cr.getAMethodCall("on") = mcn and
+        mcn.getArgument(0).mayHaveStringValue("data") and
+        handler.flowsTo(mcn.getArgument(1)) and
+        this = handler.getParameter(0)


this = mcn.getCallback(1).getParameter(0)

rdmarsh2 · 2018-08-14T17:15:48Z

I'd prefer not to check in the unmodified attack to avoid antivirus trouble on Windows. Would a modified version with the URL changed be OK?

xiemaisi · 2018-08-14T17:31:59Z

Sure, that makes sense. Hadn't thought of the antivirus angle.

xiemaisi

Almost there! A few more cosmetic things, then we should be good to go.

Could you squash everything down to one (or a handful) of commits, please?

xiemaisi · 2018-08-15T07:49:49Z

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

@@ -33,4 +33,53 @@ module Electron {
      this = DataFlow::moduleMember("electron", "BrowserView").getAnInstantiation()
    }
  }
+    /**


This still needs addressing.

xiemaisi · 2018-08-15T08:11:57Z

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

+   */
+  private class ClientRequestCallbackData extends RemoteFlowSource {
+    ClientRequestCallbackData() {
+      exists(ClientRequestCallbackParam rcp, DataFlow::MethodCallNode mcn|


Still missing.

xiemaisi · 2018-08-15T08:12:21Z

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

+  /**
+   * A data flow node that is the parameter of a response callback for an HTTP or HTTPS request made by a Node.js process, for example `res` in `http.request(url).on('response', (res) => {})`.
+   */
+  class ClientRequestResponseEvent extends RemoteFlowSource, DataFlow::ParameterNode {


Could it be?

xiemaisi · 2018-08-15T08:12:48Z

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

+        cr.getAMethodCall("on") = mcn and
+        mcn.getArgument(0).mayHaveStringValue("data") and
+        this = mcn.getCallback(1).getParameter(0)
+


Spurious blank line.

xiemaisi · 2018-08-17T11:49:08Z

Ping @rdmarsh2, could you address the few remaining superficial issues? This is basically good to go, I think.

rdmarsh2 · 2018-08-17T17:18:30Z

Addressed the last few comments and squashed down to a few commits

Extractor: fix child index values

Kotlin: Refactoring: Give diagnostic messages locations and severities

Java sqli demo

Resolve predicate expressions

rdmarsh2 added the JS label Aug 3, 2018

rdmarsh2 requested a review from a team August 3, 2018 18:20

nickrolfe pushed a commit to nickrolfe/codeql that referenced this pull request Aug 6, 2018

Merge pull request github#14 from max/js/more-destructuring-flow

eb4d8f7

JavaScript: Further improve handling of destructuring patterns.

xiemaisi suggested changes Aug 6, 2018

View reviewed changes

xiemaisi suggested changes Aug 10, 2018

View reviewed changes

rdmarsh2 force-pushed the rdmarsh/js/electron-http-client branch from 19098f8 to d966e8d Compare August 10, 2018 21:02

xiemaisi suggested changes Aug 13, 2018

View reviewed changes

xiemaisi suggested changes Aug 14, 2018

View reviewed changes

xiemaisi suggested changes Aug 15, 2018

View reviewed changes

rdmarsh2 added 3 commits August 17, 2018 10:16

JavaScript: add support for Electron http client

4da9d6d

JavaScript: add change note

4698d13

JavaScript: add the ESLint attack as a test

aaeda5d

rdmarsh2 force-pushed the rdmarsh/js/electron-http-client branch from 7fe5620 to aaeda5d Compare August 17, 2018 17:17

xiemaisi approved these changes Aug 20, 2018

View reviewed changes

semmle-qlci merged commit 44e4b25 into github:master Aug 20, 2018

rdmarsh2 mentioned this pull request Aug 22, 2018

javascript: CodeInjection source via require("http").get({json to remote server}) #88

Closed

toufik-airane mentioned this pull request Oct 23, 2020

JS: Add ElectronShellOpenExternalSink class for Electron framework security #4546

Merged

snyk-bot mentioned this pull request Apr 21, 2021

[Snyk] Upgrade c from 1.0.0 to 1.1.1 majacQ/codeql#5

Merged

snyk-bot mentioned this pull request Jun 23, 2021

[Snyk] Upgrade c from 1.0.0 to 1.1.1 aliscco/codeql#1

Open

aibaars pushed a commit that referenced this pull request Oct 14, 2021

Merge pull request #14 from github/aibaars/remove-storage-index

24b4586

Extractor: fix child index values

smowton pushed a commit to smowton/codeql that referenced this pull request Oct 28, 2021

Merge pull request github#14 from github/igfoo/refactor

cd331bc

Kotlin: Refactoring: Give diagnostic messages locations and severities

hohn added a commit to hohn/codeql that referenced this pull request Dec 13, 2021

Merge pull request github#14 from github/java-sqli-demo

cba892b

Java sqli demo

erik-krogh referenced this pull request in erik-krogh/ql Dec 15, 2021

Merge pull request #14 from github/predicate-expr-resolution

967553d

Resolve predicate expressions

erik-krogh referenced this pull request in erik-krogh/ql Dec 15, 2021

QL: Merge pull request #14 from github/predicate-expr-resolution

4f410f5

Resolve predicate expressions



		/**
		* A Node.js-style HTTP or HTTPS request made using `Electron.client`, e.g. `new client(url)`.



		/**
		* A data flow node that is the parameter of a redirect callback for an HTTP or HTTPS request made by a Node process, for example `res` in `net.request(url).on('redirect', (res) => {})`.

JavaScript: add support for NodeJS and Electron http client libraries #14

JavaScript: add support for NodeJS and Electron http client libraries #14

Uh oh!

Conversation

rdmarsh2 commented Aug 3, 2018

Uh oh!

xiemaisi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiemaisi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiemaisi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiemaisi left a comment

Choose a reason for hiding this comment

Uh oh!