Merge rc/1.18 into next.

javascript/ql/src/LanguageFeatures/EmptyArrayInit.ql

@@ -45,4 +45,5 @@ class OmittedArrayElement extends ArrayExpr {
 }
 
 from OmittedArrayElement ae
+where not ae.getFile().getFileType().isTypeScript() // ignore quirks in TypeScript tokenizer
 select ae, "Avoid omitted array elements."

javascript/ql/src/LanguageFeatures/SemicolonInsertion.ql

@@ -36,7 +36,8 @@ where s.hasSemicolonInserted() and
       asi = strictcount(Stmt ss | asi(sc, ss, true)) and
       nstmt = strictcount(Stmt ss | asi(sc, ss, _)) and
       perc = ((1-asi/nstmt)*100).floor() and
-      perc >= 90
+      perc >= 90 and
+      not s.getFile().getFileType().isTypeScript() // ignore some quirks in the TypeScript tokenizer
 select (LastLineOf)s, "Avoid automated semicolon insertion " +
                       "(" + perc + "% of all statements in $@ have an explicit semicolon).",
                       sc, "the enclosing " + sctype

javascript/ql/src/Security/CWE-918/RequestForgery.qhelp

@@ -0,0 +1,79 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+
+	<overview>
+		<p>
+
+			Directly incorporating user input into an HTTP request
+			without validating the input can facilitate different kinds of request
+			forgery attacks, where the attacker essentially controls the request.
+
+			If the vulnerable request is in server-side code, then security
+			mechanisms, such as external firewalls, can be bypassed.
+
+			If the vulnerable request is in client-side code, then unsuspecting
+			users can send malicious requests to other servers, potentially
+			resulting in a DDOS attack.
+
+		</p>
+	</overview>
+
+	<recommendation>
+
+		<p>
+
+			To guard against request forgery, it is advisable to avoid
+			putting user input directly into a remote request. If a flexible
+			remote request mechanism is required, it is recommended to maintain a
+			list of authorized request targets and choose from that list based on
+			the user input provided.
+
+		</p>
+
+	</recommendation>
+
+	<example>
+
+		<p>
+
+			The following example shows an HTTP request parameter
+			being used directly in a URL request without validating the input,
+			which facilitates an SSRF attack. The request
+			<code>http.get(...)</code> is vulnerable since attackers can choose
+			the value of <code>target</code> to be anything they want. For
+			instance, the attacker can choose
+			<code>"internal.example.com/#"</code> as the target, causing the URL
+			used in the request to be
+			<code>"https://internal.example.com/#.example.com/data"</code>.
+
+		</p>
+
+		<p>
+
+			A request to <code>https://internal.example.com</code> may
+			be problematic if that server is not meant to be
+			directly accessible from the attacker's machine.
+
+		</p>
+
+		<sample src="examples/RequestForgeryBad.js"/>
+
+		<p>
+
+			One way to remedy the problem is to use the user input to
+			select a known fixed string before performing the request:
+
+		</p>
+
+        <sample src="examples/RequestForgeryGood.js"/>
+
+	</example>
+
+	<references>
+
+		<li>OWASP: <a href="https://www.owasp.org/index.php/Server_Side_Request_Forgery">SSRF</a></li>
+
+	</references>
+</qhelp>

javascript/ql/src/Security/CWE-918/RequestForgery.ql

