Merge pull request github#5419 from erik-krogh/forgery

Approved by asgerf

Author: codeql-ci

javascript/change-notes/2021-03-17-puppeteer.md

@@ -0,0 +1,4 @@
+lgtm,codescanning
+* URIs used in the puppeteer library are now recognized as sinks for `js/request-forgery`.
+  Affected packages are
+    [puppeteer](https://www.npmjs.com/package/puppeteer)

javascript/ql/src/javascript.qll

@@ -102,6 +102,7 @@ import semmle.javascript.frameworks.Next
 import semmle.javascript.frameworks.NoSQL
 import semmle.javascript.frameworks.PkgCloud
 import semmle.javascript.frameworks.PropertyProjection
+import semmle.javascript.frameworks.Puppeteer
 import semmle.javascript.frameworks.React
 import semmle.javascript.frameworks.ReactNative
 import semmle.javascript.frameworks.Request

javascript/ql/src/semmle/javascript/ApiGraphs.qll

@@ -313,10 +313,7 @@ module API {
   module Node {
     /** Gets a node whose type has the given qualified name. */
     Node ofType(string moduleName, string exportedName) {
-      exists(TypeName tn |
-        tn.hasQualifiedName(moduleName, exportedName) and
-        result = Impl::MkCanonicalNameUse(tn).(Node).getInstance()
-      )
+      result = Impl::MkHasUnderlyingType(moduleName, exportedName).(Node).getInstance()
     }
   }
 
@@ -384,6 +381,8 @@ module API {
         imports(_, m)
         or
         m = any(CanonicalName n | isUsed(n)).getExternalModuleName()
+        or
+        any(TypeAnnotation n).hasQualifiedName(m, _)
       } or
       MkClassInstance(DataFlow::ClassNode cls) { cls = trackDefNode(_) and hasSemantics(cls) } or
       MkAsyncFuncResult(DataFlow::FunctionNode f) {
@@ -392,26 +391,13 @@ module API {
       MkDef(DataFlow::Node nd) { rhs(_, _, nd) } or
       MkUse(DataFlow::Node nd) { use(_, _, nd) } or
       /**
-       * A TypeScript canonical name that is defined somewhere, and that isn't a module root.
-       * (Module roots are represented by `MkModuleExport` nodes instead.)
-       *
-       * For most purposes, you probably want to use the `mkCanonicalNameDef` predicate instead of
-       * this constructor.
-       */
-      MkCanonicalNameDef(CanonicalName n) {
-        not n.isRoot() and
-        isDefined(n)
-      } or
-      /**
-       * A TypeScript canonical name that is used somewhere, and that isn't a module root.
-       * (Module roots are represented by `MkModuleImport` nodes instead.)
-       *
-       * For most purposes, you probably want to use the `mkCanonicalNameUse` predicate instead of
-       * this constructor.
+       * A TypeScript type, identified by name of the type-annotation.
+       * This API node is exclusively used by `API::Node::ofType`.
        */
-      MkCanonicalNameUse(CanonicalName n) {
-        not n.isRoot() and
-        isUsed(n)
+      MkHasUnderlyingType(string moduleName, string exportName) {
+        any(TypeAnnotation n).hasQualifiedName(moduleName, exportName)
+        or
+        any(Type t).hasUnderlyingType(moduleName, exportName)
       } or
       MkSyntheticCallbackArg(DataFlow::Node src, int bound, DataFlow::InvokeNode nd) {
         trackUseNode(src, true, bound).flowsTo(nd.getCalleeNode())
@@ -420,10 +406,9 @@ module API {
     class TDef = MkModuleDef or TNonModuleDef;
 
     class TNonModuleDef =
-      MkModuleExport or MkClassInstance or MkAsyncFuncResult or MkDef or MkCanonicalNameDef or
-          MkSyntheticCallbackArg;
+      MkModuleExport or MkClassInstance or MkAsyncFuncResult or MkDef or MkSyntheticCallbackArg;
 
-    class TUse = MkModuleUse or MkModuleImport or MkUse or MkCanonicalNameUse;
+    class TUse = MkModuleUse or MkModuleImport or MkUse or MkHasUnderlyingType;
 
     private predicate hasSemantics(DataFlow::Node nd) { not nd.getTopLevel().isExterns() }
 
@@ -460,20 +445,6 @@ module API {
       )
     }
 
-    /** An API-graph node representing definitions of the canonical name `cn`. */
-    private TApiNode mkCanonicalNameDef(CanonicalName cn) {
-      if cn.isModuleRoot()
-      then result = MkModuleExport(cn.getExternalModuleName())
-      else result = MkCanonicalNameDef(cn)
-    }
-
-    /** An API-graph node representing uses of the canonical name `cn`. */
-    private TApiNode mkCanonicalNameUse(CanonicalName cn) {
-      if cn.isModuleRoot()
-      then result = MkModuleImport(cn.getExternalModuleName())
-      else result = MkCanonicalNameUse(cn)
-    }
-
     /**
      * Holds if `rhs` is the right-hand side of a definition of a node that should have an
      * incoming edge from `base` labeled `lbl` in the API graph.
@@ -577,11 +548,6 @@ module API {
       exists(string m | nd = MkModuleExport(m) | exports(m, rhs))
       or
       nd = MkDef(rhs)
-      or
-      exists(CanonicalName n | nd = MkCanonicalNameDef(n) |
-        rhs = n.(Namespace).getADefinition().flow() or
-        rhs = n.(CanonicalFunctionName).getADefinition().flow()
-      )
     }
 
     /**
@@ -633,10 +599,10 @@ module API {
           ref = cls.getConstructor().getParameter(i)
         )
         or
-        exists(TypeName tn |
-          base = MkCanonicalNameUse(tn) and
+        exists(string moduleName, string exportName |
+          base = MkHasUnderlyingType(moduleName, exportName) and
           lbl = Label::instance() and
-          ref = getANodeWithType(tn)
+          ref.(DataFlow::SourceNode).hasUnderlyingType(moduleName, exportName)
         )
         or
         exists(DataFlow::InvokeNode call |
@@ -676,8 +642,6 @@ module API {
       )
       or
       nd = MkUse(ref)
-      or
-      exists(CanonicalName n | nd = MkCanonicalNameUse(n) | ref.asExpr() = n.getAnAccess())
     }
 
     /** Holds if module `m` exports `rhs`. */
@@ -832,13 +796,6 @@ module API {
       result = awaited(call, DataFlow::TypeTracker::end())
     }
 
-    private DataFlow::SourceNode getANodeWithType(TypeName tn) {
-      exists(string moduleName, string typeName |
-        tn.hasQualifiedName(moduleName, typeName) and
-        result.hasUnderlyingType(moduleName, typeName)
-      )
-    }
-
     /**
      * Holds if there is an edge from `pred` to `succ` in the API graph that is labeled with `lbl`.
      */
@@ -879,11 +836,10 @@ module API {
         succ = MkClassInstance(trackDefNode(def))
       )
       or
-      exists(CanonicalName cn1, string n, CanonicalName cn2 |
-        pred in [mkCanonicalNameDef(cn1), mkCanonicalNameUse(cn1)] and
-        cn2 = cn1.getChild(n) and
-        lbl = Label::member(n) and
-        succ in [mkCanonicalNameDef(cn2), mkCanonicalNameUse(cn2)]
+      exists(string moduleName, string exportName |
+        pred = MkModuleImport(moduleName) and
+        lbl = Label::member(exportName) and
+        succ = MkHasUnderlyingType(moduleName, exportName)
       )
       or
       exists(DataFlow::Node nd, DataFlow::FunctionNode f |

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

@@ -0,0 +1,95 @@
+/**
+ * Provides classes and predicates for reasoning about [puppeteer](https://www.npmjs.com/package/puppeteer).
+ */
+
+import javascript
+
+/**
+ * Classes and predicates modelling the [puppeteer](https://www.npmjs.com/package/puppeteer) library.
+ */
+module Puppeteer {
+  /**
+   * A reference to a module import of puppeteer.
+   */
+  private API::Node puppeteer() { result = API::moduleImport(["puppeteer", "puppeteer-core"]) }
+
+  /**
+   * A reference to a `Browser` from puppeteer.
+   */
+  private API::Node browser() {
+    result = API::Node::ofType("puppeteer", "Browser")
+    or
+    result = puppeteer().getMember(["launch", "connect"]).getReturn().getPromised()
+    or
+    result = [page(), context(), target()].getMember("browser").getReturn()
+  }
+
+  /**
+   * A reference to a `Page` from puppeteer.
+   */
+  API::Node page() {
+    result = API::Node::ofType("puppeteer", "Page")
+    or
+    result = [browser(), context()].getMember("newPage").getReturn().getPromised()
+    or
+    result = [browser(), context()].getMember("pages").getReturn().getPromised().getUnknownMember()
+    or
+    result = target().getMember("page").getReturn().getPromised()
+  }
+
+  /**
+   * A reference to a `Target` from puppeteer.
+   */
+  private API::Node target() {
+    result = API::Node::ofType("puppeteer", "Target")
+    or
+    result = [page(), browser()].getMember("target").getReturn()
+    or
+    result = context().getMember("targets").getReturn().getUnknownMember()
+    or
+    result = target().getMember("opener").getReturn()
+  }
+
+  /**
+   * A reference to a `BrowserContext` from puppeteer.
+   */
+  private API::Node context() {
+    result = API::Node::ofType("puppeteer", "BrowserContext")
+    or
+    result = [page(), target()].getMember("browserContext").getReturn()
+    or
+    result = browser().getMember("browserContexts").getReturn().getUnknownMember()
+    or
+    result = browser().getMember("createIncognitoBrowserContext").getReturn().getPromised()
+    or
+    result = browser().getMember("defaultBrowserContext").getReturn()
+  }
+
+  /**
+   * A call requesting a `Page` to navigate to some url, seen as a `ClientRequest`.
+   */
+  private class PuppeteerGotoCall extends ClientRequest::Range, API::InvokeNode {
+    PuppeteerGotoCall() { this = page().getMember("goto").getACall() }
+
+    override DataFlow::Node getUrl() { result = getArgument(0) }
+
+    override DataFlow::Node getHost() { none() }
+
+    override DataFlow::Node getADataNode() { none() }
+  }
+
+  /**
+   * A call requesting a `Page` to load a stylesheet or script, seen as a `ClientRequest`.
+   */
+  private class PuppeteerLoadResourceCall extends ClientRequest::Range, API::InvokeNode {
+    PuppeteerLoadResourceCall() {
+      this = page().getMember(["addStyleTag", "addScriptTag"]).getACall()
+    }
+
+    override DataFlow::Node getUrl() { result = getParameter(0).getMember("url").getARhs() }
+
+    override DataFlow::Node getHost() { none() }
+
+    override DataFlow::Node getADataNode() { none() }
+  }
+}

javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

@@ -636,6 +636,20 @@ module TaintedPath {
     SendPathSink() { this = DataFlow::moduleImport("send").getACall().getArgument(1) }
   }
 
+  /**
+   * A path argument given to a `Page` in puppeteer, specifying where a pdf/screenshot should be saved.
+   */
+  private class PuppeteerPath extends TaintedPath::Sink {
+    PuppeteerPath() {
+      this =
+        Puppeteer::page()
+            .getMember(["pdf", "screenshot"])
+            .getParameter(0)
+            .getMember("path")
+            .getARhs()
+    }
+  }
+
   /**
    * Holds if there is a step `src -> dst` mapping `srclabel` to `dstlabel` relevant for path traversal vulnerabilities.
    */

javascript/ql/test/ApiGraphs/typed/NodeOfType.expected

@@ -1,3 +1,4 @@
 | mongodb | Collection | index.ts:14:3:14:17 | getCollection() |
 | mongoose | Model | index.ts:22:3:22:20 | getMongooseModel() |
 | mongoose | Query | index.ts:23:3:23:20 | getMongooseQuery() |
+| puppeteer | Browser | index.ts:30:22:30:33 | this.browser |

javascript/ql/test/ApiGraphs/typed/index.ts

@@ -22,3 +22,11 @@ app.post("/find", (req, res) => {
   getMongooseModel().find({ id: v }); /* def (parameter 0 (member find (instance (member Model (member exports (module mongoose)))))) */
   getMongooseQuery().find({ id: v }); /* def (parameter 0 (member find (instance (member Query (member exports (module mongoose)))))) */
 });
+
+import * as puppeteer from 'puppeteer';
+class Renderer {
+    private browser: puppeteer.Browser;
+    foo(): void {
+        const page = this.browser.newPage(); /* use (instance (member Browser (member exports (module puppeteer)))) */
+    }
+}

javascript/ql/test/library-tests/frameworks/ClientRequests/ClientRequests.expected

@@ -5,6 +5,9 @@ test_ClientRequest
 | apollo.js:17:1:17:34 | new Pre ... yurl"}) |
 | apollo.js:20:1:20:77 | createN ... phql'}) |
 | apollo.js:23:1:23:31 | new Web ... wsUri}) |
+| puppeteer.ts:6:11:6:42 | page.go ... e.com') |
+| puppeteer.ts:8:5:8:61 | page.ad ... css" }) |
+| puppeteer.ts:18:30:18:50 | page.go ... estUrl) |
 | tst.js:11:5:11:16 | request(url) |
 | tst.js:13:5:13:20 | request.get(url) |
 | tst.js:15:5:15:23 | request.delete(url) |
@@ -138,6 +141,9 @@ test_getUrl
 | apollo.js:17:1:17:34 | new Pre ... yurl"}) | apollo.js:17:26:17:32 | "myurl" |
 | apollo.js:20:1:20:77 | createN ... phql'}) | apollo.js:20:30:20:75 | 'https: ... raphql' |
 | apollo.js:23:1:23:31 | new Web ... wsUri}) | apollo.js:23:25:23:29 | wsUri |
+| puppeteer.ts:6:11:6:42 | page.go ... e.com') | puppeteer.ts:6:21:6:41 | 'https: ... le.com' |
+| puppeteer.ts:8:5:8:61 | page.ad ... css" }) | puppeteer.ts:8:29:8:58 | "http:/ ... le.css" |
+| puppeteer.ts:18:30:18:50 | page.go ... estUrl) | puppeteer.ts:18:40:18:49 | requestUrl |
 | tst.js:11:5:11:16 | request(url) | tst.js:11:13:11:15 | url |
 | tst.js:13:5:13:20 | request.get(url) | tst.js:13:17:13:19 | url |
 | tst.js:15:5:15:23 | request.delete(url) | tst.js:15:20:15:22 | url |

javascript/ql/test/library-tests/frameworks/ClientRequests/puppeteer.ts

@@ -0,0 +1,20 @@
+import * as puppeteer from 'puppeteer';
+
+(async () => {
+    const browser = await puppeteer.launch();
+    const page = await browser.newPage();
+    await page.goto('https://example.com');
+
+    page.addStyleTag({ url: "http://example.org/style.css" })
+})();
+
+class Renderer {
+    private browser: puppeteer.Browser;
+    constructor(browser: puppeteer.Browser) {
+        this.browser = browser;
+    }
+    async foo(requestUrl: string): Promise<void> {
+        const page = await this.browser.newPage();
+        let response = await page.goto(requestUrl);
+    }
+}

javascript/ql/test/library-tests/frameworks/ClientRequests/tsconfig.json

@@ -0,0 +1 @@
+{}