From 21b51b48eb5a8c3ba29d96281f8e7eddd63c791d Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 2 Dec 2022 18:14:42 +0100 Subject: [PATCH 1/9] Adapt PathSanitizer to Kotlin --- .../semmle/code/java/frameworks/kotlin/IO.qll | 8 + .../code/java/frameworks/kotlin/Text.qll | 20 + .../code/java/security/PathSanitizer.qll | 140 +++-- .../library-tests/pathsanitizer/Test.java | 2 +- .../library-tests/pathsanitizer/TestKt.kt | 479 ++++++++++++++++++ .../test/library-tests/pathsanitizer/options | 1 + 6 files changed, 610 insertions(+), 40 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/frameworks/kotlin/IO.qll create mode 100644 java/ql/lib/semmle/code/java/frameworks/kotlin/Text.qll create mode 100644 java/ql/test/library-tests/pathsanitizer/TestKt.kt diff --git a/java/ql/lib/semmle/code/java/frameworks/kotlin/IO.qll b/java/ql/lib/semmle/code/java/frameworks/kotlin/IO.qll new file mode 100644 index 00000000000..38af34bc690 --- /dev/null +++ b/java/ql/lib/semmle/code/java/frameworks/kotlin/IO.qll @@ -0,0 +1,8 @@ +/** Provides classes and predicates related to `kotlin.io`. */ + +import java + +/** The type `kotlin.io.FilesKt`, where `File` extension methods are declared. */ +class FilesKt extends RefType { + FilesKt() { this.hasQualifiedName("kotlin.io", "FilesKt") } +} diff --git a/java/ql/lib/semmle/code/java/frameworks/kotlin/Text.qll b/java/ql/lib/semmle/code/java/frameworks/kotlin/Text.qll new file mode 100644 index 00000000000..7f5c5b316c4 --- /dev/null +++ b/java/ql/lib/semmle/code/java/frameworks/kotlin/Text.qll @@ -0,0 +1,20 @@ +/** Provides classes and predicates related to `kotlin.text`. */ + +import java + +/** The type `kotlin.text.StringsKt`, where `String` extension methods are declared. */ +class StringsKt extends RefType { + StringsKt() { this.hasQualifiedName("kotlin.text", "StringsKt") } +} + +/** A call to the extension method `String.toRegex` from `kotlin.text`. */ +class KtToRegex extends MethodAccess { + KtToRegex() { + this.getMethod().getDeclaringType() instanceof StringsKt and + this.getMethod().hasName("toRegex") + } + + string getExpressionString() { + result = this.getArgument(0).(CompileTimeConstantExpr).getStringValue() + } +} diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index bd15b0581b7..ad861ddeb42 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -5,6 +5,8 @@ private import semmle.code.java.controlflow.Guards private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSources private import semmle.code.java.dataflow.SSA +private import semmle.code.java.frameworks.kotlin.IO +private import semmle.code.java.frameworks.kotlin.Text /** A sanitizer that protects against path injection vulnerabilities. */ abstract class PathInjectionSanitizer extends DataFlow::Node { } @@ -47,16 +49,25 @@ private module ValidationMethod { */ private predicate exactPathMatchGuard(Guard g, Expr e, boolean branch) { exists(MethodAccess ma, RefType t | - t instanceof TypeString or - t instanceof TypeUri or - t instanceof TypePath or - t instanceof TypeFile or - t.hasQualifiedName("android.net", "Uri") + ( + t instanceof TypeString or + t instanceof TypeUri or + t instanceof TypePath or + t instanceof TypeFile or + t.hasQualifiedName("android.net", "Uri") + ) and + e = ma.getQualifier().getUnderlyingExpr() + or + ( + ma.getMethod().getDeclaringType() instanceof StringsKt + or + ma.getMethod().getDeclaringType() instanceof FilesKt + ) and + e = ma.getArgument(0).getUnderlyingExpr() | ma.getMethod().getDeclaringType() = t and ma = g and - ma.getMethod().getName() = ["equals", "equalsIgnoreCase"] and - e = ma.getQualifier() and + ma.getMethod().getName() = ["equals", "equalsIgnoreCase"] + ["", "$default"] and branch = true ) } @@ -82,12 +93,18 @@ private predicate localTaintFlowToPathGuard(Expr e, PathGuard g) { } private class AllowedPrefixGuard extends PathGuard instanceof MethodAccess { + Expr checkedExpr; + AllowedPrefixGuard() { - (isStringPrefixMatch(this) or isPathPrefixMatch(this)) and + ( + isStringPrefixMatch(this, checkedExpr) + or + isPathPrefixMatch(this, checkedExpr) + ) and not isDisallowedWord(super.getAnArgument()) } - override Expr getCheckedExpr() { result = super.getQualifier() } + override Expr getCheckedExpr() { result = checkedExpr } } /** @@ -149,12 +166,18 @@ private class DotDotCheckSanitizer extends PathInjectionSanitizer { } private class BlockListGuard extends PathGuard instanceof MethodAccess { + Expr checkedExpr; + BlockListGuard() { - (isStringPartialMatch(this) or isPathPrefixMatch(this)) and + ( + isStringPartialMatch(this, checkedExpr) + or + isPathPrefixMatch(this, checkedExpr) + ) and isDisallowedWord(super.getAnArgument()) } - override Expr getCheckedExpr() { result = super.getQualifier() } + override Expr getCheckedExpr() { result = checkedExpr } } /** @@ -188,70 +211,109 @@ private class BlockListSanitizer extends PathInjectionSanitizer { } } -private predicate isStringPrefixMatch(MethodAccess ma) { - exists(Method m | m = ma.getMethod() and m.getDeclaringType() instanceof TypeString | - m.hasName("startsWith") +private class ConstantOrRegex extends Expr { + ConstantOrRegex() { + this instanceof CompileTimeConstantExpr or + this instanceof KtToRegex + } + + string getStringValue() { + result = this.(CompileTimeConstantExpr).getStringValue() or + result = this.(KtToRegex).getExpressionString() + } +} + +private predicate isStringPrefixMatch(MethodAccess ma, Expr checkedExpr) { + exists(Method m | + m = ma.getMethod() and + ( + m.getDeclaringType() instanceof TypeString and + checkedExpr = ma.getQualifier().getUnderlyingExpr() + or + m.getDeclaringType() instanceof StringsKt and + checkedExpr = ma.getArgument(0).getUnderlyingExpr() + ) + | + m.hasName("startsWith" + ["", "$default"]) or - m.hasName("regionMatches") and - ma.getArgument(0).(CompileTimeConstantExpr).getIntValue() = 0 - or - m.hasName("matches") and - not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().matches(".*%") + exists(int argPos | + m.getDeclaringType() instanceof TypeString and argPos = 0 + or + m.getDeclaringType() instanceof StringsKt and argPos = 1 + | + m.hasName("regionMatches" + ["", "$default"]) and + ma.getArgument(argPos).(CompileTimeConstantExpr).getIntValue() = 0 + or + m.hasName("matches") and + not ma.getArgument(argPos).(ConstantOrRegex).getStringValue().matches(".*%") + ) ) } /** - * Holds if `ma` is a call to a method that checks a partial string match. + * Holds if `ma` is a call to a method that checks a partial string match on `checkedExpr`. */ -private predicate isStringPartialMatch(MethodAccess ma) { - isStringPrefixMatch(ma) +private predicate isStringPartialMatch(MethodAccess ma, Expr checkedExpr) { + isStringPrefixMatch(ma, checkedExpr) or - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"]) + ( + ma.getMethod().getDeclaringType() instanceof TypeString and + checkedExpr = ma.getQualifier().getUnderlyingExpr() + or + ma.getMethod().getDeclaringType() instanceof StringsKt and + checkedExpr = ma.getArgument(0).getUnderlyingExpr() + ) and + ma.getMethod() + .hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"] + ["", "$default"]) } /** - * Holds if `ma` is a call to a method that checks whether a path starts with a prefix. + * Holds if `ma` is a call to a method that checks whether `checkedExpr` starts with a prefix. */ -private predicate isPathPrefixMatch(MethodAccess ma) { +private predicate isPathPrefixMatch(MethodAccess ma, Expr checkedExpr) { exists(RefType t | - t instanceof TypePath + t instanceof TypePath and checkedExpr = ma.getQualifier().getUnderlyingExpr() or - t.hasQualifiedName("kotlin.io", "FilesKt") + t instanceof FilesKt and + checkedExpr = ma.getArgument(0).getUnderlyingExpr() | t = ma.getMethod().getDeclaringType() and - ma.getMethod().hasName("startsWith") + ma.getMethod().hasName("startsWith" + ["", "$default"]) ) } -private predicate isDisallowedWord(CompileTimeConstantExpr word) { +private predicate isDisallowedWord(ConstantOrRegex word) { word.getStringValue().matches(["/", "\\", "%WEB-INF%", "%/data%"]) } /** A complementary guard that protects against path traversal, by looking for the literal `..`. */ private class PathTraversalGuard extends PathGuard { + Expr checkedExpr; + PathTraversalGuard() { exists(MethodAccess ma | - ma.getMethod().getDeclaringType() instanceof TypeString and + ( + ma.getMethod().getDeclaringType() instanceof TypeString and + checkedExpr = ma.getQualifier().getUnderlyingExpr() + or + ma.getMethod().getDeclaringType() instanceof StringsKt and + checkedExpr = ma.getArgument(0).getUnderlyingExpr() + ) and ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." | this = ma and - ma.getMethod().hasName("contains") + ma.getMethod().hasName("contains" + ["", "$default"]) or exists(EqualityTest eq | this = eq and - ma.getMethod().hasName(["indexOf", "lastIndexOf"]) and + ma.getMethod().hasName(["indexOf", "lastIndexOf"] + ["", "$default"]) and eq.getAnOperand() = ma and eq.getAnOperand().(CompileTimeConstantExpr).getIntValue() = -1 ) ) } - override Expr getCheckedExpr() { - exists(MethodAccess ma | ma = this.(EqualityTest).getAnOperand() or ma = this | - result = ma.getQualifier() - ) - } + override Expr getCheckedExpr() { result = checkedExpr } boolean getBranch() { this instanceof MethodAccess and result = false @@ -265,7 +327,7 @@ private class PathNormalizeSanitizer extends MethodAccess { PathNormalizeSanitizer() { exists(RefType t | t instanceof TypePath or - t.hasQualifiedName("kotlin.io", "FilesKt") + t instanceof FilesKt | this.getMethod().getDeclaringType() = t and this.getMethod().hasName("normalize") diff --git a/java/ql/test/library-tests/pathsanitizer/Test.java b/java/ql/test/library-tests/pathsanitizer/Test.java index 27a15d867ce..61e0f498bd8 100644 --- a/java/ql/test/library-tests/pathsanitizer/Test.java +++ b/java/ql/test/library-tests/pathsanitizer/Test.java @@ -279,7 +279,7 @@ public class Test { } private void blockListGuardValidation(String path) throws Exception { - if (path.contains("..") || !path.startsWith("/data")) + if (path.contains("..") || path.startsWith("/data")) throw new Exception(); } diff --git a/java/ql/test/library-tests/pathsanitizer/TestKt.kt b/java/ql/test/library-tests/pathsanitizer/TestKt.kt new file mode 100644 index 00000000000..0960e9301a7 --- /dev/null +++ b/java/ql/test/library-tests/pathsanitizer/TestKt.kt @@ -0,0 +1,479 @@ +import java.io.File +import java.net.URI +import java.nio.file.Path +import java.nio.file.Paths +import android.net.Uri + +class TestKt { + fun source(): Any? { + return null + } + + fun sink(o: Any?) {} + + @Throws(Exception::class) + private fun exactPathMatchGuardValidation(path: String?) { + if (!path.equals("/safe/path")) throw Exception() + } + + @Throws(Exception::class) + fun exactPathMatchGuard() { + run { + val source = source() as String? + // This gets extracted as Object.equals, which makes the definitions in exactPathMatchGuard not catch it. + // Note that it gets correctly extracted in Java. + if (source!!.equals("/safe/path")) + sink(source) // $SPURIOUS: $ hasTaintFlow + else + sink(source) // $ hasTaintFlow + } + run { + val source = source() as URI? + if (source!!.equals(URI("http://safe/uri"))) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + run { + val source = source() as File? + if (source!!.equals(File("/safe/file"))) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + run { + val source = source() as Uri? + if (source!!.equals(Uri.parse("http://safe/uri"))) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + run { + val source = source() as String? + exactPathMatchGuardValidation(source) + sink(source) // Safe + } + } + + @Throws(Exception::class) + private fun allowListGuardValidation(path: String?) { + if (path!!.contains("..") || !path.startsWith("/safe")) throw Exception() + } + + @Throws(Exception::class) + fun allowListGuard() { + // Prefix check by itself is not enough + run { + val source = source() as String? + if (source!!.startsWith("/safe")) { + sink(source) // $ hasTaintFlow + } else + sink(source) // $ hasTaintFlow + } + // PathTraversalGuard + allowListGuard + run { + val source = source() as String? + if (!source!!.contains("..") && source.startsWith("/safe")) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + run { + val source = source() as String? + if (source!!.indexOf("..") == -1 && source.startsWith("/safe")) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + run { + val source = source() as String? + if (source!!.lastIndexOf("..") == -1 && source.startsWith("/safe")) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + // PathTraversalSanitizer + allowListGuard + run { + val source: File? = source() as File? + val normalized: String = source!!.getCanonicalPath() + if (normalized.startsWith("/safe")) { + sink(source) // Safe + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ hasTaintFlow + } + } + run { + val source: File? = source() as File? + val normalized: String = source!!.getCanonicalFile().toString() + if (normalized.startsWith("/safe")) { + sink(source) // Safe + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ hasTaintFlow + } + } + run { + val source = source() as String? + val normalized: Path = Paths.get(source).normalize() + if (normalized.startsWith("/safe")) { + sink(source) // Safe + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ hasTaintFlow + } + } + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (normalized.startsWith("/safe")) { + sink(source) // $ SPURIOUS: hasTaintFlow + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (normalized.regionMatches(0, "/safe", 0, 5)) { + sink(source) // $ SPURIOUS: hasTaintFlow + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (normalized.matches("/safe/.*".toRegex())) { + sink(source) // $ SPURIOUS: hasTaintFlow + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + // validation method + run { + val source = source() as String? + allowListGuardValidation(source) + sink(source) // Safe + } + // PathInjectionSanitizer + partial string match is considered unsafe + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (normalized.contains("/safe")) { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (normalized.regionMatches(1, "/safe", 0, 5)) { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (normalized.matches(".*/safe/.*".toRegex())) { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + } + + @Throws(Exception::class) + private fun dotDotCheckGuardValidation(path: String?) { + if (!path!!.startsWith("/safe") || path.contains("..")) throw Exception() + } + + @Throws(Exception::class) + fun dotDotCheckGuard() { + // dot dot check by itself is not enough + run { + val source = source() as String? + if (!source!!.contains("..")) { + sink(source) // $ hasTaintFlow + } else + sink(source) // $ hasTaintFlow + } + // allowListGuard + dotDotCheckGuard + run { + val source = source() as String? + if (source!!.startsWith("/safe") && !source.contains("..")) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + run { + val source = source() as String? + if (source!!.startsWith("/safe") && source.indexOf("..") == -1) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + run { + val source = source() as String? + if (!source!!.startsWith("/safe") || source.indexOf("..") != -1) + sink(source) // $ hasTaintFlow + else + sink(source) // Safe + } + run { + val source = source() as String? + if (source!!.startsWith("/safe") && source.lastIndexOf("..") == -1) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + // blockListGuard + dotDotCheckGuard + run { + val source = source() as String? + if (!source!!.startsWith("/data") && !source.contains("..")) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + run { + val source = source() as String? + if (!source!!.startsWith("/data") && source.indexOf("..") == -1) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + run { + val source = source() as String? + if (source!!.startsWith("/data") || source.indexOf("..") != -1) + sink(source) // $ hasTaintFlow + else + sink(source) // Safe + } + run { + val source = source() as String? + if (!source!!.startsWith("/data") && source.lastIndexOf("..") == -1) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + // validation method + run { + val source = source() as String? + dotDotCheckGuardValidation(source) + sink(source) // Safe + } + } + + @Throws(Exception::class) + private fun blockListGuardValidation(path: String?) { + if (path!!.contains("..") || path.startsWith("/data")) throw Exception() + } + + @Throws(Exception::class) + fun blockListGuard() { + // Prefix check by itself is not enough + run { + val source = source() as String? + if (!source!!.startsWith("/data")) { + sink(source) // $ hasTaintFlow + } else + sink(source) // $ hasTaintFlow + } + // PathTraversalGuard + blockListGuard + run { + val source = source() as String? + if (!source!!.contains("..") && !source.startsWith("/data")) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + run { + val source = source() as String? + if (source!!.indexOf("..") == -1 && !source.startsWith("/data")) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + run { + val source = source() as String? + if (source!!.lastIndexOf("..") == -1 && !source.startsWith("/data")) + sink(source) // Safe + else + sink(source) // $ hasTaintFlow + } + // PathTraversalSanitizer + blockListGuard + run { + val source: File? = source() as File? + val normalized: String = source!!.getCanonicalPath() + if (!normalized.startsWith("/data")) { + sink(source) // Safe + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ hasTaintFlow + } + } + run { + val source: File? = source() as File? + val normalized: String = source!!.getCanonicalFile().toString() + if (!normalized.startsWith("/data")) { + sink(source) // Safe + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ hasTaintFlow + } + } + run { + val source = source() as String? + val normalized: Path = Paths.get(source).normalize() + if (!normalized.startsWith("/data")) { + sink(source) // Safe + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ hasTaintFlow + } + } + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (!normalized.startsWith("/data")) { + sink(source) // $ SPURIOUS: hasTaintFlow + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (!normalized.regionMatches(0, "/data", 0, 5)) { + sink(source) // $ SPURIOUS: hasTaintFlow + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (!normalized.matches("/data/.*".toRegex())) { + sink(source) // $ SPURIOUS: hasTaintFlow + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + // validation method + run { + val source = source() as String? + blockListGuardValidation(source) + sink(source) // Safe + } + // PathInjectionSanitizer + partial string match with disallowed words + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (!normalized.contains("/")) { + sink(source) // $ SPURIOUS: hasTaintFlow + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (!normalized.regionMatches(1, "/", 0, 5)) { + sink(source) // $ SPURIOUS: hasTaintFlow + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (!normalized.matches("/".toRegex())) { + sink(source) // $ SPURIOUS: hasTaintFlow + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + // PathInjectionSanitizer + partial string match with disallowed prefixes + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (!normalized.contains("/data")) { + sink(source) // $ SPURIOUS: hasTaintFlow + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (!normalized.regionMatches(1, "/data", 0, 5)) { + sink(source) // $ SPURIOUS: hasTaintFlow + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + run { + val source = source() as String? + // normalize().toString() gets extracted as Object.toString, stopping taint here + val normalized: String = Paths.get(source).normalize().toString() + if (!normalized.matches(".*/data/.*".toRegex())) { + sink(source) // $ SPURIOUS: hasTaintFlow + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ MISSING: hasTaintFlow + } + } + } +} \ No newline at end of file diff --git a/java/ql/test/library-tests/pathsanitizer/options b/java/ql/test/library-tests/pathsanitizer/options index 1e6e9ea2bf1..02ea18b0cac 100644 --- a/java/ql/test/library-tests/pathsanitizer/options +++ b/java/ql/test/library-tests/pathsanitizer/options @@ -1 +1,2 @@ //semmle-extractor-options: --javac-args -cp ${testdir}/../../stubs/google-android-9.0.0 +//codeql-extractor-kotlin-options: ${testdir}/../../stubs/google-android-9.0.0 From 995b7327fe0a400e1ced4e62719b287e3a322b99 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 5 Dec 2022 08:59:15 +0100 Subject: [PATCH 2/9] Add missing QLDoc --- java/ql/lib/semmle/code/java/frameworks/kotlin/Text.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/java/ql/lib/semmle/code/java/frameworks/kotlin/Text.qll b/java/ql/lib/semmle/code/java/frameworks/kotlin/Text.qll index 7f5c5b316c4..62eafa9565e 100644 --- a/java/ql/lib/semmle/code/java/frameworks/kotlin/Text.qll +++ b/java/ql/lib/semmle/code/java/frameworks/kotlin/Text.qll @@ -14,6 +14,7 @@ class KtToRegex extends MethodAccess { this.getMethod().hasName("toRegex") } + /** Gets the constant string value being converted to a regex by this call. */ string getExpressionString() { result = this.getArgument(0).(CompileTimeConstantExpr).getStringValue() } From 8fb5c37ba826ed069919ce74a56c6406758ada5f Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 5 Dec 2022 08:59:20 +0100 Subject: [PATCH 3/9] Add change note --- java/ql/lib/change-notes/2022-12-05-kotlin-path-sanitizer.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/ql/lib/change-notes/2022-12-05-kotlin-path-sanitizer.md diff --git a/java/ql/lib/change-notes/2022-12-05-kotlin-path-sanitizer.md b/java/ql/lib/change-notes/2022-12-05-kotlin-path-sanitizer.md new file mode 100644 index 00000000000..ac6d59046a0 --- /dev/null +++ b/java/ql/lib/change-notes/2022-12-05-kotlin-path-sanitizer.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The library `PathSanitizer.qll` has been improved to detect more path validation patterns in Kotlin. From 71a6b09badbe5b2dc61d0c0c3fcfe5c3904f8cd7 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 5 Dec 2022 11:52:02 +0100 Subject: [PATCH 4/9] Minor syntax change in tests --- java/ql/test/library-tests/pathsanitizer/TestKt.kt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/java/ql/test/library-tests/pathsanitizer/TestKt.kt b/java/ql/test/library-tests/pathsanitizer/TestKt.kt index 0960e9301a7..ec8ab63a0cf 100644 --- a/java/ql/test/library-tests/pathsanitizer/TestKt.kt +++ b/java/ql/test/library-tests/pathsanitizer/TestKt.kt @@ -95,7 +95,7 @@ class TestKt { // PathTraversalSanitizer + allowListGuard run { val source: File? = source() as File? - val normalized: String = source!!.getCanonicalPath() + val normalized: String = source!!.canonicalPath if (normalized.startsWith("/safe")) { sink(source) // Safe sink(normalized) // Safe @@ -106,7 +106,7 @@ class TestKt { } run { val source: File? = source() as File? - val normalized: String = source!!.getCanonicalFile().toString() + val normalized: File = source!!.canonicalFile.toString() if (normalized.startsWith("/safe")) { sink(source) // Safe sink(normalized) // Safe @@ -328,7 +328,7 @@ class TestKt { // PathTraversalSanitizer + blockListGuard run { val source: File? = source() as File? - val normalized: String = source!!.getCanonicalPath() + val normalized: String = source!!.canonicalPath if (!normalized.startsWith("/data")) { sink(source) // Safe sink(normalized) // Safe @@ -339,7 +339,7 @@ class TestKt { } run { val source: File? = source() as File? - val normalized: String = source!!.getCanonicalFile().toString() + val normalized: String = source!!.canonicalFile.toString() if (!normalized.startsWith("/data")) { sink(source) // Safe sink(normalized) // Safe From 47d61e0b4d355786a47f25197878cb7ba15c173c Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 5 Dec 2022 11:52:50 +0100 Subject: [PATCH 5/9] Add test for File.startsWith --- .../library-tests/pathsanitizer/TestKt.kt | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/java/ql/test/library-tests/pathsanitizer/TestKt.kt b/java/ql/test/library-tests/pathsanitizer/TestKt.kt index ec8ab63a0cf..dc9cf86c8e1 100644 --- a/java/ql/test/library-tests/pathsanitizer/TestKt.kt +++ b/java/ql/test/library-tests/pathsanitizer/TestKt.kt @@ -106,7 +106,18 @@ class TestKt { } run { val source: File? = source() as File? - val normalized: File = source!!.canonicalFile.toString() + val normalized: File = source!!.canonicalFile + if (normalized.startsWith("/safe")) { + sink(source) // Safe + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ hasTaintFlow + } + } + run { + val source: File? = source() as File? + val normalized: String = source!!.canonicalFile.toString() if (normalized.startsWith("/safe")) { sink(source) // Safe sink(normalized) // Safe @@ -337,6 +348,17 @@ class TestKt { sink(normalized) // $ hasTaintFlow } } + run { + val source: File? = source() as File? + val normalized: File = source!!.canonicalFile + if (!normalized.startsWith("/data")) { + sink(source) // Safe + sink(normalized) // Safe + } else { + sink(source) // $ hasTaintFlow + sink(normalized) // $ hasTaintFlow + } + } run { val source: File? = source() as File? val normalized: String = source!!.canonicalFile.toString() From 85b2642a5e1ffba665aa5e21acf443d335e1da6a Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 7 Dec 2022 09:57:31 +0100 Subject: [PATCH 6/9] Extraction discrepancy fixed in kotlinc 1.7.21 --- java/ql/test/library-tests/pathsanitizer/TestKt.kt | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/java/ql/test/library-tests/pathsanitizer/TestKt.kt b/java/ql/test/library-tests/pathsanitizer/TestKt.kt index dc9cf86c8e1..d0a11dac0ee 100644 --- a/java/ql/test/library-tests/pathsanitizer/TestKt.kt +++ b/java/ql/test/library-tests/pathsanitizer/TestKt.kt @@ -20,10 +20,8 @@ class TestKt { fun exactPathMatchGuard() { run { val source = source() as String? - // This gets extracted as Object.equals, which makes the definitions in exactPathMatchGuard not catch it. - // Note that it gets correctly extracted in Java. if (source!!.equals("/safe/path")) - sink(source) // $SPURIOUS: $ hasTaintFlow + sink(source) // Safe else sink(source) // $ hasTaintFlow } From 2f622ad72c4efd9124d4793231073287466da338 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 7 Dec 2022 10:31:54 +0100 Subject: [PATCH 7/9] Refactor by introducing helper predicates --- .../code/java/security/PathSanitizer.qll | 146 +++++++++--------- 1 file changed, 75 insertions(+), 71 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index ad861ddeb42..a8c58560b03 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -49,25 +49,18 @@ private module ValidationMethod { */ private predicate exactPathMatchGuard(Guard g, Expr e, boolean branch) { exists(MethodAccess ma, RefType t | - ( - t instanceof TypeString or - t instanceof TypeUri or - t instanceof TypePath or - t instanceof TypeFile or - t.hasQualifiedName("android.net", "Uri") - ) and - e = ma.getQualifier().getUnderlyingExpr() - or - ( - ma.getMethod().getDeclaringType() instanceof StringsKt - or - ma.getMethod().getDeclaringType() instanceof FilesKt - ) and - e = ma.getArgument(0).getUnderlyingExpr() + t instanceof TypeString or + t instanceof TypeUri or + t instanceof TypePath or + t instanceof TypeFile or + t.hasQualifiedName("android.net", "Uri") or + t instanceof StringsKt or + t instanceof FilesKt | + e = getVisualQualifier(ma).getUnderlyingExpr() and ma.getMethod().getDeclaringType() = t and ma = g and - ma.getMethod().getName() = ["equals", "equalsIgnoreCase"] + ["", "$default"] and + getSourceMethod(ma.getMethod()).hasName(["equals", "equalsIgnoreCase"]) and branch = true ) } @@ -224,29 +217,19 @@ private class ConstantOrRegex extends Expr { } private predicate isStringPrefixMatch(MethodAccess ma, Expr checkedExpr) { - exists(Method m | + exists(Method m, RefType t | + m.getDeclaringType() = t and + (t instanceof TypeString or t instanceof StringsKt) and m = ma.getMethod() and - ( - m.getDeclaringType() instanceof TypeString and - checkedExpr = ma.getQualifier().getUnderlyingExpr() - or - m.getDeclaringType() instanceof StringsKt and - checkedExpr = ma.getArgument(0).getUnderlyingExpr() - ) + checkedExpr = getVisualQualifier(ma).getUnderlyingExpr() | - m.hasName("startsWith" + ["", "$default"]) + getSourceMethod(m).hasName("startsWith") or - exists(int argPos | - m.getDeclaringType() instanceof TypeString and argPos = 0 - or - m.getDeclaringType() instanceof StringsKt and argPos = 1 - | - m.hasName("regionMatches" + ["", "$default"]) and - ma.getArgument(argPos).(CompileTimeConstantExpr).getIntValue() = 0 - or - m.hasName("matches") and - not ma.getArgument(argPos).(ConstantOrRegex).getStringValue().matches(".*%") - ) + getSourceMethod(m).hasName("regionMatches") and + getVisualArgument(ma, 0).(CompileTimeConstantExpr).getIntValue() = 0 + or + m.hasName("matches") and + not getVisualArgument(ma, 0).(ConstantOrRegex).getStringValue().matches(".*%") ) } @@ -256,30 +239,23 @@ private predicate isStringPrefixMatch(MethodAccess ma, Expr checkedExpr) { private predicate isStringPartialMatch(MethodAccess ma, Expr checkedExpr) { isStringPrefixMatch(ma, checkedExpr) or - ( - ma.getMethod().getDeclaringType() instanceof TypeString and - checkedExpr = ma.getQualifier().getUnderlyingExpr() - or - ma.getMethod().getDeclaringType() instanceof StringsKt and - checkedExpr = ma.getArgument(0).getUnderlyingExpr() + exists(RefType t | t = ma.getMethod().getDeclaringType() | + t instanceof TypeString or t instanceof StringsKt ) and - ma.getMethod() - .hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"] + ["", "$default"]) + getSourceMethod(ma.getMethod()) + .hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"]) and + checkedExpr = getVisualQualifier(ma).getUnderlyingExpr() } /** * Holds if `ma` is a call to a method that checks whether `checkedExpr` starts with a prefix. */ private predicate isPathPrefixMatch(MethodAccess ma, Expr checkedExpr) { - exists(RefType t | - t instanceof TypePath and checkedExpr = ma.getQualifier().getUnderlyingExpr() - or - t instanceof FilesKt and - checkedExpr = ma.getArgument(0).getUnderlyingExpr() - | - t = ma.getMethod().getDeclaringType() and - ma.getMethod().hasName("startsWith" + ["", "$default"]) - ) + exists(RefType t | t = ma.getMethod().getDeclaringType() | + t instanceof TypePath or t instanceof FilesKt + ) and + getSourceMethod(ma.getMethod()).hasName("startsWith") and + checkedExpr = getVisualQualifier(ma) } private predicate isDisallowedWord(ConstantOrRegex word) { @@ -291,22 +267,19 @@ private class PathTraversalGuard extends PathGuard { Expr checkedExpr; PathTraversalGuard() { - exists(MethodAccess ma | - ( - ma.getMethod().getDeclaringType() instanceof TypeString and - checkedExpr = ma.getQualifier().getUnderlyingExpr() - or - ma.getMethod().getDeclaringType() instanceof StringsKt and - checkedExpr = ma.getArgument(0).getUnderlyingExpr() - ) and + exists(MethodAccess ma, Method m, RefType t | + m = ma.getMethod() and + t = m.getDeclaringType() and + (t instanceof TypeString or t instanceof StringsKt) and + checkedExpr = getVisualQualifier(ma).getUnderlyingExpr() and ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." | this = ma and - ma.getMethod().hasName("contains" + ["", "$default"]) + getSourceMethod(m).hasName("contains") or exists(EqualityTest eq | this = eq and - ma.getMethod().hasName(["indexOf", "lastIndexOf"] + ["", "$default"]) and + getSourceMethod(m).hasName(["indexOf", "lastIndexOf"]) and eq.getAnOperand() = ma and eq.getAnOperand().(CompileTimeConstantExpr).getIntValue() = -1 ) @@ -325,15 +298,46 @@ private class PathTraversalGuard extends PathGuard { /** A complementary sanitizer that protects against path traversal using path normalization. */ private class PathNormalizeSanitizer extends MethodAccess { PathNormalizeSanitizer() { - exists(RefType t | - t instanceof TypePath or - t instanceof FilesKt - | - this.getMethod().getDeclaringType() = t and + exists(RefType t | this.getMethod().getDeclaringType() = t | + (t instanceof TypePath or t instanceof FilesKt) and this.getMethod().hasName("normalize") + or + t instanceof TypeFile and + this.getMethod().hasName(["getCanonicalPath", "getCanonicalFile"]) ) - or - this.getMethod().getDeclaringType() instanceof TypeFile and - this.getMethod().hasName(["getCanonicalPath", "getCanonicalFile"]) } } + +/** + * Gets the qualifier of `ma` as seen in the source code. + * This is a helper predicate to solve discrepancies between + * what `getQualifier` actually gets in Java and Kotlin. + */ +private Expr getVisualQualifier(MethodAccess ma) { + if getSourceMethod(ma.getMethod()) instanceof ExtensionMethod + then result = ma.getArgument(0) + else result = ma.getQualifier() +} + +/** + * Gets the argument of `ma` at position `argPos` as seen in the source code. + * This is a helper predicate to solve discrepancies between + * what `getArgument` actually gets in Java and Kotlin. + */ +bindingset[argPos] +private Argument getVisualArgument(MethodAccess ma, int argPos) { + if getSourceMethod(ma.getMethod()) instanceof ExtensionMethod + then result = ma.getArgument(argPos + 1) + else result = ma.getArgument(argPos) +} + +/** + * Gets the proxied method if `m` is a Kotlin proxy that supplies default parameter values, + * Otherwise, just gets `m`. + */ +private Method getSourceMethod(Method m) { + m = result.getKotlinParameterDefaultsProxy() + or + not exists(Method src | m = src.getKotlinParameterDefaultsProxy()) and + result = m +} From ccd465d669881a11b20e837221a22cfb41a32406 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 7 Dec 2022 10:38:33 +0100 Subject: [PATCH 8/9] Update java/ql/lib/semmle/code/java/security/PathSanitizer.qll --- java/ql/lib/semmle/code/java/security/PathSanitizer.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index a8c58560b03..77dbf26b8db 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -332,7 +332,7 @@ private Argument getVisualArgument(MethodAccess ma, int argPos) { } /** - * Gets the proxied method if `m` is a Kotlin proxy that supplies default parameter values, + * Gets the proxied method if `m` is a Kotlin proxy that supplies default parameter values. * Otherwise, just gets `m`. */ private Method getSourceMethod(Method m) { From 6dcc0cc188dd4a0b4a922f81a27fdbdc73a2b96b Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 7 Dec 2022 10:50:23 +0100 Subject: [PATCH 9/9] Further simplification --- .../code/java/security/PathSanitizer.qll | 41 ++++++------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index 77dbf26b8db..76661aa4842 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -86,18 +86,12 @@ private predicate localTaintFlowToPathGuard(Expr e, PathGuard g) { } private class AllowedPrefixGuard extends PathGuard instanceof MethodAccess { - Expr checkedExpr; - AllowedPrefixGuard() { - ( - isStringPrefixMatch(this, checkedExpr) - or - isPathPrefixMatch(this, checkedExpr) - ) and + (isStringPrefixMatch(this) or isPathPrefixMatch(this)) and not isDisallowedWord(super.getAnArgument()) } - override Expr getCheckedExpr() { result = checkedExpr } + override Expr getCheckedExpr() { result = getVisualQualifier(this).getUnderlyingExpr() } } /** @@ -159,18 +153,12 @@ private class DotDotCheckSanitizer extends PathInjectionSanitizer { } private class BlockListGuard extends PathGuard instanceof MethodAccess { - Expr checkedExpr; - BlockListGuard() { - ( - isStringPartialMatch(this, checkedExpr) - or - isPathPrefixMatch(this, checkedExpr) - ) and + (isStringPartialMatch(this) or isPathPrefixMatch(this)) and isDisallowedWord(super.getAnArgument()) } - override Expr getCheckedExpr() { result = checkedExpr } + override Expr getCheckedExpr() { result = getVisualQualifier(this).getUnderlyingExpr() } } /** @@ -216,12 +204,11 @@ private class ConstantOrRegex extends Expr { } } -private predicate isStringPrefixMatch(MethodAccess ma, Expr checkedExpr) { +private predicate isStringPrefixMatch(MethodAccess ma) { exists(Method m, RefType t | m.getDeclaringType() = t and (t instanceof TypeString or t instanceof StringsKt) and - m = ma.getMethod() and - checkedExpr = getVisualQualifier(ma).getUnderlyingExpr() + m = ma.getMethod() | getSourceMethod(m).hasName("startsWith") or @@ -234,28 +221,26 @@ private predicate isStringPrefixMatch(MethodAccess ma, Expr checkedExpr) { } /** - * Holds if `ma` is a call to a method that checks a partial string match on `checkedExpr`. + * Holds if `ma` is a call to a method that checks a partial string match. */ -private predicate isStringPartialMatch(MethodAccess ma, Expr checkedExpr) { - isStringPrefixMatch(ma, checkedExpr) +private predicate isStringPartialMatch(MethodAccess ma) { + isStringPrefixMatch(ma) or exists(RefType t | t = ma.getMethod().getDeclaringType() | t instanceof TypeString or t instanceof StringsKt ) and getSourceMethod(ma.getMethod()) - .hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"]) and - checkedExpr = getVisualQualifier(ma).getUnderlyingExpr() + .hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"]) } /** - * Holds if `ma` is a call to a method that checks whether `checkedExpr` starts with a prefix. + * Holds if `ma` is a call to a method that checks whether a path starts with a prefix. */ -private predicate isPathPrefixMatch(MethodAccess ma, Expr checkedExpr) { +private predicate isPathPrefixMatch(MethodAccess ma) { exists(RefType t | t = ma.getMethod().getDeclaringType() | t instanceof TypePath or t instanceof FilesKt ) and - getSourceMethod(ma.getMethod()).hasName("startsWith") and - checkedExpr = getVisualQualifier(ma) + getSourceMethod(ma.getMethod()).hasName("startsWith") } private predicate isDisallowedWord(ConstantOrRegex word) {