Merge pull request #6714 from valeria-meli/javascript/ssrf

Approved by asgerf

javascript/ql/src/experimental/Security/CWE-918/SSRF.js

@@ -0,0 +1,15 @@
+const axios = require('axios');
+
+export const handler = async (req, res, next) => {
+  const { target } = req.body;
+
+  try {
+    // BAD: `target` is controlled by the attacker
+    const response = await axios.get('https://example.com/current_api/' + target);
+
+    // process request response
+    use(response);
+  } catch (err) {
+    // process error
+  }
+};

javascript/ql/src/experimental/Security/CWE-918/SSRF.qhelp

@@ -0,0 +1,49 @@
+<!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
+        server side request forgery attacks, where the attacker essentially controls the request.
+    </p>
+</overview>
+
+<recommendation>
+    <p>
+        To guard against server side request forgery, it is advisable to avoid putting user input directly into a
+        network request. If using user input is necessary, then is mandatory to validate them. Only allow numeric and alphanumeric values.
+        URL encoding is not a solution in certain scenarios, such as, an architecture build over NGINX proxies.
+    </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>axios.get("https://example.com/current_api/"+target)</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>"../super_secret_api"</code> as the target, causing the
+        URL to become <code>"https://example.com/super_secret_api"</code>.
+    </p>
+
+    <p>
+        A request to <code>https://example.com/super_secret_api</code> may be problematic if that api is not
+        meant to be directly accessible from the attacker's machine.
+    </p>
+
+    <sample src="SSRF.js"/>
+
+    <p>
+        One way to remedy the problem is to validate the user input to only allow alphanumeric values:
+    </p>
+
+    <sample src="SSRFGood.js"/>
+</example>
+
+<references>
+
+<li>OWASP: <a href="https://www.owasp.org/www-community/attacks/Server_Side_Request_Forgery">SSRF</a></li>
+
+</references>
+</qhelp>

javascript/ql/src/experimental/Security/CWE-918/SSRF.ql

@@ -0,0 +1,19 @@
+/**
+ * @id javascript/ssrf
+ * @kind path-problem
+ * @name Uncontrolled data used in network request
+ * @description Sending network requests with user-controlled data as part of the URL allows for request forgery attacks.
+ * @problem.severity error
+ * @precision medium
+ * @tags security
+ *       external/cwe/cwe-918
+ */
+
+import javascript
+import SSRF
+import DataFlow::PathGraph
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node request
+where
+  cfg.hasFlowPath(source, sink) and request = sink.getNode().(RequestForgery::Sink).getARequest()
+select sink, source, sink, "The URL of this request depends on a user-provided value"

javascript/ql/src/experimental/Security/CWE-918/SSRF.qll

