Merge pull request github#5587 from smowton/smowton/admin/promote-ssrf-query

Promote SSRF query from experimental

Author: smowton

java/change-notes/2021-04-06-ssrf-query.md

@@ -0,0 +1,4 @@
+lgtm,codescanning
+* The query "Server-side request forgery (SSRF)" (`java/ssrf`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @porcupineyhairs](https://github.com/github/codeql/pull/3454).
+* Models for `URI` and `HttpRequest` in the `java.net` package have been improved. This may lead to more results from any query where these types' methods are relevant.
+* Models for Apache HttpComponents' `RequestLine` and `BasicRequestLine` types. This may lead to more results from any query where these types' methods are relevant.

java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.java renamed to java/ql/src/Security/CWE/CWE-918/RequestForgery.java

@@ -5,22 +5,24 @@
 
 
 <overview>
-<p>Directly incorporating user input into a HTTP request without validating the input
-can facilitate Server Side Request Forgery (SSRF) attacks. In these attacks, the server
-may be tricked into making a request and interacting with an attacker-controlled server. 
+<p>Directly incorporating user input into an HTTP request without validating the input
+can facilitate server-side request forgery (SSRF) attacks. In these attacks, the server
+may be tricked into making a request and interacting with an attacker-controlled server.
 </p>
 
 </overview>
 <recommendation>
 
-<p>To guard against SSRF attacks, it is advisable to avoid putting user input
-directly into the request URL. Instead, maintain a list of authorized
-URLs on the server; then choose from that list based on the user input provided.</p>
+<p>To guard against SSRF attacks, you should avoid putting user-provided input
+directly into a request URL. Instead, maintain a list of authorized
+URLs on the server; then choose from that list based on the input provided.
+Alternatively, ensure requests constructed from user input are limited to
+a particular host or more restrictive URL prefix.</p>
 
 </recommendation>
 <example>
 
-<p>The following example shows an HTTP request parameter being used directly in a forming a
+<p>The following example shows an HTTP request parameter being used directly to form a
 new request without validating the input, which facilitates SSRF attacks.
 It also shows how to remedy the problem by validating the user input against a known fixed string.
 </p>

java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.qhelp renamed to java/ql/src/Security/CWE/CWE-918/RequestForgery.qhelp

@@ -0,0 +1,20 @@
+/**
+ * @name Server-side request forgery
+ * @description Making web requests based on unvalidated user-input
+ *              may cause the server to communicate with malicious servers.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/ssrf
+ * @tags security
+ *       external/cwe/cwe-918
+ */
+
+import java
+import semmle.code.java.security.RequestForgeryConfig
+import DataFlow::PathGraph
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, RequestForgeryConfiguration conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Potential server-side request forgery due to $@.",
+  source.getNode(), "a user-provided value"

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

@@ -194,15 +194,6 @@ predicate urlOpen(DataFlow::Node node1, DataFlow::Node node2) {
   )
 }
 
-/** Constructor of `BasicRequestLine` */
-predicate basicRequestLine(DataFlow::Node node1, DataFlow::Node node2) {
-  exists(ConstructorCall mcc |
-    mcc.getConstructedType().hasQualifiedName("org.apache.http.message", "BasicRequestLine") and
-    mcc.getArgument(1) = node1.asExpr() and // `BasicRequestLine(String method, String uri, ProtocolVersion version)
-    node2.asExpr() = mcc
-  )
-}
-
 class BasicAuthFlowConfig extends TaintTracking::Configuration {
   BasicAuthFlowConfig() { this = "InsecureBasicAuth::BasicAuthFlowConfig" }
 
@@ -236,7 +227,6 @@ class BasicAuthFlowConfig extends TaintTracking::Configuration {
   override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
     apacheHttpRequest(node1, node2) or
     createURI(node1, node2) or
-    basicRequestLine(node1, node2) or
     createURL(node1, node2) or
     urlOpen(node1, node2)
   }

java/ql/src/experimental/Security/CWE/CWE-522/InsecureBasicAuth.ql

@@ -175,6 +175,19 @@ class FormattingCall extends Call {
     )
   }
 
