Merge pull request github#6136 from smowton/smowton/admin/spring-xss-content-type-sensitivity

Spring HTTP: improve content-type sensitivity

Author: aschackmull

java/change-notes/2021-08-03-spring-content-types.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The XSS query now accounts for more ways to set the content-type of an entity served via a Spring HTTP endpoint. This may flag more cases where an XSS-vulnerable content-type is set, and exclude more cases where a non-vulnerable content-type such as `application/json` is set.

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

@@ -100,30 +100,55 @@ class SpringResponseBodyAnnotationType extends AnnotationType {
   }
 }
 
+private class SpringRequestMappingAnnotation extends Annotation {
+  SpringRequestMappingAnnotation() { this.getType() instanceof SpringRequestMappingAnnotationType }
+}
+
+private Expr getProducesExpr(RefType rt) {
+  result = rt.getAnAnnotation().(SpringRequestMappingAnnotation).getValue("produces")
+  or
+  rt.getAnAnnotation().(SpringRequestMappingAnnotation).getValue("produces").(ArrayInit).getSize() =
+    0 and
+  result = getProducesExpr(rt.getASupertype())
+}
+
 /**
  * A method on a Spring controller that is executed in response to a web request.
  */
 class SpringRequestMappingMethod extends SpringControllerMethod {
-  Annotation requestMappingAnnotation;
+  SpringRequestMappingAnnotation requestMappingAnnotation;
 
   SpringRequestMappingMethod() {
     // Any method that declares the @RequestMapping annotation, or overrides a method that declares
     // the annotation. We have to do this explicit check because the @RequestMapping annotation is
     // not declared with @Inherited.
     exists(Method superMethod |
       this.overrides*(superMethod) and
-      requestMappingAnnotation = superMethod.getAnAnnotation() and
-      requestMappingAnnotation.getType() instanceof SpringRequestMappingAnnotationType
+      requestMappingAnnotation = superMethod.getAnAnnotation()
     )
   }
 
   /** Gets a request mapping parameter. */
   SpringRequestMappingParameter getARequestParameter() { result = getAParameter() }
 
-  /** Gets the "produces" @RequestMapping annotation value, if present. */
+  /** Gets the "produces" @RequestMapping annotation value, if present. If an array is specified, gets the array. */
+  Expr getProducesExpr() {
+    result = requestMappingAnnotation.getValue("produces")
+    or
+    requestMappingAnnotation.getValue("produces").(ArrayInit).getSize() = 0 and
+    result = getProducesExpr(this.getDeclaringType())
+  }
+
+  /** Gets a "produces" @RequestMapping annotation value. If an array is specified, gets a member of the array. */
+  Expr getAProducesExpr() {
+    result = this.getProducesExpr() and not result instanceof ArrayInit
+    or
+    result = this.getProducesExpr().(ArrayInit).getAnInit()
+  }
+
+  /** Gets the "produces" @RequestMapping annotation value, if present and a string constant. */
   string getProduces() {
-    result =
-      requestMappingAnnotation.getValue("produces").(CompileTimeConstantExpr).getStringValue()
+    result = this.getProducesExpr().(CompileTimeConstantExpr).getStringValue()
   }
 
   /** Gets the "value" @RequestMapping annotation value, if present. */

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

@@ -5,6 +5,9 @@
 
 import java
 private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.dataflow.DataFlow
+private import semmle.code.java.frameworks.spring.SpringController
+private import semmle.code.java.security.XSS as XSS
 
 /** The class `org.springframework.http.HttpEntity` or an instantiation of it. */
 class SpringHttpEntity extends Class {
@@ -140,3 +143,178 @@ private class SpringHttpFlowStep extends SummaryModelCsv {
       ]
   }
 }
