Adapt PathSanitizer to Kotlin

This commit is contained in:
Tony Torralba
2022-12-02 18:14:42 +01:00
parent 6bffb11749
commit 21b51b48eb
6 changed files with 610 additions and 40 deletions

View File

@@ -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") }
}

View File

@@ -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()
}
}

View File

@@ -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<DataFlow::guardChecksSig/3 validationGuard> {
*/
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()
|
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.hasName("regionMatches") and
ma.getArgument(0).(CompileTimeConstantExpr).getIntValue() = 0
m.getDeclaringType() instanceof StringsKt and
checkedExpr = ma.getArgument(0).getUnderlyingExpr()
)
|
m.hasName("startsWith" + ["", "$default"])
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(0).(CompileTimeConstantExpr).getStringValue().matches(".*%")
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"])
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
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")

View File

@@ -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();
}

View File

@@ -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
}
}
}
}

View File

@@ -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