Fix XSS FPs when content type is safe

github · Jan 28, 2025 · 0ccf4ce · 0ccf4ce
1 parent 9f3572d
commit 0ccf4ce
Show file tree

Hide file tree

Showing 3 changed files with 108 additions and 4 deletions.
diff --git a/java/ql/lib/semmle/code/java/frameworks/Servlets.qll b/java/ql/lib/semmle/code/java/frameworks/Servlets.qll
@@ -315,6 +315,16 @@ class ResponseSetHeaderMethod extends Method {
   }
 }
 
+/**
+ * The method `setContentType` declared in `javax.servlet.http.HttpServletResponse`.
+ */
+class ResponseSetContentTypeMethod extends Method {
+  ResponseSetContentTypeMethod() {
+    this.getDeclaringType() instanceof ServletResponse and
+    this.hasName("setContentType")
+  }
+}
+
 /**
  * A class that has `javax.servlet.Servlet` as an ancestor.
  */

diff --git a/java/ql/lib/semmle/code/java/security/XSS.qll b/java/ql/lib/semmle/code/java/security/XSS.qll
@@ -92,9 +92,25 @@ private class WritingMethod extends Method {
 /** An output stream or writer that writes to a servlet, JSP or JSF response. */
 class XssVulnerableWriterSource extends MethodCall {
   XssVulnerableWriterSource() {
-    this.getMethod() instanceof ServletResponseGetWriterMethod
-    or
-    this.getMethod() instanceof ServletResponseGetOutputStreamMethod
+    (
+      this.getMethod() instanceof ServletResponseGetWriterMethod
+      or
+      this.getMethod() instanceof ServletResponseGetOutputStreamMethod
+    ) and
+    not exists(MethodCall mc, Expr contentType |
+      mc.getMethod() instanceof ResponseSetContentTypeMethod and
+      contentType = mc.getArgument(0)
+      or
+      (
+        mc.getMethod() instanceof ResponseAddHeaderMethod or
+        mc.getMethod() instanceof ResponseSetHeaderMethod
+      ) and
+      mc.getArgument(0).(CompileTimeConstantExpr).getStringValue().toLowerCase() = "content-type" and
+      contentType = mc.getArgument(1)
+    |
+      isXssSafeContentTypeString(contentType.(CompileTimeConstantExpr).getStringValue()) and
+      DataFlow::localExprFlow(mc.getQualifier(), this.getQualifier())
+    )
     or
     exists(Method m | m = this.getMethod() |
       m.hasQualifiedName("javax.servlet.jsp", "JspContext", "getOut")
@@ -106,6 +122,11 @@ class XssVulnerableWriterSource extends MethodCall {
   }
 }
 
+pragma[nomagic]
+private predicate isXssSafeContentTypeString(string s) {
+  s = any(CompileTimeConstantExpr cte).getStringValue() and isXssSafeContentType(s)
+}
+
 /**
  * A xss vulnerable writer source node.
  */

diff --git a/java/ql/test/query-tests/security/CWE-079/semmle/tests/XSS.java b/java/ql/test/query-tests/security/CWE-079/semmle/tests/XSS.java
@@ -12,7 +12,7 @@
 import javax.servlet.http.HttpServletResponse;
 
 public class XSS extends HttpServlet {
-	protected void doGet(HttpServletRequest request, HttpServletResponse response)
+	protected void doGet(HttpServletRequest request, HttpServletResponse response, boolean safeContentType, boolean getWriter, int setContentMethod)
 			throws ServletException, IOException {
 		// BAD: a request parameter is written directly to the Servlet response stream
 		response.getWriter()
@@ -38,6 +38,79 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response)
 
 		// GOOD: sanitizer
 		response.getOutputStream().write(hudson.Util.escape(request.getPathInfo()).getBytes()); // safe
+
+		if(safeContentType) {
+			if(getWriter) {
+				if(setContentMethod == 0) {
+					// GOOD: set content-type to something safe
+					response.setContentType("text/plain");
+					response.getWriter().print(request.getPathInfo());
+				}
+				else if(setContentMethod == 1) {
+					// GOOD: set content-type to something safe
+					response.setHeader("Content-Type", "text/plain");
+					response.getWriter().print(request.getPathInfo());
+				}
+				else {
+					// GOOD: set content-type to something safe
+					response.addHeader("Content-Type", "text/plain");
+					response.getWriter().print(request.getPathInfo());
+				}
+			}
+			else {
+				if(setContentMethod == 0) {
+					// GOOD: set content-type to something safe
+					response.setContentType("text/plain");
+					response.getOutputStream().write(request.getPathInfo().getBytes());
+				}
+				else if(setContentMethod == 1) {
+					// GOOD: set content-type to something safe
+					response.setHeader("Content-Type", "text/plain");
+					response.getOutputStream().write(request.getPathInfo().getBytes());
+				}
+				else {
+					// GOOD: set content-type to something safe
+					response.addHeader("Content-Type", "text/plain");
+					response.getOutputStream().write(request.getPathInfo().getBytes());
+				}
+			}
+		}
+		else {
+			if(getWriter) {
+				if(setContentMethod == 0) {
+					// BAD: set content-type to something that is not safe
+					response.setContentType("text/html");
+					response.getWriter().print(request.getPathInfo()); // $ xss
+				}
+				else if(setContentMethod == 1) {
+					// BAD: set content-type to something that is not safe
+					response.setHeader("Content-Type", "text/html");
+					response.getWriter().print(request.getPathInfo()); // $ xss
+				}
+				else {
+					// BAD: set content-type to something that is not safe
+					response.addHeader("Content-Type", "text/html");
+					response.getWriter().print(request.getPathInfo()); // $ xss
+				}
+			}
+			else {
+				if(setContentMethod == 0) {
+					// BAD: set content-type to something that is not safe
+					response.setContentType("text/html");
+					response.getOutputStream().write(request.getPathInfo().getBytes()); // $ xss
+				}
+				else if(setContentMethod == 1) {
+					// BAD: set content-type to something that is not safe
+					response.setHeader("Content-Type", "text/html");
+					response.getOutputStream().write(request.getPathInfo().getBytes()); // $ xss
+				}
+				else {
+					// BAD: set content-type to something that is not safe
+					response.addHeader("Content-Type", "text/html");
+					response.getOutputStream().write(request.getPathInfo().getBytes()); // $ xss
+				}
+			}
+		}
 	}
 
 	/**