+  /** Gets the `i`th argument to be formatted. The index `i` is one-based. */
+  Expr getArgumentToBeFormatted(int i) {
+    i >= 1 and
+    if this.hasExplicitVarargsArray()
+    then
+      result =
+        this.getArgument(1 + this.getFormatStringIndex())
+            .(ArrayCreationExpr)
+            .getInit()
+            .getInit(i - 1)
+    else result = this.getArgument(this.getFormatStringIndex() + i)
+  }
+
   /** Holds if the varargs argument is given as an explicit array. */
   private predicate hasExplicitVarargsArray() {
     this.getNumArgument() = this.getFormatStringIndex() + 2 and
@@ -353,6 +366,11 @@ class FormatString extends string {
    * is not referred by any format specifier.
    */
   /*abstract*/ int getASkippedFmtSpecIndex() { none() }
+
+  /**
+   * Gets an offset (zero-based) in this format string where argument `argNo` (1-based) will be interpolated, if any.
+   */
+  int getAnArgUsageOffset(int argNo) { none() }
 }
 
 private class PrintfFormatString extends FormatString {
@@ -425,6 +443,22 @@ private class PrintfFormatString extends FormatString {
     result > count(int i | fmtSpecRefersToSequentialIndex(i)) and
     not result = fmtSpecRefersToSpecificIndex(_)
   }
+
+  private int getFmtSpecRank(int specOffset) {
+    rank[result](int i | this.fmtSpecIsRef(i)) = specOffset
+  }
+
+  override int getAnArgUsageOffset(int argNo) {
+    argNo = fmtSpecRefersToSpecificIndex(result)
+    or
+    result = rank[argNo](int i | fmtSpecRefersToSequentialIndex(i))
+    or
+    fmtSpecRefersToPrevious(result) and
+    exists(int previousOffset |
+      getFmtSpecRank(previousOffset) = getFmtSpecRank(result) - 1 and
+      previousOffset = getAnArgUsageOffset(argNo)
+    )
+  }
 }
 
 private class LoggerFormatString extends FormatString {
@@ -449,4 +483,6 @@ private class LoggerFormatString extends FormatString {
   }
 
   override int getMaxFmtSpecIndex() { result = count(int i | fmtPlaceholder(i)) }
+
+  override int getAnArgUsageOffset(int argNo) { result = rank[argNo](int i | fmtPlaceholder(i)) }
 }

java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.ql

@@ -82,6 +82,8 @@ private module Frameworks {
   private import semmle.code.java.frameworks.guava.Guava
   private import semmle.code.java.frameworks.jackson.JacksonSerializability
   private import semmle.code.java.frameworks.JaxWS
+  private import semmle.code.java.frameworks.spring.SpringHttp
+  private import semmle.code.java.frameworks.spring.SpringWebClient
   private import semmle.code.java.security.ResponseSplitting
   private import semmle.code.java.security.InformationLeak
   private import semmle.code.java.security.XSS
@@ -209,6 +211,8 @@ private predicate sinkModelCsv(string row) {
       // Open URL
       "java.net;URL;false;openConnection;;;Argument[-1];open-url",
       "java.net;URL;false;openStream;;;Argument[-1];open-url",
+      "java.net.http;HttpRequest;false;newBuilder;;;Argument[0];open-url",
+      "java.net.http;HttpRequest$Builder;false;uri;;;Argument[0];open-url",
       // Create file
       "java.io;FileOutputStream;false;FileOutputStream;;;Argument[0];create-file",
       "java.io;RandomAccessFile;false;RandomAccessFile;;;Argument[0];create-file",
@@ -248,6 +252,8 @@ private predicate summaryModelCsv(string row) {
       "javax.xml.transform.stream;StreamSource;false;getInputStream;;;Argument[-1];ReturnValue;taint",
       "java.nio;ByteBuffer;false;get;;;Argument[-1];ReturnValue;taint",
       "java.net;URI;false;toURL;;;Argument[-1];ReturnValue;taint",
+      "java.net;URI;false;toString;;;Argument[-1];ReturnValue;taint",
+      "java.net;URI;false;toAsciiString;;;Argument[-1];ReturnValue;taint",
       "java.io;File;false;toURI;;;Argument[-1];ReturnValue;taint",
       "java.io;File;false;toPath;;;Argument[-1];ReturnValue;taint",
       "java.nio.file;Path;false;toFile;;;Argument[-1];ReturnValue;taint",