+
+private predicate specifiesContentType(SpringRequestMappingMethod method) {
+  exists(method.getAProducesExpr())
+}
+
+private class SpringXssSink extends XSS::XssSink {
+  SpringXssSink() {
+    exists(SpringRequestMappingMethod requestMappingMethod, ReturnStmt rs |
+      requestMappingMethod = rs.getEnclosingCallable() and
+      this.asExpr() = rs.getResult() and
+      (
+        not specifiesContentType(requestMappingMethod) or
+        isXssVulnerableContentTypeExpr(requestMappingMethod.getAProducesExpr())
+      )
+    |
+      // If a Spring request mapping method is either annotated with @ResponseBody (or equivalent),
+      // or returns a HttpEntity or sub-type, then the return value of the method is converted into
+      // a HTTP reponse using a HttpMessageConverter implementation. The implementation is chosen
+      // based on the return type of the method, and the Accept header of the request.
+      //
+      // By default, the only message converter which produces a response which is vulnerable to
+      // XSS is the StringHttpMessageConverter, which "Accept"s all text/* content types, including
+      // text/html. Therefore, if a browser request includes "text/html" in the "Accept" header,
+      // any String returned will be converted into a text/html response.
+      requestMappingMethod.isResponseBody() and
+      requestMappingMethod.getReturnType() instanceof TypeString
+      or
+      exists(Type returnType |
+        // A return type of HttpEntity<T> or ResponseEntity<T> represents an HTTP response with both
+        // a body and a set of headers. The body is subject to the same HttpMessageConverter
+        // process as above.
+        returnType = requestMappingMethod.getReturnType() and
+        (
+          returnType instanceof SpringHttpEntity
+          or
+          returnType instanceof SpringResponseEntity
+        )
+      |
+        // The type argument, representing the type of the body, is type String
+        returnType.(ParameterizedClass).getTypeArgument(0) instanceof TypeString
+        or
+        // Return type is a Raw class, which means no static type information on the body. In this
+        // case we will still treat this as an XSS sink, but rely on our taint flow steps for
+        // HttpEntity/ResponseEntity to only pass taint into those instances if the body type was
+        // String.
+        returnType instanceof RawClass
+      )
+    )
+  }
+}
+
+private string getSpringConstantContentType(FieldAccess e) {
+  e.getQualifier().getType().(RefType).hasQualifiedName("org.springframework.http", "MediaType") and
+  exists(string fieldName | e.getField().hasName(fieldName) |
+    fieldName = "APPLICATION_ATOM_XML" + ["", "_VALUE"] and result = "application/atom+xml"
+    or
+    fieldName = "APPLICATION_CBOR" + ["", "_VALUE"] and result = "application/cbor"
+    or
+    fieldName = "APPLICATION_FORM_URLENCODED" + ["", "_VALUE"] and
+    result = "application/x-www-form-urlencoded"
+    or
+    fieldName = "APPLICATION_JSON" + ["", "_VALUE"] and result = "application/json"
+    or
+    fieldName = "APPLICATION_JSON_UTF8" + ["", "_VALUE"] and
+    result = "application/json;charset=UTF-8"
+    or
+    fieldName = "APPLICATION_NDJSON" + ["", "_VALUE"] and result = "application/x-ndjson"
+    or
+    fieldName = "APPLICATION_OCTET_STREAM" + ["", "_VALUE"] and result = "application/octet-stream"
+    or
+    fieldName = "APPLICATION_PDF" + ["", "_VALUE"] and result = "application/pdf"
+    or
+    fieldName = "APPLICATION_PROBLEM_JSON" + ["", "_VALUE"] and result = "application/problem+json"
+    or
+    fieldName = "APPLICATION_PROBLEM_JSON_UTF8" + ["", "_VALUE"] and
+    result = "application/problem+json;charset=UTF-8"
+    or
+    fieldName = "APPLICATION_PROBLEM_XML" + ["", "_VALUE"] and result = "application/problem+xml"
+    or
+    fieldName = "APPLICATION_RSS_XML" + ["", "_VALUE"] and result = "application/rss+xml"
+    or
+    fieldName = "APPLICATION_STREAM_JSON" + ["", "_VALUE"] and result = "application/stream+json"
+    or
+    fieldName = "APPLICATION_XHTML_XML" + ["", "_VALUE"] and result = "application/xhtml+xml"
+    or
+    fieldName = "APPLICATION_XML" + ["", "_VALUE"] and result = "application/xml"
+    or
+    fieldName = "IMAGE_GIF" + ["", "_VALUE"] and result = "image/gif"
+    or
+    fieldName = "IMAGE_JPEG" + ["", "_VALUE"] and result = "image/jpeg"
+    or
+    fieldName = "IMAGE_PNG" + ["", "_VALUE"] and result = "image/png"
+    or
+    fieldName = "MULTIPART_FORM_DATA" + ["", "_VALUE"] and result = "multipart/form-data"
+    or
+    fieldName = "MULTIPART_MIXED" + ["", "_VALUE"] and result = "multipart/mixed"
+    or
+    fieldName = "MULTIPART_RELATED" + ["", "_VALUE"] and result = "multipart/related"
+    or
+    fieldName = "TEXT_EVENT_STREAM" + ["", "_VALUE"] and result = "text/event-stream"
+    or
+    fieldName = "TEXT_HTML" + ["", "_VALUE"] and result = "text/html"
+    or
+    fieldName = "TEXT_MARKDOWN" + ["", "_VALUE"] and result = "text/markdown"
+    or
+    fieldName = "TEXT_PLAIN" + ["", "_VALUE"] and result = "text/plain"
+    or
+    fieldName = "TEXT_XML" + ["", "_VALUE"] and result = "text/xml"
+  )
+}
+
+private predicate isXssVulnerableContentTypeExpr(Expr e) {
+  XSS::isXssVulnerableContentType(e.(CompileTimeConstantExpr).getStringValue()) or
+  XSS::isXssVulnerableContentType(getSpringConstantContentType(e))
+}
+
+private predicate isXssSafeContentTypeExpr(Expr e) {
+  XSS::isXssSafeContentType(e.(CompileTimeConstantExpr).getStringValue()) or
+  XSS::isXssSafeContentType(getSpringConstantContentType(e))
+}
+
+private DataFlow::Node getABodyBuilderWithExplicitContentType(Expr contentType) {
+  result.asExpr() =
+    any(MethodAccess ma |
+      ma.getCallee()
+          .hasQualifiedName("org.springframework.http", "ResponseEntity$BodyBuilder", "contentType") and
+      contentType = ma.getArgument(0)
+    )
+  or
+  result.asExpr() =
+    any(MethodAccess ma |
+      ma.getQualifier() = getABodyBuilderWithExplicitContentType(contentType).asExpr() and
+      ma.getType()
+          .(RefType)
+          .hasQualifiedName("org.springframework.http", "ResponseEntity$BodyBuilder")
+    )
+  or
+  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 {
+  SanitizedBodyCall() {
+    this.asExpr() =
+      any(MethodAccess ma |
+        ma.getQualifier() = getASanitizedBodyBuilder().asExpr() and
+        ma.getCallee().hasName("body")
+      ).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/lib/semmle/code/java/security/XSS.qll

@@ -46,48 +46,6 @@ private class DefaultXssSink extends XssSink {
       writer.hasFlowToExpr(ma.getQualifier()) and
       this.asExpr() = ma.getArgument(_)
     )
-    or
-    exists(SpringRequestMappingMethod requestMappingMethod, ReturnStmt rs |
-      requestMappingMethod = rs.getEnclosingCallable() and
-      this.asExpr() = rs.getResult() and
-      (
-        not exists(requestMappingMethod.getProduces()) or
-        requestMappingMethod.getProduces().matches("text/%")
-      )
-    |
-      // If a Spring request mapping method is either annotated with @ResponseBody (or equivalent),
-      // or returns a HttpEntity or sub-type, then the return value of the method is converted into
-      // a HTTP reponse using a HttpMessageConverter implementation. The implementation is chosen
-      // based on the return type of the method, and the Accept header of the request.
-      //
-      // By default, the only message converter which produces a response which is vulnerable to
-      // XSS is the StringHttpMessageConverter, which "Accept"s all text/* content types, including
-      // text/html. Therefore, if a browser request includes "text/html" in the "Accept" header,
-      // any String returned will be converted into a text/html response.
-      requestMappingMethod.isResponseBody() and
-      requestMappingMethod.getReturnType() instanceof TypeString
-      or
-      exists(Type returnType |
-        // A return type of HttpEntity<T> or ResponseEntity<T> represents an HTTP response with both
-        // a body and a set of headers. The body is subject to the same HttpMessageConverter
-        // process as above.
-        returnType = requestMappingMethod.getReturnType() and
-        (
-          returnType instanceof SpringHttpEntity
-          or
-          returnType instanceof SpringResponseEntity
-        )
-      |
-        // The type argument, representing the type of the body, is type String
-        returnType.(ParameterizedClass).getTypeArgument(0) instanceof TypeString
-        or
-        // Return type is a Raw class, which means no static type information on the body. In this
-        // case we will still treat this as an XSS sink, but rely on our taint flow steps for
-        // HttpEntity/ResponseEntity to only pass taint into those instances if the body type was
-        // String.
-        returnType instanceof RawClass
-      )
-    )
   }
 }
 

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

@@ -28,11 +28,11 @@ public static ResponseEntity<String> specificContentType(boolean safeContentType
     }
     else {
       if(chainDirectly) {
-        return builder.contentType(MediaType.APPLICATION_JSON).body(userControlled); // $SPURIOUS: xss
+        return builder.contentType(MediaType.APPLICATION_JSON).body(userControlled);
       }
       else {
         ResponseEntity.BodyBuilder builder2 = builder.contentType(MediaType.APPLICATION_JSON);
-        return builder2.body(userControlled); // $SPURIOUS: xss
+        return builder2.body(userControlled);
       }
     }
 
@@ -60,7 +60,7 @@ public static ResponseEntity<String> methodContentTypeSafeStringLiteral(String u
 
   @GetMapping(value = "/xyz", produces = MediaType.TEXT_HTML_VALUE)
   public static ResponseEntity<String> methodContentTypeUnsafe(String userControlled) {
-    return ResponseEntity.ok(userControlled); // $MISSING: xss
+    return ResponseEntity.ok(userControlled); // $xss
   }
 
   @GetMapping(value = "/xyz", produces = "text/html")
@@ -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)
@@ -105,12 +105,12 @@ public static ResponseEntity<String> methodContentTypeMaybeSafeStringLiterals(St
   private static class ClassContentTypeSafe {
     @GetMapping(value = "/abc")
     public ResponseEntity<String> test(String userControlled) {
-      return ResponseEntity.ok(userControlled); // $SPURIOUS: xss
+      return ResponseEntity.ok(userControlled);
     }
 
     @GetMapping(value = "/abc")
     public String testDirectReturn(String userControlled) {
-      return userControlled; // $SPURIOUS: xss
+      return userControlled;
     }
 
     @GetMapping(value = "/xyz", produces = {"text/html"})
@@ -139,12 +139,12 @@ public String testDirectReturn(String userControlled) {
 
     @GetMapping(value = "/xyz", produces = {"application/json"})
     public ResponseEntity<String> overridesWithSafe(String userControlled) {
-      return ResponseEntity.ok(userControlled); // $SPURIOUS: xss
+      return ResponseEntity.ok(userControlled);
     }
 
     @GetMapping(value = "/abc")
     public ResponseEntity<String> overridesWithSafe2(String userControlled) {
-      return ResponseEntity.ok().contentType(MediaType.APPLICATION_JSON).body(userControlled); // $SPURIOUS: xss
+      return ResponseEntity.ok().contentType(MediaType.APPLICATION_JSON).body(userControlled);
     }
   }