Merge pull request #5940 from pwntester/main

Remove XSS sink for Java

java/change-notes/2021-05-28-remove-senderror-xss-sink.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The query "Cross-site scripting" (`java/xss`) has been improved to report fewer false positives by removing the `javax.servlet.http.HttpServletResponse.sendError` sink since Servlet API implementations generally already escape the error message, preventing script injection.

java/documentation/library-coverage/flow-model-coverage.csv

@@ -1,42 +1,42 @@
-package,sink,source,summary,sink:bean-validation,sink:create-file,sink:header-splitting,sink:ldap,sink:open-url,sink:set-hostname-verifier,sink:url-open-stream,sink:xpath,sink:xss,source:remote,summary:taint,summary:value
-android.util,,16,,,,,,,,,,,16,,
-android.webkit,3,2,,,,,,,,,,3,2,,
-com.esotericsoftware.kryo.io,,,1,,,,,,,,,,,1,
-com.esotericsoftware.kryo5.io,,,1,,,,,,,,,,,1,
-com.fasterxml.jackson.databind,,,2,,,,,,,,,,,2,
-com.google.common.base,,,28,,,,,,,,,,,22,6
-com.google.common.io,6,,69,,,,,,,6,,,,68,1
-com.unboundid.ldap.sdk,17,,,,,,17,,,,,,,,
-java.beans,,,1,,,,,,,,,,,1,
-java.io,3,,20,,3,,,,,,,,,20,
-java.lang,,,1,,,,,,,,,,,1,
-java.net,2,3,4,,,,,2,,,,,3,4,
-java.nio,10,,2,,10,,,,,,,,,2,
-java.util,,,13,,,,,,,,,,,13,
-javax.naming.directory,1,,,,,,1,,,,,,,,
-javax.net.ssl,2,,,,,,,,2,,,,,,
-javax.servlet,4,21,2,,,3,,,,,,1,21,2,
-javax.validation,1,1,,1,,,,,,,,,1,,
-javax.ws.rs.core,1,,,,,1,,,,,,,,,
-javax.xml.transform.sax,,,4,,,,,,,,,,,4,
-javax.xml.transform.stream,,,2,,,,,,,,,,,2,
-javax.xml.xpath,3,,,,,,,,,,3,,,,
-org.apache.commons.codec,,,2,,,,,,,,,,,2,
-org.apache.commons.io,,,22,,,,,,,,,,,22,
-org.apache.commons.lang3,,,313,,,,,,,,,,,299,14
-org.apache.commons.text,,,203,,,,,,,,,,,203,
-org.apache.directory.ldap.client.api,1,,,,,,1,,,,,,,,
-org.apache.hc.core5.function,,,1,,,,,,,,,,,1,
-org.apache.hc.core5.http,1,2,39,,,,,,,,,1,2,39,
-org.apache.hc.core5.net,,,2,,,,,,,,,,,2,
-org.apache.hc.core5.util,,,22,,,,,,,,,,,18,4
-org.apache.http,2,3,66,,,,,,,,,2,3,59,7
-org.dom4j,20,,,,,,,,,,20,,,,
-org.springframework.ldap.core,14,,,,,,14,,,,,,,,
-org.springframework.security.web.savedrequest,,6,,,,,,,,,,,6,,
-org.springframework.web.client,,3,,,,,,,,,,,3,,
-org.springframework.web.context.request,,8,,,,,,,,,,,8,,
-org.springframework.web.multipart,,12,,,,,,,,,,,12,,
-org.xml.sax,,,1,,,,,,,,,,,1,
-org.xmlpull.v1,,3,,,,,,,,,,,3,,
-play.mvc,,4,,,,,,,,,,,4,,
+package,sink,source,summary,sink:bean-validation,sink:create-file,sink:header-splitting,sink:information-leak,sink:ldap,sink:open-url,sink:set-hostname-verifier,sink:url-open-stream,sink:xpath,sink:xss,source:remote,summary:taint,summary:value
+android.util,,16,,,,,,,,,,,,16,,
+android.webkit,3,2,,,,,,,,,,,3,2,,
+com.esotericsoftware.kryo.io,,,1,,,,,,,,,,,,1,
+com.esotericsoftware.kryo5.io,,,1,,,,,,,,,,,,1,
+com.fasterxml.jackson.databind,,,2,,,,,,,,,,,,2,
+com.google.common.base,,,28,,,,,,,,,,,,22,6
+com.google.common.io,6,,69,,,,,,,,6,,,,68,1
+com.unboundid.ldap.sdk,17,,,,,,,17,,,,,,,,
+java.beans,,,1,,,,,,,,,,,,1,
+java.io,3,,20,,3,,,,,,,,,,20,
+java.lang,,,1,,,,,,,,,,,,1,
+java.net,2,3,4,,,,,,2,,,,,3,4,
+java.nio,10,,2,,10,,,,,,,,,,2,
+java.util,,,13,,,,,,,,,,,,13,
+javax.naming.directory,1,,,,,,,1,,,,,,,,
+javax.net.ssl,2,,,,,,,,,2,,,,,,
+javax.servlet,4,21,2,,,3,1,,,,,,,21,2,
+javax.validation,1,1,,1,,,,,,,,,,1,,
+javax.ws.rs.core,1,,,,,1,,,,,,,,,,
+javax.xml.transform.sax,,,4,,,,,,,,,,,,4,
+javax.xml.transform.stream,,,2,,,,,,,,,,,,2,
+javax.xml.xpath,3,,,,,,,,,,,3,,,,
+org.apache.commons.codec,,,2,,,,,,,,,,,,2,
+org.apache.commons.io,,,22,,,,,,,,,,,,22,
+org.apache.commons.lang3,,,313,,,,,,,,,,,,299,14
+org.apache.commons.text,,,203,,,,,,,,,,,,203,
+org.apache.directory.ldap.client.api,1,,,,,,,1,,,,,,,,
+org.apache.hc.core5.function,,,1,,,,,,,,,,,,1,
+org.apache.hc.core5.http,1,2,39,,,,,,,,,,1,2,39,
+org.apache.hc.core5.net,,,2,,,,,,,,,,,,2,
+org.apache.hc.core5.util,,,22,,,,,,,,,,,,18,4
+org.apache.http,2,3,66,,,,,,,,,,2,3,59,7
+org.dom4j,20,,,,,,,,,,,20,,,,
+org.springframework.ldap.core,14,,,,,,,14,,,,,,,,
+org.springframework.security.web.savedrequest,,6,,,,,,,,,,,,6,,
+org.springframework.web.client,,3,,,,,,,,,,,,3,,
+org.springframework.web.context.request,,8,,,,,,,,,,,,8,,
+org.springframework.web.multipart,,12,,,,,,,,,,,,12,,
+org.xml.sax,,,1,,,,,,,,,,,,1,
+org.xmlpull.v1,,3,,,,,,,,,,,,3,,
+play.mvc,,4,,,,,,,,,,,,4,,

