Merge pull request #18607 from owen-mc/java/xss-content-type-sanitizer

Java: Add XSS Sanitizer for `HttpServletResponse.setContentType` with safe values

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.
  */

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.
  */

java/ql/src/change-notes/2025-01-28-fix-xss-content-type-safe.md

@@ -0,0 +1,4 @@
+---
+category: majorAnalysis
+---
+* Fixed false positive alerts in the java query "Cross-site scripting" (`java/xss`) when `javax.servlet.http.HttpServletResponse` is used with a content type which is not exploitable.

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

@@ -19,18 +19,18 @@ public class JaxXSS {
     if(!safeContentType) {
       if(chainDirectly) {
         if(contentTypeFirst)
-          return builder.type(MediaType.TEXT_HTML).entity(userControlled).build(); // $xss
+          return builder.type(MediaType.TEXT_HTML).entity(userControlled).build(); // $ xss
         else
-          return builder.entity(userControlled).type(MediaType.TEXT_HTML).build(); // $xss
+          return builder.entity(userControlled).type(MediaType.TEXT_HTML).build(); // $ xss
       }
       else {
         if(contentTypeFirst) {
           Response.ResponseBuilder builder2 = builder.type(MediaType.TEXT_HTML);
-          return builder2.entity(userControlled).build(); // $xss
+          return builder2.entity(userControlled).build(); // $ xss
         }
         else {
           Response.ResponseBuilder builder2 = builder.entity(userControlled);
-          return builder2.type(MediaType.TEXT_HTML).build(); // $xss
+          return builder2.type(MediaType.TEXT_HTML).build(); // $ xss
         }
       }
     }
@@ -105,39 +105,39 @@ public class JaxXSS {
     else {
       if(route == 0) {
         // via ok, as a string literal:
-        return Response.ok("text/html").entity(userControlled).build(); // $xss
+        return Response.ok("text/html").entity(userControlled).build(); // $ xss
       }
       else if(route == 1) {
         // via ok, as a string constant:
-        return Response.ok(MediaType.TEXT_HTML).entity(userControlled).build(); // $xss
+        return Response.ok(MediaType.TEXT_HTML).entity(userControlled).build(); // $ xss
       }
       else if(route == 2) {
         // via ok, as a MediaType constant:
-        return Response.ok(MediaType.TEXT_HTML_TYPE).entity(userControlled).build(); // $xss
+        return Response.ok(MediaType.TEXT_HTML_TYPE).entity(userControlled).build(); // $ xss
       }
       else if(route == 3) {
         // via ok, as a Variant, via constructor:
-        return Response.ok(new Variant(MediaType.TEXT_HTML_TYPE, "language", "encoding")).entity(userControlled).build(); // $xss
+        return Response.ok(new Variant(MediaType.TEXT_HTML_TYPE, "language", "encoding")).entity(userControlled).build(); // $ xss
       }
       else if(route == 4) {
         // via ok, as a Variant, via static method:
-        return Response.ok(Variant.mediaTypes(MediaType.TEXT_HTML_TYPE).build()).entity(userControlled).build(); // $xss
+        return Response.ok(Variant.mediaTypes(MediaType.TEXT_HTML_TYPE).build()).entity(userControlled).build(); // $ xss
       }
       else if(route == 5) {
         // via ok, as a Variant, via instance method:
-        return Response.ok(Variant.languages(Locale.UK).mediaTypes(MediaType.TEXT_HTML_TYPE).build()).entity(userControlled).build(); // $xss
+        return Response.ok(Variant.languages(Locale.UK).mediaTypes(MediaType.TEXT_HTML_TYPE).build()).entity(userControlled).build(); // $ xss
       }
       else if(route == 6) {
         // via builder variant, before entity:
-        return Response.ok().variant(new Variant(MediaType.TEXT_HTML_TYPE, "language", "encoding")).entity(userControlled).build(); // $xss
+        return Response.ok().variant(new Variant(MediaType.TEXT_HTML_TYPE, "language", "encoding")).entity(userControlled).build(); // $ xss
       }
       else if(route == 7) {
         // via builder variant, after entity:
-        return Response.ok().entity(userControlled).variant(new Variant(MediaType.TEXT_HTML_TYPE, "language", "encoding")).build(); // $xss
+        return Response.ok().entity(userControlled).variant(new Variant(MediaType.TEXT_HTML_TYPE, "language", "encoding")).build(); // $ xss
       }
       else if(route == 8) {
         // provide entity via ok, then content-type via builder:
-        return Response.ok(userControlled).type(MediaType.TEXT_HTML_TYPE).build(); // $xss
+        return Response.ok(userControlled).type(MediaType.TEXT_HTML_TYPE).build(); // $ xss
       }
     }
 
@@ -162,27 +162,27 @@ public class JaxXSS {
 
   @GET @Produces(MediaType.TEXT_HTML)
   public static Response methodContentTypeUnsafe(String userControlled) {
-    return Response.ok(userControlled).build(); // $xss
+    return Response.ok(userControlled).build(); // $ xss
   }
 
   @POST @Produces(MediaType.TEXT_HTML)
   public static Response methodContentTypeUnsafePost(String userControlled) {
-    return Response.ok(userControlled).build(); // $xss
+    return Response.ok(userControlled).build(); // $ xss
   }
 
   @GET @Produces("text/html")
   public static Response methodContentTypeUnsafeStringLiteral(String userControlled) {
-    return Response.ok(userControlled).build(); // $xss
+    return Response.ok(userControlled).build(); // $ xss
   }
 
   @GET @Produces({MediaType.TEXT_HTML, MediaType.APPLICATION_JSON})
   public static Response methodContentTypeMaybeSafe(String userControlled) {
-    return Response.ok(userControlled).build(); // $xss
+    return Response.ok(userControlled).build(); // $ xss
   }
 
   @GET @Produces(MediaType.APPLICATION_JSON)
   public static Response methodContentTypeSafeOverriddenWithUnsafe(String userControlled) {
-    return Response.ok().type(MediaType.TEXT_HTML).entity(userControlled).build(); // $xss
+    return Response.ok().type(MediaType.TEXT_HTML).entity(userControlled).build(); // $ xss
   }
 
   @GET @Produces(MediaType.TEXT_HTML)
@@ -205,12 +205,12 @@ public class JaxXSS {
 
     @GET @Produces({"text/html"})
     public Response overridesWithUnsafe(String userControlled) {
-      return Response.ok(userControlled).build(); // $xss
+      return Response.ok(userControlled).build(); // $ xss
     }
 
     @GET
     public Response overridesWithUnsafe2(String userControlled) {
-      return Response.ok().type(MediaType.TEXT_HTML).entity(userControlled).build(); // $xss
+      return Response.ok().type(MediaType.TEXT_HTML).entity(userControlled).build(); // $ xss
     }
   }
 
@@ -219,12 +219,12 @@ public class JaxXSS {
   public static class ClassContentTypeUnsafe {
     @GET
     public Response test(String userControlled) {
-      return Response.ok(userControlled).build(); // $xss
+      return Response.ok(userControlled).build(); // $ xss
     }
 
     @GET
     public String testDirectReturn(String userControlled) {
-      return userControlled; // $xss
+      return userControlled; // $ xss
     }
 
     @GET @Produces({"application/json"})
@@ -240,12 +240,12 @@ public class JaxXSS {
 
   @GET
   public static Response entityWithNoMediaType(String userControlled) {
-    return Response.ok(userControlled).build(); // $xss
+    return Response.ok(userControlled).build(); // $ xss
   }
 
   @GET
   public static String stringWithNoMediaType(String userControlled) {
-    return userControlled; // $xss
+    return userControlled; // $ xss
   }
 
-}
+}

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

@@ -26,7 +26,7 @@ public class JsfXSS extends Renderer
         writer.write("(function(){");
         writer.write("dswh.init('" + windowId + "','"
                 + "......" + "',"
-                + -1 + ",{"); // $xss
+                + -1 + ",{"); // $ xss
         writer.write("});");
         writer.write("})();");
         writer.write("</script>");
@@ -57,13 +57,13 @@ public class JsfXSS extends Renderer
     {
         ExternalContext ec = facesContext.getExternalContext();
         ResponseWriter writer = facesContext.getResponseWriter();
-        writer.write(ec.getRequestParameterMap().keySet().iterator().next()); // $xss
-        writer.write(ec.getRequestParameterNames().next()); // $xss
-        writer.write(ec.getRequestParameterValuesMap().get("someKey")[0]); // $xss
-        writer.write(ec.getRequestParameterValuesMap().keySet().iterator().next()); // $xss
-        writer.write(ec.getRequestPathInfo()); // $xss
-        writer.write(((Cookie)ec.getRequestCookieMap().get("someKey")).getName()); // $xss
-        writer.write(ec.getRequestHeaderMap().get("someKey")); // $xss
-        writer.write(ec.getRequestHeaderValuesMap().get("someKey")[0]); // $xss
+        writer.write(ec.getRequestParameterMap().keySet().iterator().next()); // $ xss
+        writer.write(ec.getRequestParameterNames().next()); // $ xss
+        writer.write(ec.getRequestParameterValuesMap().get("someKey")[0]); // $ xss
+        writer.write(ec.getRequestParameterValuesMap().keySet().iterator().next()); // $ xss
+        writer.write(ec.getRequestPathInfo()); // $ xss
+        writer.write(((Cookie)ec.getRequestCookieMap().get("someKey")).getName()); // $ xss
+        writer.write(ec.getRequestHeaderMap().get("someKey")); // $ xss
+        writer.write(ec.getRequestHeaderValuesMap().get("someKey")[0]); // $ xss
     }
 }

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

@@ -6,7 +6,7 @@ import android.webkit.WebSettings;
 public class SetJavascriptEnabled {
     public static void configureWebViewUnsafe(WebView view) {
         WebSettings settings = view.getSettings();
-        settings.setJavaScriptEnabled(true); // $javascriptEnabled
+        settings.setJavaScriptEnabled(true); // $ javascriptEnabled
     }
 
     public static void configureWebViewSafe(WebView view) {

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

@@ -17,13 +17,13 @@ public class SpringXSS {
 
     ResponseEntity.BodyBuilder builder = ResponseEntity.ok();
 
-    if(safeContentType) {
+    if(!safeContentType) {
       if(chainDirectly) {
-        return builder.contentType(MediaType.TEXT_HTML).body(userControlled); // $xss
+        return builder.contentType(MediaType.TEXT_HTML).body(userControlled); // $ xss
       }
       else {
         ResponseEntity.BodyBuilder builder2 = builder.contentType(MediaType.TEXT_HTML);
-        return builder2.body(userControlled); // $xss
+        return builder2.body(userControlled); // $ xss
       }
     }
     else {
@@ -60,22 +60,22 @@ public class SpringXSS {
 
   @GetMapping(value = "/xyz", produces = MediaType.TEXT_HTML_VALUE)
   public static ResponseEntity<String> methodContentTypeUnsafe(String userControlled) {
-    return ResponseEntity.ok(userControlled); // $xss
+    return ResponseEntity.ok(userControlled); // $ xss
   }
 
   @GetMapping(value = "/xyz", produces = "text/html")
   public static ResponseEntity<String> methodContentTypeUnsafeStringLiteral(String userControlled) {
-    return ResponseEntity.ok(userControlled); // $xss
+    return ResponseEntity.ok(userControlled); // $ xss
   }
 
   @GetMapping(value = "/xyz", produces = {MediaType.TEXT_HTML_VALUE, MediaType.APPLICATION_JSON_VALUE})
   public static ResponseEntity<String> methodContentTypeMaybeSafe(String userControlled) {
-    return ResponseEntity.ok(userControlled); // $xss
+    return ResponseEntity.ok(userControlled); // $ xss
   }
 
   @GetMapping(value = "/xyz", produces = MediaType.APPLICATION_JSON_VALUE)
   public static ResponseEntity<String> methodContentTypeSafeOverriddenWithUnsafe(String userControlled) {
-    return ResponseEntity.ok().contentType(MediaType.TEXT_HTML).body(userControlled); // $xss
+    return ResponseEntity.ok().contentType(MediaType.TEXT_HTML).body(userControlled); // $ xss
   }
 
   @GetMapping(value = "/xyz", produces = MediaType.TEXT_HTML_VALUE)
@@ -88,13 +88,13 @@ public class SpringXSS {
     // Also try out some alternative constructors for the ResponseEntity:
     switch(constructionMethod) {
       case 0:
-      return ResponseEntity.ok(userControlled); // $xss
+      return ResponseEntity.ok(userControlled); // $ xss
       case 1:
-      return ResponseEntity.of(Optional.of(userControlled)); // $xss
+      return ResponseEntity.of(Optional.of(userControlled)); // $ xss
       case 2:
-      return ResponseEntity.ok().body(userControlled); // $xss
+      return ResponseEntity.ok().body(userControlled); // $ xss
       case 3:
-      return new ResponseEntity<String>(userControlled, HttpStatus.OK); // $xss
+      return new ResponseEntity<String>(userControlled, HttpStatus.OK); // $ xss
       default:
       return null;
     }
@@ -115,12 +115,12 @@ public class SpringXSS {
 
     @GetMapping(value = "/xyz", produces = {"text/html"})
     public ResponseEntity<String> overridesWithUnsafe(String userControlled) {
-      return ResponseEntity.ok(userControlled); // $xss
+      return ResponseEntity.ok(userControlled); // $ xss
     }
 
     @GetMapping(value = "/abc")
     public ResponseEntity<String> overridesWithUnsafe2(String userControlled) {
-      return ResponseEntity.ok().contentType(MediaType.TEXT_HTML).body(userControlled); // $xss
+      return ResponseEntity.ok().contentType(MediaType.TEXT_HTML).body(userControlled); // $ xss
     }
   }
 
@@ -129,12 +129,12 @@ public class SpringXSS {
   private static class ClassContentTypeUnsafe {
     @GetMapping(value = "/abc")
     public ResponseEntity<String> test(String userControlled) {
-      return ResponseEntity.ok(userControlled); // $xss
+      return ResponseEntity.ok(userControlled); // $ xss
     }
 
     @GetMapping(value = "/abc")
     public String testDirectReturn(String userControlled) {
-      return userControlled; // $xss
+      return userControlled; // $ xss
     }
 
     @GetMapping(value = "/xyz", produces = {"application/json"})
@@ -150,12 +150,12 @@ public class SpringXSS {
 
   @GetMapping(value = "/abc")
   public static ResponseEntity<String> entityWithNoMediaType(String userControlled) {
-    return ResponseEntity.ok(userControlled); // $xss
+    return ResponseEntity.ok(userControlled); // $ xss
   }
 
   @GetMapping(value = "/abc")
   public static String stringWithNoMediaType(String userControlled) {
-    return userControlled; // $xss
+    return userControlled; // $ xss
   }
 
   @GetMapping(value = "/abc")

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

@@ -12,11 +12,11 @@ import javax.servlet.http.HttpServletRequest;
 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()
-				.print("The page \"" + request.getParameter("page") + "\" was not found."); // $xss
+				.print("The page \"" + request.getParameter("page") + "\" was not found."); // $ xss
 
 		// GOOD: servlet API encodes the error message HTML for the HTML context
 		response.sendError(HttpServletResponse.SC_NOT_FOUND,
@@ -31,13 +31,86 @@ public class XSS extends HttpServlet {
 				"The page \"" + capitalizeName(request.getParameter("page")) + "\" was not found.");
 
 		// BAD: outputting the path of the resource
-		response.getWriter().print("The path section of the URL was " + request.getPathInfo()); // $xss
+		response.getWriter().print("The path section of the URL was " + request.getPathInfo()); // $ xss
 
 		// BAD: typical XSS, this time written to an OutputStream instead of a Writer
-		response.getOutputStream().write(request.getPathInfo().getBytes()); // $xss
+		response.getOutputStream().write(request.getPathInfo().getBytes()); // $ xss
 
 		// 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
+				}
+			}
+		}
 	}
 
 	/**