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

Jax-RS: implement content-type tracking

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,6 +283,23 @@ 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") }
@@ -290,17 +307,10 @@ class JaxRSProducesAnnotation extends JaxRSAnnotation {
   /**
    * 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 @@ public class JakartaRs1 { // $ RootResourceClass
     @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 @@ public class JaxRs1 { // $ RootResourceClass
     @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/query-tests/security/CWE-079/semmle/tests/JaxXSS.java

@@ -37,18 +37,18 @@ public class JaxXSS {
     else {
       if(chainDirectly) {
         if(contentTypeFirst)
-          return builder.type(MediaType.APPLICATION_JSON).entity(userControlled).build(); // $SPURIOUS: xss
+          return builder.type(MediaType.APPLICATION_JSON).entity(userControlled).build();
         else
-          return builder.entity(userControlled).type(MediaType.APPLICATION_JSON).build(); // $SPURIOUS: xss
+          return builder.entity(userControlled).type(MediaType.APPLICATION_JSON).build();
       }
       else {
         if(contentTypeFirst) {
           Response.ResponseBuilder builder2 = builder.type(MediaType.APPLICATION_JSON);
-          return builder2.entity(userControlled).build(); // $SPURIOUS: xss
+          return builder2.entity(userControlled).build();
         }
         else {
           Response.ResponseBuilder builder2 = builder.entity(userControlled);
-          return builder2.type(MediaType.APPLICATION_JSON).build(); // $SPURIOUS: xss
+          return builder2.type(MediaType.APPLICATION_JSON).build();
         }
       }
     }
@@ -63,39 +63,43 @@ public class JaxXSS {
     if(safeContentType) {
       if(route == 0) {
         // via ok, as a string literal:
-        return Response.ok(userControlled, "application/json").build(); // $SPURIOUS: xss
+        return Response.ok(userControlled, "application/json").build();
       }
       else if(route == 1) {
         // via ok, as a string constant:
-        return Response.ok(userControlled, MediaType.APPLICATION_JSON).build(); // $SPURIOUS: xss
+        return Response.ok(userControlled, MediaType.APPLICATION_JSON).build();
       }
       else if(route == 2) {
         // via ok, as a MediaType constant:
-        return Response.ok(userControlled, MediaType.APPLICATION_JSON_TYPE).build(); // $SPURIOUS: xss
+        return Response.ok(userControlled, MediaType.APPLICATION_JSON_TYPE).build();
       }
       else if(route == 3) {
         // via ok, as a Variant, via constructor:
-        return Response.ok(userControlled, new Variant(MediaType.APPLICATION_JSON_TYPE, "language", "encoding")).build(); // $SPURIOUS: xss
+        return Response.ok(userControlled, new Variant(MediaType.APPLICATION_JSON_TYPE, "language", "encoding")).build();
       }
       else if(route == 4) {
         // via ok, as a Variant, via static method:
-        return Response.ok(userControlled, Variant.mediaTypes(MediaType.APPLICATION_JSON_TYPE).build().get(0)).build(); // $SPURIOUS: xss
+        return Response.ok(userControlled, Variant.mediaTypes(MediaType.APPLICATION_JSON_TYPE).build().get(0)).build();
+      }
+      else if(route == -4) {
+        // via ok, as a Variant, via static method (testing multiple media types):
+        return Response.ok(userControlled, Variant.mediaTypes(MediaType.APPLICATION_JSON_TYPE, MediaType.APPLICATION_OCTET_STREAM_TYPE).build().get(0)).build();
       }
       else if(route == 5) {
         // via ok, as a Variant, via instance method:
-        return Response.ok(userControlled, Variant.languages(Locale.UK).mediaTypes(MediaType.APPLICATION_JSON_TYPE).build().get(0)).build(); // $SPURIOUS: xss
+        return Response.ok(userControlled, Variant.languages(Locale.UK).mediaTypes(MediaType.APPLICATION_JSON_TYPE).build().get(0)).build();
       }
       else if(route == 6) {
         // via builder variant, before entity:
-        return Response.ok().variant(new Variant(MediaType.APPLICATION_JSON_TYPE, "language", "encoding")).entity(userControlled).build(); // $SPURIOUS: xss
+        return Response.ok().variant(new Variant(MediaType.APPLICATION_JSON_TYPE, "language", "encoding")).entity(userControlled).build();
       }
       else if(route == 7) {
         // via builder variant, after entity:
-        return Response.ok().entity(userControlled).variant(new Variant(MediaType.APPLICATION_JSON_TYPE, "language", "encoding")).build(); // $SPURIOUS: xss
+        return Response.ok().entity(userControlled).variant(new Variant(MediaType.APPLICATION_JSON_TYPE, "language", "encoding")).build();
       }
       else if(route == 8) {
         // provide entity via ok, then content-type via builder:
-        return Response.ok(userControlled).type(MediaType.APPLICATION_JSON_TYPE).build(); // $SPURIOUS: xss
+        return Response.ok(userControlled).type(MediaType.APPLICATION_JSON_TYPE).build();
       }
     }
     else {
@@ -158,27 +162,27 @@ public class JaxXSS {
 
   @GET @Produces(MediaType.TEXT_HTML)
   public static Response methodContentTypeUnsafe(String userControlled) {
-    return Response.ok(userControlled).build(); // $MISSING: xss
+    return Response.ok(userControlled).build(); // $xss
   }
 
   @POST @Produces(MediaType.TEXT_HTML)
   public static Response methodContentTypeUnsafePost(String userControlled) {
-    return Response.ok(userControlled).build(); // $MISSING: xss
+    return Response.ok(userControlled).build(); // $xss
   }
 
   @GET @Produces("text/html")
   public static Response methodContentTypeUnsafeStringLiteral(String userControlled) {
-    return Response.ok(userControlled).build(); // $MISSING: 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(); // $MISSING: 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(); // $MISSING: xss
+    return Response.ok().type(MediaType.TEXT_HTML).entity(userControlled).build(); // $xss
   }
 
   @GET @Produces(MediaType.TEXT_HTML)
@@ -201,12 +205,12 @@ public class JaxXSS {
 
     @GET @Produces({"text/html"})
     public Response overridesWithUnsafe(String userControlled) {
-      return Response.ok(userControlled).build(); // $MISSING: xss
+      return Response.ok(userControlled).build(); // $xss
     }
 
     @GET
     public Response overridesWithUnsafe2(String userControlled) {
-      return Response.ok().type(MediaType.TEXT_HTML).entity(userControlled).build(); // $MISSING: xss
+      return Response.ok().type(MediaType.TEXT_HTML).entity(userControlled).build(); // $xss
     }
   }
 
@@ -215,12 +219,12 @@ public class JaxXSS {
   public static class ClassContentTypeUnsafe {
     @GET
     public Response test(String userControlled) {
-      return Response.ok(userControlled).build(); // $MISSING: xss
+      return Response.ok(userControlled).build(); // $xss
     }
 
     @GET
     public String testDirectReturn(String userControlled) {
-      return userControlled; // $MISSING: xss
+      return userControlled; // $xss
     }
 
     @GET @Produces({"application/json"})