@@ -0,0 +1,18 @@
+/**
+ * @name Uncontrolled data used in remote request
+ * @description Sending remote requests with user-controlled data allows for request forgery attacks.
+ * @kind problem
+ * @problem.severity error
+ * @precision medium
+ * @id js/request-forgery
+ * @tags security
+ *       external/cwe/cwe-918
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.RequestForgery::RequestForgery
+
+from Configuration cfg, DataFlow::Node source, Sink sink, DataFlow::Node request
+where cfg.hasFlow(source, sink) and
+      request = sink.getARequest()
+select request, "The $@ of this request depends on $@.", sink, sink.getKind(), source, "a user-provided value"

javascript/ql/src/Security/CWE-918/examples/RequestForgeryBad.js

@@ -0,0 +1,12 @@
+import http from 'http';
+import url from 'url';
+
+var server = http.createServer(function(req, res) {
+    var target = url.parse(request.url, true).query.target;
+
+    // BAD: `target` is controlled by the attacker
+    http.get('https://' + target + ".example.com/data/", res => {
+        // process request response ...
+    });
+
+});

javascript/ql/src/Security/CWE-918/examples/RequestForgeryGood.js

@@ -0,0 +1,19 @@
+import http from 'http';
+import url from 'url';
+
+var server = http.createServer(function(req, res) {
+    var target = url.parse(request.url, true).query.target;
+
+    var subdomain;
+    if (target === 'EU') {
+        subdomain = "europe"
+    } else {
+        subdomain = "world"
+    }
+
+    // GOOD: `subdomain` is controlled by the server
+    http.get('https://' + subdomain + ".example.com/data/", res => {
+        // process request response ...
+    });
+
+});

javascript/ql/src/Statements/MisleadingIndentationAfterControlStmt.ql

@@ -39,6 +39,7 @@ where misleadingIndentationCandidate(ctrl, s1, s2) and
       f.hasIndentation(ctrlStartLine, indent, _) and
       f.hasIndentation(startLine1, indent, _) and
       f.hasIndentation(startLine2, indent, _) and
-      not s2 instanceof EmptyStmt
+      not s2 instanceof EmptyStmt and
+      not f.getFileType().isTypeScript() // ignore quirks in TypeScript tokenizer
 select (FirstLineOf)s2, "The indentation of this statement suggests that it is controlled by $@, while in fact it is not.",
             (FirstLineOf)ctrl, "this statement"

javascript/ql/src/javascript.qll

@@ -54,6 +54,7 @@ import semmle.javascript.frameworks.AWS
 import semmle.javascript.frameworks.Azure
 import semmle.javascript.frameworks.Babel
 import semmle.javascript.frameworks.ComposedFunctions
+import semmle.javascript.frameworks.ClientRequests
 import semmle.javascript.frameworks.Credentials
 import semmle.javascript.frameworks.CryptoLibraries
 import semmle.javascript.frameworks.DigitalOcean

javascript/ql/src/semmle/javascript/Stmt.qll

@@ -607,6 +607,10 @@ abstract class EnhancedForLoop extends LoopStmt {
   override Stmt getBody() {
     result = getChildStmt(2)
   }
+
+  override ControlFlowNode getFirstControlFlowNode() {
+    result = getIteratorExpr().getFirstControlFlowNode()
+  }
 }
 
 /** A `for`-`in` loop. */

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

