Spring HTTP: mark explicitly content-typed body calls as sinks

Previously only the return from the request-handler method constituted a sink, and was filtered by the Produces annotation if any, even though a BodyBuilder could explicitly override.

These sinks are also marked as out-barriers to avoid duplicate paths when the Produces annotation is in agreement.

Author: smowton

java/ql/lib/semmle/code/java/frameworks/spring/SpringHttp.qll

@@ -266,24 +266,32 @@ private predicate isXssSafeContentTypeExpr(Expr e) {
   XSS::isXssSafeContentType(getSpringConstantContentType(e))
 }
 
-private DataFlow::Node getASanitizedBodyBuilder() {
+private DataFlow::Node getABodyBuilderWithExplicitContentType(Expr contentType) {
   result.asExpr() =
     any(MethodAccess ma |
       ma.getCallee()
           .hasQualifiedName("org.springframework.http", "ResponseEntity<>$BodyBuilder",
             "contentType") and
-      isXssSafeContentTypeExpr(ma.getArgument(0))
+      contentType = ma.getArgument(0)
     )
   or
   result.asExpr() =
     any(MethodAccess ma |
-      ma.getQualifier() = getASanitizedBodyBuilder().asExpr() and
+      ma.getQualifier() = getABodyBuilderWithExplicitContentType(contentType).asExpr() and
       ma.getType()
           .(RefType)
           .hasQualifiedName("org.springframework.http", "ResponseEntity<>$BodyBuilder")
     )
   or
-  DataFlow::localFlow(getASanitizedBodyBuilder(), result)
+  DataFlow::localFlow(getABodyBuilderWithExplicitContentType(contentType), result)
+}
+
+private DataFlow::Node getASanitizedBodyBuilder() {
+  result = getABodyBuilderWithExplicitContentType(any(Expr e | isXssSafeContentTypeExpr(e)))
+}
+
+private DataFlow::Node getAVulnerableBodyBuilder() {
+  result = getABodyBuilderWithExplicitContentType(any(Expr e | isXssVulnerableContentTypeExpr(e)))
 }
 
 private class SanitizedBodyCall extends XSS::XssSanitizer {
@@ -295,3 +303,21 @@ private class SanitizedBodyCall extends XSS::XssSanitizer {
       ).getArgument(0)
   }
 }
+
+/**
+ * Mark BodyBuilder.body calls with an explicitly vulnerable Content-Type as themselves sinks,
+ * as the eventual return site from a RequestHandler may have a benign @Produces annotation that
+ * would otherwise sanitise the result.
+ *
+ * Note these are SinkBarriers so that a return from a RequestHandlerMethod is not also flagged
+ * for the same path.
+ */
+private class ExplicitlyVulnerableBodyArgument extends XSS::XssSinkBarrier {
+  ExplicitlyVulnerableBodyArgument() {
+    this.asExpr() =
+      any(MethodAccess ma |
+        ma.getQualifier() = getAVulnerableBodyBuilder().asExpr() and
+        ma.getCallee().hasName("body")
+      ).getArgument(0)
+  }
+}

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

@@ -75,7 +75,7 @@ public static ResponseEntity<String> methodContentTypeMaybeSafe(String userContr
 
   @GetMapping(value = "/xyz", produces = MediaType.APPLICATION_JSON_VALUE)
   public static ResponseEntity<String> methodContentTypeSafeOverriddenWithUnsafe(String userControlled) {
-    return ResponseEntity.ok().contentType(MediaType.TEXT_HTML).body(userControlled); // $MISSING: xss
+    return ResponseEntity.ok().contentType(MediaType.TEXT_HTML).body(userControlled); // $xss
   }
 
   @GetMapping(value = "/xyz", produces = MediaType.TEXT_HTML_VALUE)