Add HTTP Request Splitting to Netty Query

Author: JLLeitschuh

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,78 @@
  * @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-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
+    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
+    vulnerableArgumentIndex = 2
+  }
+}
+
+private class InsecureDefaultHttpRequestClassInstantiation extends RequestSplittingInsecureNettyObjectCreation {
+  InsecureDefaultHttpRequestClassInstantiation() {
+    getConstructedType().hasQualifiedName("io.netty.handler.codec.http", "DefaultHttpRequest") and
+    vulnerableArgumentIndex = 3
   }
 }
 
-private class InsecureDefaultFullHttpResponseClassInstantiation extends InsecureNettyObjectCreation {
+private class InsecureDefaultFullHttpResponseClassInstantiation extends ResponseSplittingInsecureNettyObjectCreation {
   InsecureDefaultFullHttpResponseClassInstantiation() {
     getConstructedType().hasQualifiedName("io.netty.handler.codec.http", "DefaultFullHttpResponse") and
-    getArgument(3).(CompileTimeConstantExpr).getBooleanValue() = false
+    vulnerableArgumentIndex = [2, 3]
+  }
+}
+
+private class InsecureDefaultFullHttpRequestClassInstantiation extends RequestSplittingInsecureNettyObjectCreation {
+  InsecureDefaultFullHttpRequestClassInstantiation() {
+    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/smuggle 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 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. This 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>In the case of code calling 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 library 'netty' with HTTP request-splitting verification configurations.
+The second way will verify 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>.

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`