@@ -0,0 +1,194 @@
+/**
+ * Provides classes for modelling the client-side of a URL request.
+ *
+ * Subclass `ClientRequest` to refine the behavior of the analysis on existing client requests.
+ * Subclass `CustomClientRequest` to introduce new kinds of client requests.
+ */
+
+import javascript
+
+/**
+ * A call that performs a request to a URL.
+ */
+abstract class CustomClientRequest extends DataFlow::InvokeNode {
+
+  /**
+   * Gets the URL of the request.
+   */
+  abstract DataFlow::Node getUrl();
+}
+
+/**
+ * A call that performs a request to a URL.
+ */
+class ClientRequest extends DataFlow::InvokeNode {
+
+  CustomClientRequest custom;
+
+  ClientRequest() {
+    this = custom
+  }
+
+  /**
+   * Gets the URL of the request.
+   */
+  DataFlow::Node getUrl() {
+    result = custom.getUrl()
+  }
+}
+
+/**
+ * Gets name of an HTTP request method, in all-lowercase.
+ */
+private string httpMethodName() {
+  result = any(HTTP::RequestMethodName m).toLowerCase()
+}
+
+/**
+ * Gets the name of a property that likely contains a  URL value.
+ */
+private string urlPropertyName() {
+  result = "uri" or
+  result = "url"
+}
+
+/**
+ * A model of a URL request made using the `request` library.
+ */
+private class RequestUrlRequest extends CustomClientRequest {
+
+  DataFlow::Node url;
+
+  RequestUrlRequest() {
+    exists (string moduleName, DataFlow::SourceNode callee |
+      this = callee.getACall() |
+      (
+        moduleName = "request" or
+        moduleName = "request-promise" or
+        moduleName = "request-promise-any" or
+        moduleName = "request-promise-native"
+      ) and
+      (
+        callee = DataFlow::moduleImport(moduleName) or
+        callee = DataFlow::moduleMember(moduleName, httpMethodName())
+      ) and
+      (
+        url = getArgument(0) or
+        url = getOptionArgument(0, urlPropertyName())
+      )
+    )
+  }
+
+  override DataFlow::Node getUrl() {
+    result = url
+  }
+
+}
+
+/**
+ * A model of a URL request made using the `axios` library.
+ */
+private class AxiosUrlRequest extends CustomClientRequest {
+
+  DataFlow::Node url;
+
+  AxiosUrlRequest() {
+    exists (string moduleName, DataFlow::SourceNode callee |
+      this = callee.getACall() |
+      moduleName = "axios" and
+      (
+        callee = DataFlow::moduleImport(moduleName) or
+        callee = DataFlow::moduleMember(moduleName, httpMethodName()) or
+        callee = DataFlow::moduleMember(moduleName, "request")
+      ) and
+      (
+        url = getArgument(0) or
+        // depends on the method name and the call arity, over-approximating slightly in the name of simplicity
+        url = getOptionArgument([0..2], urlPropertyName())
+      )
+    )
+  }
+
+  override DataFlow::Node getUrl() {
+    result = url
+  }
+
+}
+
+/**
+ * A model of a URL request made using an implementation of the `fetch` API.
+ */
+private class FetchUrlRequest extends CustomClientRequest {
+
+  DataFlow::Node url;
+
+  FetchUrlRequest() {
+    exists (string moduleName, DataFlow::SourceNode callee |
+      this = callee.getACall() |
+      (
+        moduleName = "node-fetch" or
+        moduleName = "cross-fetch" or
+        moduleName = "isomorphic-fetch"
+      ) and
+      callee = DataFlow::moduleImport(moduleName) and
+      url = getArgument(0)
+    )
+    or
+    (
+      this = DataFlow::globalVarRef("fetch").getACall() and
+      url = getArgument(0)
+    )
+  }
+
+  override DataFlow::Node getUrl() {
+    result = url
+  }
+
+}
+
+/**
+ * A model of a URL request made using the `got` library.
+ */
+private class GotUrlRequest extends CustomClientRequest {
+
+  DataFlow::Node url;
+
+  GotUrlRequest() {
+    exists (string moduleName, DataFlow::SourceNode callee |
+      this = callee.getACall() |
+      moduleName = "got" and
+      (
+        callee = DataFlow::moduleImport(moduleName) or
+        callee = DataFlow::moduleMember(moduleName, "stream")
+      ) and
+      url = getArgument(0) and not exists (getOptionArgument(1, "baseUrl"))
+    )
+  }
+
+  override DataFlow::Node getUrl() {
+    result = url
+  }
+
+}
+
+/**
+ * A model of a URL request made using the `superagent` library.
+ */
+private class SuperAgentUrlRequest extends CustomClientRequest {
+
+  DataFlow::Node url;
+
+  SuperAgentUrlRequest() {
+    exists (string moduleName, DataFlow::SourceNode callee |
+      this = callee.getACall() |
+      moduleName = "superagent" and
+      callee = DataFlow::moduleMember(moduleName, httpMethodName()) and
+      url = getArgument(0)
+    )
+  }
+
+  override DataFlow::Node getUrl() {
+    result = url
+  }
+
+}

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

@@ -33,37 +33,51 @@ module Electron {
       this = DataFlow::moduleMember("electron", "BrowserView").getAnInstantiation()
     }
   }
-  
+
   /**
    * A Node.js-style HTTP or HTTPS request made using an Electron module.
    */
-  abstract class ClientRequest extends NodeJSLib::ClientRequest {}
+  abstract class CustomElectronClientRequest extends NodeJSLib::CustomNodeJSClientRequest {}
+
+  /**
+   * A Node.js-style HTTP or HTTPS request made using an Electron module.
+   */
+  class ElectronClientRequest extends NodeJSLib::NodeJSClientRequest {
+
+    ElectronClientRequest() {
+      this instanceof CustomElectronClientRequest
+    }
+
+  }
   
   /**
    * A Node.js-style HTTP or HTTPS request made using `electron.net`, for example `net.request(url)`.
    */
