Merge pull request github#6162 from smowton/smowton/feature/jax-rs-content-type-sensitivity-fixes

Jax-RS: implement content-type tracking

Author: smowton

java/change-notes/2021-06-25-jax-rs-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 Jax-RS 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/src/Security/CWE/CWE-079/XSS.ql

@@ -25,6 +25,8 @@ class XSSConfig extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) { node instanceof XssSanitizer }
 
+  override predicate isSanitizerOut(DataFlow::Node node) { node instanceof XssSinkBarrier }
+
   override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
     any(XssAdditionalTaintStep s).step(node1, node2)
   }

java/ql/src/semmle/code/java/frameworks/JaxWS.qll

@@ -283,24 +283,34 @@ class MessageBodyReaderRead extends Method {
   }
 }
 
+/**
+ * Gets a constant content-type described by expression `e` (either a string constant or a Jax-RS MediaType field access).
+ */
+string getContentTypeString(Expr e) {
+  result = e.(CompileTimeConstantExpr).getStringValue() and
+  result != ""
+  or
+  exists(Field jaxMediaType |
+    // Accesses to static fields on `MediaType` class do not have constant strings in the database
+    // so convert the field name to a content type string
+    jaxMediaType.getDeclaringType().hasQualifiedName(getAJaxRsPackage("core"), "MediaType") and
+    jaxMediaType.getAnAccess() = e and
+    // e.g. MediaType.TEXT_PLAIN => text/plain
+    result = jaxMediaType.getName().toLowerCase().replaceAll("_value", "").replaceAll("_", "/")
+  )
+}
+
 /** An `@Produces` annotation that describes which content types can be produced by this resource. */
 class JaxRSProducesAnnotation extends JaxRSAnnotation {
   JaxRSProducesAnnotation() { this.getType().hasQualifiedName(getAJaxRsPackage(), "Produces") }
 
   /**
    * Gets a declared content type that can be produced by this resource.
    */
-  string getADeclaredContentType() {
-    result = this.getAValue().(CompileTimeConstantExpr).getStringValue()
+  Expr getADeclaredContentTypeExpr() {
+    result = this.getAValue() and not result instanceof ArrayInit
     or
-    exists(Field jaxMediaType |
-      // Accesses to static fields on `MediaType` class do not have constant strings in the database
-      // so convert the field name to a content type string
-      jaxMediaType.getDeclaringType().hasQualifiedName(getAJaxRsPackage("core"), "MediaType") and
-      jaxMediaType.getAnAccess() = this.getAValue() and
-      // e.g. MediaType.TEXT_PLAIN => text/plain
-      result = jaxMediaType.getName().toLowerCase().replaceAll("_", "/")
-    )
+    result = this.getAValue().(ArrayInit).getAnInit()
   }
 }
 
@@ -319,7 +329,9 @@ private class JaxRSXssSink extends XssSink {
     |
       not exists(resourceMethod.getProducesAnnotation())
       or
-      resourceMethod.getProducesAnnotation().getADeclaredContentType() = "text/plain"
+      isXssVulnerableContentType(getContentTypeString(resourceMethod
+              .getProducesAnnotation()
+              .getADeclaredContentTypeExpr()))
     )
   }
 }
@@ -796,3 +808,150 @@ private class JaxRsUrlOpenSink extends SinkModelCsv {
       ]
   }
 }