java/documentation/library-coverage/flow-model-coverage.rst

@@ -12,8 +12,8 @@ Java framework & library support
    `Apache Commons IO <https://commons.apache.org/proper/commons-io/>`_,``org.apache.commons.io``,,22,,,,,,,,
    Google,``com.google.common.*``,,97,6,,6,,,,,
    Java Standard Library,``java.*``,3,41,15,13,,,,,,2
-   Java extensions,``javax.*``,22,8,12,,,1,,1,1,
+   Java extensions,``javax.*``,22,8,12,,,,,1,1,
    `Spring <https://spring.io/>`_,``org.springframework.*``,29,,14,,,,,14,,
    Others,"``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.databind``, ``com.unboundid.ldap.sdk``, ``org.dom4j``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``",7,5,37,,,,,17,,
-   Totals,,84,821,91,13,6,7,,33,1,2
+   Totals,,84,821,91,13,6,6,,33,1,2
 

java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql

@@ -15,7 +15,7 @@
 import java
 import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.dataflow.TaintTracking
-import semmle.code.java.security.XSS
+import semmle.code.java.security.InformationLeak
 
 /**
  * One of the `printStackTrace()` overloads on `Throwable`.
@@ -83,14 +83,14 @@ predicate stackTraceExpr(Expr exception, MethodAccess stackTraceString) {
   )
 }
 
-class StackTraceStringToXssSinkFlowConfig extends TaintTracking::Configuration {
-  StackTraceStringToXssSinkFlowConfig() {
-    this = "StackTraceExposure::StackTraceStringToXssSinkFlowConfig"
+class StackTraceStringToHttpResponseSinkFlowConfig extends TaintTracking::Configuration {
+  StackTraceStringToHttpResponseSinkFlowConfig() {
+    this = "StackTraceExposure::StackTraceStringToHttpResponseSinkFlowConfig"
   }
 
   override predicate isSource(DataFlow::Node src) { stackTraceExpr(_, src.asExpr()) }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof XssSink }
+  override predicate isSink(DataFlow::Node sink) { sink instanceof InformationLeakSink }
 }
 
 /**
@@ -105,8 +105,8 @@ predicate printsStackExternally(MethodAccess call, Expr stackTrace) {
 /**
  * A stringified stack trace flows to an external sink.
  */
