From 856046ce50da2297cd4002dcdf6511e51c558376 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Fri, 25 Jun 2021 18:07:18 +0100 Subject: [PATCH] Jax-RS: implement content-type tracking This follows content-type specifications across Variant-related functions and the ResponseBuilder class in order to sanitize or sink entities as appropriate. --- .../src/semmle/code/java/frameworks/JaxWS.qll | 155 +++++++++++++++++- .../security/CWE-079/semmle/tests/JaxXSS.java | 40 +++-- 2 files changed, 174 insertions(+), 21 deletions(-) diff --git a/java/ql/src/semmle/code/java/frameworks/JaxWS.qll b/java/ql/src/semmle/code/java/frameworks/JaxWS.qll index 3dd19bff928..9e6ef2c1dfc 100644 --- a/java/ql/src/semmle/code/java/frameworks/JaxWS.qll +++ b/java/ql/src/semmle/code/java/frameworks/JaxWS.qll @@ -284,7 +284,8 @@ class MessageBodyReaderRead extends Method { } private string getContentTypeString(Expr e) { - result = e.(CompileTimeConstantExpr).getStringValue() + 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 @@ -327,8 +328,9 @@ private class JaxRSXssSink extends XssSink { | not exists(resourceMethod.getProducesAnnotation()) or - getContentTypeString(resourceMethod.getProducesAnnotation().getADeclaredContentTypeExpr()) = - "text/plain" + isXssVulnerableContentType(getContentTypeString(resourceMethod + .getProducesAnnotation() + .getADeclaredContentTypeExpr())) ) } } @@ -805,3 +807,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`. + * + * This routine 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", "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::localFlow(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) + } +} diff --git a/java/ql/test/query-tests/security/CWE-079/semmle/tests/JaxXSS.java b/java/ql/test/query-tests/security/CWE-079/semmle/tests/JaxXSS.java index 62f99bbdab0..5a55cb39a5d 100644 --- a/java/ql/test/query-tests/security/CWE-079/semmle/tests/JaxXSS.java +++ b/java/ql/test/query-tests/security/CWE-079/semmle/tests/JaxXSS.java @@ -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"})