+
+private predicate isXssVulnerableContentTypeExpr(Expr e) {
+  isXssVulnerableContentType(getContentTypeString(e))
+}
+
+private predicate isXssSafeContentTypeExpr(Expr e) { isXssSafeContentType(getContentTypeString(e)) }
+
+/**
+ * Gets a builder expression or related type that is configured to use the given `contentType`.
+ *
+ * This could be an instance of `Response.ResponseBuilder`, `Variant`, `Variant.VariantListBuilder` or
+ * a `List<Variant>`.
+ *
+ * This predicate is used to search forwards for response entities set after the content-type is configured.
+ * It does not need to consider cases where the entity is set in the same call, or the entity has already
+ * been set: these are handled by simple sanitization below.
+ */
+private DataFlow::Node getABuilderWithExplicitContentType(Expr contentType) {
+  // Base case: ResponseBuilder.type(contentType)
+  result.asExpr() =
+    any(MethodAccess ma |
+      ma.getCallee().hasQualifiedName(getAJaxRsPackage("core"), "Response$ResponseBuilder", "type") and
+      contentType = ma.getArgument(0)
+    )
+  or
+  // Base case: new Variant(contentType, ...)
+  result.asExpr() =
+    any(ClassInstanceExpr cie |
+      cie.getConstructedType().hasQualifiedName(getAJaxRsPackage("core"), "Variant") and
+      contentType = cie.getArgument(0)
+    )
+  or
+  // Base case: Variant[.VariantListBuilder].mediaTypes(...)
+  result.asExpr() =
+    any(MethodAccess ma |
+      ma.getCallee()
+          .hasQualifiedName(getAJaxRsPackage("core"), ["Variant", "Variant$VariantListBuilder"],
+            "mediaTypes") and
+      contentType = ma.getAnArgument()
+    )
+  or
+  // Recursive case: propagate through variant list building:
+  result.asExpr() =
+    any(MethodAccess ma |
+      (
+        ma.getType()
+            .(RefType)
+            .hasQualifiedName(getAJaxRsPackage("core"), "Variant$VariantListBuilder")
+        or
+        ma.getMethod()
+            .hasQualifiedName(getAJaxRsPackage("core"), "Variant$VariantListBuilder", "build")
+      ) and
+      [ma.getAnArgument(), ma.getQualifier()] =
+        getABuilderWithExplicitContentType(contentType).asExpr()
+    )
+  or
+  // Recursive case: propagate through a List.get operation
+  result.asExpr() =
+    any(MethodAccess ma |
+      ma.getMethod().hasQualifiedName("java.util", "List<Variant>", "get") and
+      ma.getQualifier() = getABuilderWithExplicitContentType(contentType).asExpr()
+    )
+  or
+  // Recursive case: propagate through Response.ResponseBuilder operations, including the `variant(...)` operation.
+  result.asExpr() =
+    any(MethodAccess ma |
+      ma.getType().(RefType).hasQualifiedName(getAJaxRsPackage("core"), "Response$ResponseBuilder") and
+      [ma.getQualifier(), ma.getArgument(0)] =
+        getABuilderWithExplicitContentType(contentType).asExpr()
+    )
+  or
+  // Recursive case: ordinary local dataflow
+  DataFlow::localFlowStep(getABuilderWithExplicitContentType(contentType), result)
+}
+
+private DataFlow::Node getASanitizedBuilder() {
+  result = getABuilderWithExplicitContentType(any(Expr e | isXssSafeContentTypeExpr(e)))
+}
+
+private DataFlow::Node getAVulnerableBuilder() {
+  result = getABuilderWithExplicitContentType(any(Expr e | isXssVulnerableContentTypeExpr(e)))
+}
+
+/**
+ * A response builder sanitized by setting a safe content type.
+ *
+ * The content type could be set before the `entity(...)` call that needs sanitizing
+ * (e.g. `Response.ok().type("application/json").entity(sanitizeMe)`)
+ * or at the same time (e.g. `Response.ok(sanitizeMe, "application/json")`
+ * or the content-type could be set afterwards (e.g. `Response.ok().entity(userControlled).type("application/json")`)
+ *
+ * This differs from `getASanitizedBuilder` in that we also include functions that must set the entity
+ * at the same time, or the entity must already have been set, so propagating forwards to sanitize future
+ * build steps is not necessary.
+ */
+private class SanitizedResponseBuilder extends XssSanitizer {
+  SanitizedResponseBuilder() {
+    // e.g. sanitizeMe.type("application/json")
+    this = getASanitizedBuilder()
+    or
+    this.asExpr() =
+      any(MethodAccess ma |
+        ma.getMethod().hasQualifiedName(getAJaxRsPackage("core"), "Response", "ok") and
+        (
+          // e.g. Response.ok(sanitizeMe, new Variant("application/json", ...))
+          ma.getArgument(1) = getASanitizedBuilder().asExpr()
+          or
+          // e.g. Response.ok(sanitizeMe, "application/json")
+          isXssSafeContentTypeExpr(ma.getArgument(1))
+        )
+      )
+  }
+}
+
+/**
+ * An entity call that serves as a sink and barrier because it has a vulnerable content-type set.
+ *
+ * We flag these as direct sinks because otherwise it may be sanitized when it reaches a resource
+ * method with a safe-looking `@Produces` annotation. They are barriers because otherwise if the
+ * resource method does *not* have a safe-looking `@Produces` annotation then it would be doubly
+ * reported, once at the `entity(...)` call and once on return from the resource method.
+ */
+private class VulnerableEntity extends XssSinkBarrier {
+  VulnerableEntity() {
+    this.asExpr() =
+      any(MethodAccess ma |
+        (
+          // Vulnerable content-type already set:
+          ma.getQualifier() = getAVulnerableBuilder().asExpr()
+          or
+          // Vulnerable content-type set in the future:
+          getAVulnerableBuilder().asExpr().(MethodAccess).getQualifier*() = ma
+        ) and
+        ma.getMethod().hasName("entity")
+      ).getArgument(0)
+    or
+    this.asExpr() =
+      any(MethodAccess ma |
+        (
+          isXssVulnerableContentTypeExpr(ma.getArgument(1))
+          or
+          ma.getArgument(1) = getAVulnerableBuilder().asExpr()
+        ) and
+        ma.getMethod().hasName("ok")
+      ).getArgument(0)
+  }
+}

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