-predicate stringifiedStackFlowsExternally(XssSink externalExpr, Expr stackTrace) {
-  exists(MethodAccess stackTraceString, StackTraceStringToXssSinkFlowConfig conf |
+predicate stringifiedStackFlowsExternally(DataFlow::Node externalExpr, Expr stackTrace) {
+  exists(MethodAccess stackTraceString, StackTraceStringToHttpResponseSinkFlowConfig conf |
     stackTraceExpr(stackTrace, stackTraceString) and
     conf.hasFlow(DataFlow::exprNode(stackTraceString), externalExpr)
   )
@@ -123,21 +123,21 @@ class GetMessageFlowSource extends MethodAccess {
   }
 }
 
-class GetMessageFlowSourceToXssSinkFlowConfig extends TaintTracking::Configuration {
-  GetMessageFlowSourceToXssSinkFlowConfig() {
-    this = "StackTraceExposure::GetMessageFlowSourceToXssSinkFlowConfig"
+class GetMessageFlowSourceToHttpResponseSinkFlowConfig extends TaintTracking::Configuration {
+  GetMessageFlowSourceToHttpResponseSinkFlowConfig() {
+    this = "StackTraceExposure::GetMessageFlowSourceToHttpResponseSinkFlowConfig"
   }
 
   override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof GetMessageFlowSource }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof XssSink }
+  override predicate isSink(DataFlow::Node sink) { sink instanceof InformationLeakSink }
 }
 
 /**
  * A call to `getMessage()` that then flows to a servlet response.
  */
-predicate getMessageFlowsExternally(XssSink externalExpr, GetMessageFlowSource getMessage) {
-  any(GetMessageFlowSourceToXssSinkFlowConfig conf)
+predicate getMessageFlowsExternally(DataFlow::Node externalExpr, GetMessageFlowSource getMessage) {
+  any(GetMessageFlowSourceToHttpResponseSinkFlowConfig conf)
       .hasFlow(DataFlow::exprNode(getMessage), externalExpr)
 }
 

java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll

@@ -80,6 +80,7 @@ private module Frameworks {
   private import semmle.code.java.frameworks.guava.Guava
   private import semmle.code.java.frameworks.jackson.JacksonSerializability
   private import semmle.code.java.security.ResponseSplitting
+  private import semmle.code.java.security.InformationLeak
   private import semmle.code.java.security.XSS
   private import semmle.code.java.security.LdapInjection
   private import semmle.code.java.security.XPath

java/ql/src/semmle/code/java/security/InformationLeak.qll

@@ -0,0 +1,27 @@
+/** Provides classes to reason about System Information Leak vulnerabilities. */
+
+import java
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.dataflow.ExternalFlow
+import semmle.code.java.security.XSS
+
+/** CSV sink models representing methods not susceptible to XSS but outputing to an HTTP response body. */
+private class DefaultInformationLeakSinkModel extends SinkModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        "javax.servlet.http;HttpServletResponse;false;sendError;(int,String);;Argument[1];information-leak"
+      ]
+  }
+}
+
+/** A sink that represent a method that outputs data to an HTTP response. */
+abstract class InformationLeakSink extends DataFlow::Node { }
+
+/** A default sink representing methods outputing data to an HTTP response. */
+private class DefaultInformationLeakSink extends InformationLeakSink {
+  DefaultInformationLeakSink() {
+    sinkNode(this, "information-leak") or
+    this instanceof XssSink
+  }
+}

java/ql/src/semmle/code/java/security/XSS.qll

@@ -34,7 +34,6 @@ private class DefaultXssSinkModel extends SinkModelCsv {
   override predicate row(string row) {
     row =
       [
-        "javax.servlet.http;HttpServletResponse;false;sendError;(int,String);;Argument[1];xss",
         "android.webkit;WebView;false;loadData;;;Argument[0];xss",
         "android.webkit;WebView;false;loadUrl;;;Argument[0];xss",
         "android.webkit;WebView;false;loadDataWithBaseURL;;;Argument[1];xss"

java/ql/test/query-tests/security/CWE-079/semmle/tests/XSS.expected

@@ -1,19 +1,15 @@
 edges
 | XSS.java:23:21:23:48 | getParameter(...) : String | XSS.java:23:5:23:70 | ... + ... |
-| XSS.java:27:21:27:48 | getParameter(...) : String | XSS.java:27:5:27:70 | ... + ... |
 | XSS.java:38:67:38:87 | getPathInfo(...) : String | XSS.java:38:30:38:87 | ... + ... |
 | XSS.java:41:36:41:56 | getPathInfo(...) : String | XSS.java:41:36:41:67 | getBytes(...) |
 nodes
 | XSS.java:23:5:23:70 | ... + ... | semmle.label | ... + ... |
 | XSS.java:23:21:23:48 | getParameter(...) : String | semmle.label | getParameter(...) : String |
-| XSS.java:27:5:27:70 | ... + ... | semmle.label | ... + ... |
-| XSS.java:27:21:27:48 | getParameter(...) : String | semmle.label | getParameter(...) : String |
 | XSS.java:38:30:38:87 | ... + ... | semmle.label | ... + ... |
 | XSS.java:38:67:38:87 | getPathInfo(...) : String | semmle.label | getPathInfo(...) : String |
 | XSS.java:41:36:41:56 | getPathInfo(...) : String | semmle.label | getPathInfo(...) : String |
 | XSS.java:41:36:41:67 | getBytes(...) | semmle.label | getBytes(...) |
 #select
 | XSS.java:23:5:23:70 | ... + ... | XSS.java:23:21:23:48 | getParameter(...) : String | XSS.java:23:5:23:70 | ... + ... | Cross-site scripting vulnerability due to $@. | XSS.java:23:21:23:48 | getParameter(...) | user-provided value |
-| XSS.java:27:5:27:70 | ... + ... | XSS.java:27:21:27:48 | getParameter(...) : String | XSS.java:27:5:27:70 | ... + ... | Cross-site scripting vulnerability due to $@. | XSS.java:27:21:27:48 | getParameter(...) | user-provided value |
 | XSS.java:38:30:38:87 | ... + ... | XSS.java:38:67:38:87 | getPathInfo(...) : String | XSS.java:38:30:38:87 | ... + ... | Cross-site scripting vulnerability due to $@. | XSS.java:38:67:38:87 | getPathInfo(...) | user-provided value |
 | XSS.java:41:36:41:67 | getBytes(...) | XSS.java:41:36:41:56 | getPathInfo(...) : String | XSS.java:41:36:41:67 | getBytes(...) | Cross-site scripting vulnerability due to $@. | XSS.java:41:36:41:56 | getPathInfo(...) | user-provided value |

java/ql/test/query-tests/security/CWE-079/semmle/tests/XSS.java

@@ -22,7 +22,7 @@ public class XSS extends HttpServlet {
 		response.getWriter().print(
 				"The page \"" + request.getParameter("page") + "\" was not found.");
 
-		// BAD: a request parameter is written directly to an error response page
+		// GOOD: servlet API encodes the error message HTML for the HTML context
 		response.sendError(HttpServletResponse.SC_NOT_FOUND,
 				"The page \"" + request.getParameter("page") + "\" was not found.");
 
@@ -30,7 +30,7 @@ public class XSS extends HttpServlet {
 		response.sendError(HttpServletResponse.SC_NOT_FOUND,
 				"The page \"" + encodeForHtml(request.getParameter("page")) + "\" was not found.");
 		
-		// FALSE NEGATIVE: passed through function that is not a secure check
+		// GOOD: servlet API encodes the error message HTML for the HTML context
 		response.sendError(HttpServletResponse.SC_NOT_FOUND,
 				"The page \"" + capitalizeName(request.getParameter("page")) + "\" was not found.");