-  private class NetRequest extends ClientRequest {
+  private class NetRequest extends CustomElectronClientRequest {
     NetRequest() {
       this = DataFlow::moduleMember("electron", "net").getAMemberCall("request")
     }
-    
-    override DataFlow::Node getOptions() {
-      result = this.(DataFlow::MethodCallNode).getArgument(0)
+
+    override DataFlow::Node getUrl() {
+      result = getArgument(0) or
+      result = getOptionArgument(0, "url")
     }
+
   }
-  
-  
+
   /**
    * A Node.js-style HTTP or HTTPS request made using `electron.client`, for example `new client(url)`.
    */
-  private class NewClientRequest extends ClientRequest {
+  private class NewClientRequest extends CustomElectronClientRequest {
     NewClientRequest() {
       this = DataFlow::moduleMember("electron", "ClientRequest").getAnInstantiation()
     }
-    
-    override DataFlow::Node getOptions() {
-      result = this.(DataFlow::NewNode).getArgument(0)
+
+    override DataFlow::Node getUrl() {
+      result = getArgument(0) or
+      result = getOptionArgument(0, "url")
     }
+
   }
   
     
@@ -75,12 +89,12 @@ module Electron {
       exists(NodeJSLib::ClientRequestHandler handler |
         this = handler.getParameter(0) and
         handler.getAHandledEvent() = "redirect" and
-        handler.getClientRequest() instanceof ClientRequest
+        handler.getClientRequest() instanceof ElectronClientRequest
       )
     }
     
     override string getSourceType() {
-      result = "Electron ClientRequest redirect event"
+      result = "ElectronClientRequest redirect event"
     }
   }
 }

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

@@ -502,53 +502,47 @@ module NodeJSLib {
   }
 
   /**
-   * A data flow node that is an HTTP or HTTPS client request made by a Node.js server, for example `http.request(url)`.
+   * A data flow node that is an HTTP or HTTPS client request made by a Node.js application, for example `http.request(url)`.
    */
-  abstract class ClientRequest extends DataFlow::DefaultSourceNode {
-    /**
-     * Gets the options object or string URL used to make the request.
-     */
-    abstract DataFlow::Node getOptions();
+  abstract class CustomNodeJSClientRequest extends CustomClientRequest {
+
+  }
+
+  /**
+   * A data flow node that is an HTTP or HTTPS client request made by a Node.js application, for example `http.request(url)`.
+   */
+  class NodeJSClientRequest extends ClientRequest {
+
+    NodeJSClientRequest() {
+      this instanceof CustomNodeJSClientRequest
+    }
+
   }
   
   /**
-   * A data flow node that is an HTTP or HTTPS client request made by a Node.js server, for example `http.request(url)`.
+   * A model of a URL request in the Node.js `http` library.
    */
-  private class HttpRequest extends ClientRequest {
-    HttpRequest() {
-      exists(string protocol |
+  private class NodeHttpUrlRequest extends CustomNodeJSClientRequest {
+
+    DataFlow::Node url;
+
+    NodeHttpUrlRequest() {
+      exists (string moduleName, DataFlow::SourceNode callee |
+        this = callee.getACall() |
+        (moduleName = "http" or moduleName = "https") and
         (
-          protocol = "http" or
-          protocol = "https"
-        )
-        and
-        this = DataFlow::moduleImport(protocol).getAMemberCall("request")
+          callee = DataFlow::moduleMember(moduleName, any(HTTP::RequestMethodName m).toLowerCase())
+          or
+          callee = DataFlow::moduleMember(moduleName, "request")
+        ) and
+        url = getArgument(0)
       )
     }
-    
-    override DataFlow::Node getOptions() {
-      result = this.(DataFlow::MethodCallNode).getArgument(0)
-    }
-  }
-  
-  /**
-   * A data flow node that is an HTTP or HTTPS client request made by a Node.js process, for example `https.get(url)`.
-   */
-  private class HttpGet extends ClientRequest {
-    HttpGet() {
-      exists(string protocol |
-        (
-          protocol = "http" or
-          protocol = "https"
-        )
-        and
-        this = DataFlow::moduleImport(protocol).getAMemberCall("get")
-      )
-    }
-    
-    override DataFlow::Node getOptions() {
-      result = this.(DataFlow::MethodCallNode).getArgument(0)
+
+    override DataFlow::Node getUrl() {
+      result = url
     }
+
   }
   
   /**
@@ -556,13 +550,13 @@ module NodeJSLib {
    */
   private class ClientRequestCallbackParam extends DataFlow::ParameterNode, RemoteFlowSource {
     ClientRequestCallbackParam() {
-      exists(ClientRequest req |
+      exists(NodeJSClientRequest req |
         this = req.(DataFlow::MethodCallNode).getCallback(1).getParameter(0)
       )
     }
     
     override string getSourceType() {
-      result = "ClientRequest callback parameter"
+      result = "NodeJSClientRequest callback parameter"
     }
   }
   
@@ -589,7 +583,7 @@ module NodeJSLib {
    */
   class ClientRequestHandler extends DataFlow::FunctionNode {
     string handledEvent;
-    ClientRequest clientRequest;
+    NodeJSClientRequest clientRequest;
     
     ClientRequestHandler() {
       exists(DataFlow::MethodCallNode mcn |
@@ -609,7 +603,7 @@ module NodeJSLib {
     /**
      * Gets a request this callback is registered for.
      */
-    ClientRequest getClientRequest() {
+    NodeJSClientRequest getClientRequest() {
       result = clientRequest
     }
   }
@@ -626,7 +620,7 @@ module NodeJSLib {
     }
     
     override string getSourceType() {
-      result = "ClientRequest response event"
+      result = "NodeJSClientRequest response event"
     }
   }
   
@@ -643,7 +637,7 @@ module NodeJSLib {
     }
     
     override string getSourceType() {
-      result = "ClientRequest data event"
+      result = "NodeJSClientRequest data event"
     }
   }
   
@@ -667,7 +661,7 @@ module NodeJSLib {
     }
     
     override string getSourceType() {
-      result = "ClientRequest login event"
+      result = "NodeJSClientRequest login event"
     }
   }
   
@@ -725,7 +719,7 @@ module NodeJSLib {
     }
     
     override string getSourceType() {
-      result = "ClientRequest error event"
+      result = "NodeJSClientRequest error event"
     }
   }
 }

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

@@ -0,0 +1,87 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about request forgery.
+ */
+
+import semmle.javascript.security.dataflow.RemoteFlowSources
+import UrlConcatenation
+
+module RequestForgery {
+
+  /**
+   * A data flow source for request forgery.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for request forgery.
+   */
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets a request that uses this sink.
+     */
+    abstract DataFlow::Node getARequest();
+
+    /**
+     * Gets the kind of this sink.
+     */
+    abstract string getKind();
+
+  }
+
+  /**
+   * A sanitizer for request forgery.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A taint tracking configuration for request forgery.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() {
+      this = "RequestForgery"
+    }
+
+    override predicate isSource(DataFlow::Node source) {
+      source instanceof Source
+    }
+
+    override predicate isSink(DataFlow::Node sink) {
+      sink instanceof Sink
+    }
+
+    override predicate isSanitizer(DataFlow::Node node) {
+      super.isSanitizer(node) or
+      node instanceof Sanitizer
+    }
+
+    override predicate isSanitizer(DataFlow::Node source, DataFlow::Node sink) {
+      sanitizingPrefixEdge(source, sink)
+    }
+
+  }
+
+  /** A source of remote user input, considered as a flow source for request forgery. */
+  private class RemoteFlowSourceAsSource extends Source {
+    RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
+  }
+
+  /**
+   * The URL of a URL request, viewed as a sink for request forgery.
+   */
+  private class ClientRequestUrlAsSink extends Sink {
+
+    ClientRequest request;
+
+    ClientRequestUrlAsSink() {
+      this = request.getUrl()
+    }
+
+    override DataFlow::Node getARequest() {
+      result = request
+    }
+
+    override string getKind() {
+      result = "URL"
+    }
+  }
+}