Merge pull request #7823 from JLLeitschuh/improve/JLL/combined_http_headers

Java: Add HTTP Request Splitting to Netty Query

java/ql/src/Security/CWE/CWE-113/NettyRequestSplitting.java

@@ -0,0 +1,13 @@
+public class NettyRequestSplitting {
+    // BAD: Disables the internal response splitting verification
+    private final DefaultHttpHeaders badHeaders = new DefaultHttpHeaders(false);
+
+    // GOOD: Verifies headers passed don't contain CRLF characters
+    private final DefaultHttpHeaders goodHeaders = new DefaultHttpHeaders();
+
+    // BAD: Disables the internal response splitting verification
+    private final DefaultHttpRequest badRequest = new DefaultHttpRequest(httpVersion, method, uri, false);
+
+    // GOOD: Verifies headers passed don't contain CRLF characters
+    private final DefaultHttpRequest goodResponse = new DefaultHttpRequest(httpVersion, method, uri);
+}

java/ql/src/Security/CWE/CWE-113/NettyResponseSplitting.ql

@@ -7,35 +7,82 @@
  * @problem.severity error
  * @security-severity 6.1
  * @precision high
- * @id java/netty-http-response-splitting
+ * @id java/netty-http-request-or-response-splitting
  * @tags security
+ *       external/cwe/cwe-93
  *       external/cwe/cwe-113
  */
 
 import java
+import semmle.code.java.dataflow.FlowSources
 