@@ -15,6 +15,12 @@ abstract class XssSink extends DataFlow::Node { }
 /** A sanitizer that neutralizes dangerous characters that can be used to perform a XSS attack. */
 abstract class XssSanitizer extends DataFlow::Node { }
 
+/**
+ * A sink that represent a method that outputs data without applying contextual output encoding,
+ * and which should truncate flow paths such that downstream sinks are not flagged as well.
+ */
+abstract class XssSinkBarrier extends XssSink { }
+
 /**
  * A unit class for adding additional taint steps.
  *
@@ -132,3 +138,20 @@ class ServletWriterSource extends MethodAccess {
     )
   }
 }
+
+/**
+ * Holds if `s` is an HTTP Content-Type vulnerable to XSS.
+ */
+bindingset[s]
+predicate isXssVulnerableContentType(string s) {
+  s.regexpMatch("(?i)text/(html|xml|xsl|rdf|vtt|cache-manifest).*") or
+  s.regexpMatch("(?i)application/(.*\\+)?xml.*") or
+  s.regexpMatch("(?i)cache-manifest.*") or
+  s.regexpMatch("(?i)image/svg\\+xml.*")
+}
+
+/**
+ * Holds if `s` is an HTTP Content-Type that is not vulnerable to XSS.
+ */
+bindingset[s]
+predicate isXssSafeContentType(string s) { not isXssVulnerableContentType(s) }

java/ql/test/library-tests/frameworks/JaxWs/JakartaRs1.java

@@ -71,7 +71,7 @@ int Get() { // $ ResourceMethod ResourceMethodOnResourceClass
     @Produces("text/html") // $ ProducesAnnotation=text/html
     @POST
     boolean Post() { // $ ResourceMethod=text/html ResourceMethodOnResourceClass
-      return false;
+      return false; // $ XssSink
     }
 
     @Produces(MediaType.TEXT_PLAIN) // $ ProducesAnnotation=text/plain

java/ql/test/library-tests/frameworks/JaxWs/JaxRs.ql

@@ -25,7 +25,8 @@ class JaxRsTest extends InlineExpectationsTest {
       element = resourceMethod.toString() and
       if exists(resourceMethod.getProducesAnnotation())
       then
-        value = resourceMethod.getProducesAnnotation().getADeclaredContentType() and
+        value =
+          getContentTypeString(resourceMethod.getProducesAnnotation().getADeclaredContentTypeExpr()) and
         value != ""
       else
         // Filter out empty strings that stem from using stubs.
@@ -143,7 +144,7 @@ class JaxRsTest extends InlineExpectationsTest {
     exists(JaxRSProducesAnnotation producesAnnotation |
       producesAnnotation.getLocation() = location and
       element = producesAnnotation.toString() and
-      value = producesAnnotation.getADeclaredContentType() and
+      value = getContentTypeString(producesAnnotation.getADeclaredContentTypeExpr()) and
       value != ""
       // Filter out empty strings that stem from using stubs.
       // If we built the test against the real JAR then the field

java/ql/test/library-tests/frameworks/JaxWs/JaxRs1.java

@@ -71,7 +71,7 @@ int Get() { // $ ResourceMethod ResourceMethodOnResourceClass
     @Produces("text/html") // $ ProducesAnnotation=text/html
     @POST
     boolean Post() { // $ ResourceMethod=text/html ResourceMethodOnResourceClass
-      return false;
+      return false; // $ XssSink
     }
 
     @Produces(MediaType.TEXT_PLAIN) // $ ProducesAnnotation=text/plain