@@ -0,0 +1,154 @@
+import javascript
+import semmle.javascript.security.dataflow.RequestForgeryCustomizations
+import semmle.javascript.security.dataflow.UrlConcatenation
+
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "SSRF" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RequestForgery::Source }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof RequestForgery::Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    super.isSanitizer(node) or
+    node instanceof RequestForgery::Sanitizer
+  }
+
+  private predicate hasSanitizingSubstring(DataFlow::Node nd) {
+    nd.getStringValue().regexpMatch(".*[?#].*")
+    or
+    hasSanitizingSubstring(StringConcatenation::getAnOperand(nd))
+    or
+    hasSanitizingSubstring(nd.getAPredecessor())
+  }
+
+  private predicate strictSanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) {
+    exists(DataFlow::Node operator, int n |
+      StringConcatenation::taintStep(source, sink, operator, n) and
+      hasSanitizingSubstring(StringConcatenation::getOperand(operator, [0 .. n - 1]))
+    )
+  }
+
+  override predicate isSanitizerEdge(DataFlow::Node source, DataFlow::Node sink) {
+    strictSanitizingPrefixEdge(source, sink)
+  }
+
+  override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) {
+    nd instanceof IntegerCheck or
+    nd instanceof ValidatorCheck or
+    nd instanceof TernaryOperatorSanitizerGuard
+  }
+}
+
+/**
+ * This sanitizers models the next example:
+ * let valid = req.params.id ? Number.isInteger(req.params.id) : false
+ * if (valid) { sink(req.params.id) }
+ *
+ * This sanitizer models this way of using ternary operators,
+ * when the sanitizer guard is used as any of the branches
+ * instead of being used as the condition.
+ *
+ * This sanitizer sanitize the corresponding if statement branch.
+ */
+class TernaryOperatorSanitizer extends RequestForgery::Sanitizer {
+  TernaryOperatorSanitizer() {
+    exists(
+      TaintTracking::SanitizerGuardNode guard, IfStmt ifStmt, DataFlow::Node taintedInput,
+      boolean outcome, Stmt r, DataFlow::Node falseNode
+    |
+      ifStmt.getCondition().flow().getAPredecessor+() = guard and
+      ifStmt.getCondition().flow().getAPredecessor+() = falseNode and
+      falseNode.asExpr().(BooleanLiteral).mayHaveBooleanValue(false) and
+      not ifStmt.getCondition() instanceof LogicalBinaryExpr and
+      guard.sanitizes(outcome, taintedInput.asExpr()) and
+      (
+        outcome = true and r = ifStmt.getThen() and not ifStmt.getCondition() instanceof LogNotExpr
+        or
+        outcome = false and r = ifStmt.getElse() and not ifStmt.getCondition() instanceof LogNotExpr
+        or
+        outcome = false and r = ifStmt.getThen() and ifStmt.getCondition() instanceof LogNotExpr
+        or
+        outcome = true and r = ifStmt.getElse() and ifStmt.getCondition() instanceof LogNotExpr
+      ) and
+      r.getFirstControlFlowNode()
+          .getBasicBlock()
+          .(ReachableBasicBlock)
+          .dominates(this.getBasicBlock())
+    )
+  }
+}
+
+/**
+ * This sanitizer guard is another way of modeling the example from above
+ * In this case:
+ * let valid = req.params.id ? Number.isInteger(req.params.id) : false
+ * if (!valid) { return }
+ * sink(req.params.id)
+ *
+ * The previous sanitizer is not enough,
+ * because we are sanitizing the entire if statement branch
+ * but we need to sanitize the use of this variable from now on.
+ *
+ * Thats why we model this sanitizer guard which says that
+ * the result of the ternary operator execution is a sanitizer guard.
+ */
+class TernaryOperatorSanitizerGuard extends TaintTracking::SanitizerGuardNode {
+  TaintTracking::SanitizerGuardNode originalGuard;
+
+  TernaryOperatorSanitizerGuard() {
+    this.getAPredecessor+().asExpr().(BooleanLiteral).mayHaveBooleanValue(false) and
+    this.getAPredecessor+() = originalGuard and
+    not this.asExpr() instanceof LogicalBinaryExpr
+  }
+
+  override predicate sanitizes(boolean outcome, Expr e) {
+    not this.asExpr() instanceof LogNotExpr and
+    originalGuard.sanitizes(outcome, e)
+    or
+    exists(boolean originalOutcome |
+      this.asExpr() instanceof LogNotExpr and
+      originalGuard.sanitizes(originalOutcome, e) and
+      (
+        originalOutcome = true and outcome = false
+        or
+        originalOutcome = false and outcome = true
+      )
+    )
+  }
+}
+
+/**
+ * Number.isInteger is a sanitizer guard because a number can't be used to exploit a SSRF.
+ */
+class IntegerCheck extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode {
+  IntegerCheck() { this = DataFlow::globalVarRef("Number").getAMemberCall("isInteger") }
+
+  override predicate sanitizes(boolean outcome, Expr e) {
+    outcome = true and
+    e = getArgument(0).asExpr()
+  }
+}
+
+/**
+ * ValidatorCheck identifies if exists a call to validator's library methods.
+ * validator is a library which has a variety of input-validation functions. We are interesed in
+ * checking that source is a number (any type of number) or an alphanumeric value.
+ */
+class ValidatorCheck extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode {
+  ValidatorCheck() {
+    exists(DataFlow::SourceNode mod, string method |
+      mod = DataFlow::moduleImport("validator") and
+      this = mod.getAChainedMethodCall(method) and
+      method in [
+          "isAlphanumeric", "isAlpha", "isDecimal", "isFloat", "isHexadecimal", "isHexColor",
+          "isInt", "isNumeric", "isOctal", "isUUID"
+        ]
+    )
+  }
+
+  override predicate sanitizes(boolean outcome, Expr e) {
+    outcome = true and
+    e = getArgument(0).asExpr()
+  }
+}

javascript/ql/src/experimental/Security/CWE-918/SSRFGood.js

@@ -0,0 +1,20 @@
+const axios = require('axios');
+const validator = require('validator');
+
+export const handler = async (req, res, next) => {
+  const { target } = req.body;
+
+  if (!validator.isAlphanumeric(target)) {
+    return next(new Error('Bad request'));
+  }
+
+  try {
+    // `target` is validated
+    const response = await axios.get('https://example.com/current_api/' + target);
+
+    // process request response
+    use(response);
+  } catch (err) {
+    // process error
+  }
+};