diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql index 35d33dc8a84..6fb5d164ff9 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql @@ -15,6 +15,18 @@ import UnsafeUrlForward import semmle.code.java.dataflow.FlowSources import DataFlow::PathGraph +private class StartsWithSanitizer extends DataFlow::BarrierGuard { + StartsWithSanitizer() { + this.(MethodAccess).getMethod().hasName("startsWith") and + this.(MethodAccess).getMethod().getDeclaringType() instanceof TypeString and + this.(MethodAccess).getMethod().getNumberOfParameters() = 1 + } + + override predicate checks(Expr e, boolean branch) { + e = this.(MethodAccess).getQualifier() and branch = true + } +} + class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" } @@ -25,39 +37,18 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { ma.getMethod() .getDeclaringType() .getASupertype*() - .hasQualifiedName("javax.servlet.http", "HttpServletRequest") + .hasQualifiedName("javax.servlet.http", "HttpServletRequest") and + ma = source.asExpr() ) } override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink } - override predicate isSanitizer(DataFlow::Node node) { - node.getType() instanceof BoxedType - or - node.getType() instanceof PrimitiveType - or - exists(AddExpr ae | - ae.getRightOperand() = node.asExpr() and - ( - not ae.getLeftOperand().(CompileTimeConstantExpr).getStringValue().matches("/WEB-INF/%") and - not ae.getLeftOperand().(CompileTimeConstantExpr).getStringValue() = "forward:" - ) - ) - or - exists(MethodAccess ma, int i | - ma.getMethod().hasName("format") and - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "redirect:" and - ma.getArgument(i) = node.asExpr() and - i != 0 - ) - or - exists(StringBuilderAppendCall ma1, StringBuilderAppendCall ma2 | - DataFlow2::localExprFlow(ma1.getQualifier(), ma2.getQualifier()) and - ma1.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "redirect:" and - ma2.getArgument(0) = node.asExpr() - ) + override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { + guard instanceof StartsWithSanitizer } + + override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer } } from DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeUrlForwardFlowConfig conf diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll index 019712a69e6..6926518b448 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -3,11 +3,141 @@ import DataFlow import semmle.code.java.dataflow.FlowSources import semmle.code.java.frameworks.Servlets +/** A sanitizer for unsafe url forward vulnerabilities. */ +abstract class UnsafeUrlForwardSanitizer extends DataFlow::Node { } + +private class PrimitiveSanitizer extends UnsafeUrlForwardSanitizer { + PrimitiveSanitizer() { + this.getType() instanceof PrimitiveType or + this.getType() instanceof BoxedType or + this.getType() instanceof NumberType + } +} + +private class UnsafeUrlForwardSantizer extends UnsafeUrlForwardSanitizer { + UnsafeUrlForwardSantizer() { this.asExpr() instanceof UnsafeUrlForwardSanitizedExpr } +} + +private class UnsafeUrlForwardSanitizingConstantPrefix extends CompileTimeConstantExpr { + UnsafeUrlForwardSanitizingConstantPrefix() { + not this.getStringValue().matches("/WEB-INF/%") and + not this.getStringValue() = "forward:" + } +} + +private Expr getAUnsafeUrlForwardSanitizingPrefix() { + result instanceof UnsafeUrlForwardSanitizingConstantPrefix + or + result.(AddExpr).getAnOperand() = getAUnsafeUrlForwardSanitizingPrefix() +} + /** A call to `StringBuilder.append` method. */ -class StringBuilderAppendCall extends MethodAccess { - StringBuilderAppendCall() { - this.getMethod().hasName("append") and - this.getMethod().getDeclaringType() instanceof StringBuildingType +class StringBuilderAppend extends MethodAccess { + StringBuilderAppend() { + this.getMethod().getDeclaringType() instanceof StringBuildingType and + this.getMethod().hasName("append") + } +} + +private Expr getQualifier(Expr e) { result = e.(MethodAccess).getQualifier() } + +/** + * An extension of `StringBuilderVar` that also accounts for strings appended in StringBuilder/Buffer's constructor + * and in `append` calls chained onto the constructor call. + * + * The original `StringBuilderVar` doesn't care about these because it is designed to model taint, and + * in taint rules terms these are not needed, as the connection between construction, appends and the + * eventual `toString` is more obvious. + */ +private class StringBuilderVarExt extends StringBuilderVar { + /** + * Returns a first assignment after this StringBuilderVar is first assigned. + * + * For example, for `StringBuilder sbv = new StringBuilder("1").append("2"); sbv.append("3").append("4");` + * this returns the append of `"3"`. + */ + private StringBuilderAppend getAFirstAppendAfterAssignment() { + result = this.getAnAppend() and not result = this.getNextAppend(_) + } + + /** + * Gets the next `append` after `prev`, where `prev` is, perhaps after some more `append` or other + * chained calls, assigned to this `StringBuilderVar`. + */ + private StringBuilderAppend getNextAssignmentChainedAppend(StringBuilderConstructorOrAppend prev) { + getQualifier*(result) = this.getAnAssignedValue() and + result.getQualifier() = prev + } + + /** + * Get a constructor call or `append` call that contributes a string to this string builder. + */ + StringBuilderConstructorOrAppend getAConstructorOrAppend() { + exists(this.getNextAssignmentChainedAppend(result)) or + result = this.getAnAssignedValue() or + result = this.getAnAppend() + } + + /** + * Like `StringBuilderVar.getNextAppend`, except including appends and constructors directly + * assigned to this `StringBuilderVar`. + */ + private StringBuilderAppend getNextAppendIncludingAssignmentChains( + StringBuilderConstructorOrAppend prev + ) { + result = getNextAssignmentChainedAppend(prev) + or + prev = this.getAnAssignedValue() and + result = this.getAFirstAppendAfterAssignment() + or + result = this.getNextAppend(prev) + } + + /** + * Implements `StringBuilderVarExt.getNextAppendIncludingAssignmentChains+(prev)`. + */ + pragma[nomagic] + StringBuilderAppend getSubsequentAppendIncludingAssignmentChains( + StringBuilderConstructorOrAppend prev + ) { + result = this.getNextAppendIncludingAssignmentChains(prev) or + result = + this.getSubsequentAppendIncludingAssignmentChains(this.getNextAppendIncludingAssignmentChains(prev)) + } +} + +private class StringBuilderConstructorOrAppend extends Call { + StringBuilderConstructorOrAppend() { + this instanceof StringBuilderAppend or + this.(ClassInstanceExpr).getConstructedType() instanceof StringBuildingType + } +} + +private class UnsafeUrlForwardSanitizedExpr extends Expr { + UnsafeUrlForwardSanitizedExpr() { + // Sanitize expressions that come after a sanitizing prefix in a tree of string additions: + this = + any(AddExpr add | add.getLeftOperand() = getAUnsafeUrlForwardSanitizingPrefix()) + .getRightOperand() + or + // Sanitize expressions that come after a sanitizing prefix in a sequence of StringBuilder operations: + exists( + StringBuilderConstructorOrAppend appendSanitizingConstant, + StringBuilderAppend subsequentAppend, StringBuilderVarExt v + | + appendSanitizingConstant = v.getAConstructorOrAppend() and + appendSanitizingConstant.getArgument(0) = getAUnsafeUrlForwardSanitizingPrefix() and + v.getSubsequentAppendIncludingAssignmentChains(appendSanitizingConstant) = subsequentAppend and + this = subsequentAppend.getArgument(0) + ) + or + exists(MethodAccess ma, int i | + ma.getMethod().hasName("format") and + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getArgument(0) instanceof UnsafeUrlForwardSanitizingConstantPrefix and + ma.getArgument(i) = this and + i != 0 + ) } } @@ -16,7 +146,7 @@ class StringBuilderAppendCall extends MethodAccess { * * E.g: `"forward:" + url` */ -class ForwardBuilderExpr extends AddExpr { +private class ForwardBuilderExpr extends AddExpr { ForwardBuilderExpr() { this.getLeftOperand().(CompileTimeConstantExpr).getStringValue() = "forward:" } @@ -27,7 +157,7 @@ class ForwardBuilderExpr extends AddExpr { * * E.g: `StringBuilder.append("forward:")` */ -class ForwardAppendCall extends StringBuilderAppendCall { +private class ForwardAppendCall extends StringBuilderAppend { ForwardAppendCall() { this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "forward:" } diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected index 5b9c80ff598..b70a131f93c 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected @@ -20,6 +20,7 @@ nodes | UnsafeUrlForward.java:38:33:38:35 | url | semmle.label | url | | UnsafeUrlForward.java:47:19:47:28 | url : String | semmle.label | url : String | | UnsafeUrlForward.java:49:33:49:62 | ... + ... | semmle.label | ... + ... | +subpaths #select | UnsafeUrlForward.java:14:27:14:29 | url | UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:13:27:13:36 | url | user-provided value | | UnsafeUrlForward.java:20:28:20:30 | url | UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:18:27:18:36 | url | user-provided value | diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/options b/java/ql/test/experimental/query-tests/security/CWE-552/options index a9289108747..ba166b547a0 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-552/options +++ b/java/ql/test/experimental/query-tests/security/CWE-552/options @@ -1 +1 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.2.3/ \ No newline at end of file +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8/ \ No newline at end of file