-abstract private class InsecureNettyObjectCreation extends ClassInstanceExpr { }
+abstract private class InsecureNettyObjectCreation extends ClassInstanceExpr {
+  int vulnerableArgumentIndex;
 
-private class InsecureDefaultHttpHeadersClassInstantiation extends InsecureNettyObjectCreation {
+  InsecureNettyObjectCreation() {
+    DataFlow::localExprFlow(any(CompileTimeConstantExpr ctce | ctce.getBooleanValue() = false),
+      this.getArgument(vulnerableArgumentIndex))
+  }
+
+  abstract string splittingType();
+}
+
+abstract private class RequestOrResponseSplittingInsecureNettyObjectCreation extends InsecureNettyObjectCreation {
+  override string splittingType() { result = "Request splitting or response splitting" }
+}
+
+/**
+ * Request splitting can allowing an attacker to inject/smuggle an additional HTTP request into the socket connection.
+ */
+abstract private class RequestSplittingInsecureNettyObjectCreation extends InsecureNettyObjectCreation {
+  override string splittingType() { result = "Request splitting" }
+}
+
+/**
+ * Response splitting can lead to HTTP vulnerabilities like XSS and cache poisoning.
+ */
+abstract private class ResponseSplittingInsecureNettyObjectCreation extends InsecureNettyObjectCreation {
+  override string splittingType() { result = "Response splitting" }
+}
+
+private class InsecureDefaultHttpHeadersClassInstantiation extends RequestOrResponseSplittingInsecureNettyObjectCreation {
   InsecureDefaultHttpHeadersClassInstantiation() {
-    getConstructedType().hasQualifiedName("io.netty.handler.codec.http", "DefaultHttpHeaders") and
-    getArgument(0).(CompileTimeConstantExpr).getBooleanValue() = false
+    this.getConstructedType()
+        .hasQualifiedName("io.netty.handler.codec.http",
+          ["DefaultHttpHeaders", "CombinedHttpHeaders"]) and
+    vulnerableArgumentIndex = 0
   }
 }
 
-private class InsecureDefaultHttpResponseClassInstantiation extends InsecureNettyObjectCreation {
+private class InsecureDefaultHttpResponseClassInstantiation extends ResponseSplittingInsecureNettyObjectCreation {
   InsecureDefaultHttpResponseClassInstantiation() {
-    getConstructedType().hasQualifiedName("io.netty.handler.codec.http", "DefaultHttpResponse") and
-    getArgument(2).(CompileTimeConstantExpr).getBooleanValue() = false
+    this.getConstructedType().hasQualifiedName("io.netty.handler.codec.http", "DefaultHttpResponse") and
+    vulnerableArgumentIndex = 2
   }
 }
 
-private class InsecureDefaultFullHttpResponseClassInstantiation extends InsecureNettyObjectCreation {
+private class InsecureDefaultHttpRequestClassInstantiation extends RequestSplittingInsecureNettyObjectCreation {
+  InsecureDefaultHttpRequestClassInstantiation() {
+    this.getConstructedType().hasQualifiedName("io.netty.handler.codec.http", "DefaultHttpRequest") and
+    vulnerableArgumentIndex = 3
+  }
+}
+
+private class InsecureDefaultFullHttpResponseClassInstantiation extends ResponseSplittingInsecureNettyObjectCreation {
   InsecureDefaultFullHttpResponseClassInstantiation() {
-    getConstructedType().hasQualifiedName("io.netty.handler.codec.http", "DefaultFullHttpResponse") and
-    getArgument(3).(CompileTimeConstantExpr).getBooleanValue() = false
+    this.getConstructedType()
+        .hasQualifiedName("io.netty.handler.codec.http", "DefaultFullHttpResponse") and
+    vulnerableArgumentIndex = [2, 3]
+  }
+}
+
+private class InsecureDefaultFullHttpRequestClassInstantiation extends RequestSplittingInsecureNettyObjectCreation {
+  InsecureDefaultFullHttpRequestClassInstantiation() {
+    this.getConstructedType()
+        .hasQualifiedName("io.netty.handler.codec.http", "DefaultFullHttpRequest") and
+    vulnerableArgumentIndex = [3, 4]
   }
 }
 
 from InsecureNettyObjectCreation new
-select new, "Response-splitting vulnerability due to header value verification being disabled."
+select new, new.splittingType() + " vulnerability due to header value verification being disabled."

java/ql/src/Security/CWE/CWE-113/ResponseSplitting.qhelp

@@ -5,16 +5,24 @@
 
 <overview>
 <p>Directly writing user input (for example, an HTTP request parameter) to an HTTP header
-can lead to an HTTP response-splitting vulnerability.
-If the user input includes blank lines in it, and if the servlet container does not itself
-escape the blank lines, then a remote user can cause the response to turn into two separate
-responses, one of which is controlled by the remote user.</p>
+can lead to an HTTP request-splitting or response-splitting vulnerability.</p>
+
+<p>HTTP response splitting can lead to vulnerabilities such as XSS and cache poisoning.</p>
+<p>HTTP request splitting can allow an attacker to inject an additional HTTP request into a client's outgoing socket connection.
+This can allow an attacker to perform an SSRF-like attack.</p>
+
+<p>In the context of a servlet container, if the user input includes blank lines
+and the servlet container does not escape the blank lines,
+then a remote user can cause the response to turn into two separate responses. 
+The remote user can then control one or more responses, which is also HTTP response splitting.</p>
 </overview>
 
 <recommendation>
 <p>Guard against HTTP header splitting in the same way as guarding against cross-site scripting.
 Before passing any data into HTTP headers, either check the data for special characters, or
 escape any special characters that are present.</p>
+
+<p>If the code calls Netty API's directly, ensure that the <code>validateHeaders</code> parameter is set to <code>true</code>.</p>
 </recommendation>
 
 <example>
@@ -33,6 +41,13 @@ The second way will verify the parameters before using them to build the HTTP re
 <sample src="NettyResponseSplitting.java" />
 </example>
 
+<example>
+<p>The following example shows the use of the netty library with configurations for verification of HTTP request splitting.
+The second recommended approach in the example verifies the parameters before using them to build the HTTP request.</p>
+
+<sample src="NettyRequestSplitting.java" />
+</example>
+
 <references>
 <li>
 InfosecWriters: <a href="http://www.infosecwriters.com/Papers/DCrab_HTTP_Response.pdf">HTTP response splitting</a>.
@@ -44,5 +59,8 @@ OWASP:
 <li>
 Wikipedia: <a href="http://en.wikipedia.org/wiki/HTTP_response_splitting">HTTP response splitting</a>.
 </li>
+<li>
+CAPEC: <a href="https://capec.mitre.org/data/definitions/105.html">CAPEC-105: HTTP Request Splitting</a>
+</li>
 </references>
 </qhelp>

java/ql/src/change-notes/2021-01-02-netty-response-splitting-improve.md

@@ -0,0 +1,9 @@
+---
+category: breaking
+---
+* Add more classes to Netty request/response splitting. Change identification to `java/netty-http-request-or-response-splitting`.
+  Identify request splitting differently from response splitting in query results.
+  Support addional classes:
+  * `io.netty.handler.codec.http.CombinedHttpHeaders`
+  * `io.netty.handler.codec.http.DefaultHttpRequest`
+  * `io.netty.handler.codec.http.DefaultFullHttpRequest`