+
+
+
+
+ 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.
+
+
+
+
+
+
+
+
+ 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.
+
+
+
+
+
+
+
+
+
+ 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
+ http.get(...) is vulnerable since attackers can choose
+ the value of target to be anything they want. For
+ instance, the attacker can choose
+ "internal.example.com/#" as the target, causing the URL
+ used in the request to be
+ "https://internal.example.com/#.example.com/data".
+
+
+
+
+
+ A request to https://internal.example.com may
+ be problematic if that server is not meant to be
+ directly accessible from the attacker's machine.
+
+
+
+
+
+
+
+ One way to remedy the problem is to use the user input to
+ select a known fixed string before performing the request:
+
+
+
+
+
+
+
+
+
+ OWASP: SSRF
+
+
+
diff --git a/javascript/ql/src/Security/CWE-918/RequestForgery.ql b/javascript/ql/src/Security/CWE-918/RequestForgery.ql
new file mode 100644
index 000000000000..8035f2ef56fb
--- /dev/null
+++ b/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"
diff --git a/javascript/ql/src/Security/CWE-918/examples/RequestForgeryBad.js b/javascript/ql/src/Security/CWE-918/examples/RequestForgeryBad.js
new file mode 100644
index 000000000000..d0eb8b56b0fc
--- /dev/null
+++ b/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 ...
+ });
+
+});
diff --git a/javascript/ql/src/Security/CWE-918/examples/RequestForgeryGood.js b/javascript/ql/src/Security/CWE-918/examples/RequestForgeryGood.js
new file mode 100644
index 000000000000..2c1662a5a1e7
--- /dev/null
+++ b/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 ...
+ });
+
+});
diff --git a/javascript/ql/src/javascript.qll b/javascript/ql/src/javascript.qll
index 436520d3fcf1..16bdff91f76a 100644
--- a/javascript/ql/src/javascript.qll
+++ b/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
diff --git a/javascript/ql/src/semmle/javascript/Stmt.qll b/javascript/ql/src/semmle/javascript/Stmt.qll
index facc438e12b4..486298a38965 100644
--- a/javascript/ql/src/semmle/javascript/Stmt.qll
+++ b/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. */
diff --git a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
new file mode 100644
index 000000000000..a19b5effca35
--- /dev/null
+++ b/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
+ }
+
+}
diff --git a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll b/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
index a60309c9be10..b7c80eca8031 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/Electron.qll
+++ b/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 CustomElectronClientRequest extends NodeJSLib::CustomNodeJSClientRequest {}
+
/**
* A Node.js-style HTTP or HTTPS request made using an Electron module.
*/
- abstract class ClientRequest extends NodeJSLib::ClientRequest {}
+ 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"
}
}
}
\ No newline at end of file
diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
index 6a11bef5edc5..ba4dc9731e7f 100644
--- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
+++ b/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 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)`.
*/
- private class HttpRequest extends ClientRequest {
- HttpRequest() {
- exists(string protocol |
- (
- protocol = "http" or
- protocol = "https"
- )
- and
- this = DataFlow::moduleImport(protocol).getAMemberCall("request")
- )
- }
-
- override DataFlow::Node getOptions() {
- result = this.(DataFlow::MethodCallNode).getArgument(0)
+ 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 process, for example `https.get(url)`.
+ * A model of a URL request in the Node.js `http` library.
*/
- private class HttpGet extends ClientRequest {
- HttpGet() {
- 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("get")
+ 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)
+
+ 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"
}
}
}
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/RequestForgery.qll b/javascript/ql/src/semmle/javascript/security/dataflow/RequestForgery.qll
new file mode 100644
index 000000000000..6881382672ab
--- /dev/null
+++ b/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"
+ }
+ }
+}
diff --git a/javascript/ql/test/library-tests/CFG/CFG.expected b/javascript/ql/test/library-tests/CFG/CFG.expected
index d2a79ea52a5a..c78a966ed341 100644
--- a/javascript/ql/test/library-tests/CFG/CFG.expected
+++ b/javascript/ql/test/library-tests/CFG/CFG.expected
@@ -644,15 +644,14 @@
| forof | 1 | entry node of functio ... ;\\n} | 2 | {\\n f ... ;\\n} |
| forof | 1 | f | 1 | functio ... ;\\n} |
| forof | 1 | functio ... ;\\n} | 10 | exit node of