diff --git a/java/ql/lib/semmle/code/java/security/Files.qll b/java/ql/lib/semmle/code/java/security/Files.qll index d4326a6faad..2cc55a1076f 100644 --- a/java/ql/lib/semmle/code/java/security/Files.qll +++ b/java/ql/lib/semmle/code/java/security/Files.qll @@ -82,6 +82,7 @@ private class FileSummaryModels extends SummaryModelCsv { "java.io;File;true;getCanonicalFile;;;Argument[-1];ReturnValue;taint;manual", "java.io;File;true;getCanonicalPath;;;Argument[-1];ReturnValue;taint;manual", "java.io;File;true;toPath;;;Argument[-1];ReturnValue;taint;manual", + "java.io;File;true;toString;;;Argument[-1];ReturnValue;taint;manual", "java.io;File;true;toURI;;;Argument[-1];ReturnValue;taint;manual", "java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint;manual", "java.nio.file;Path;true;resolve;;;Argument[-1..0];ReturnValue;taint;manual", diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index cd2c7527ff9..4f833d48268 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -72,7 +72,8 @@ private class ExactPathMatchSanitizer extends PathInjectionSanitizer { private class AllowListGuard extends Guard instanceof MethodAccess { AllowListGuard() { (isStringPrefixMatch(this) or isPathPrefixMatch(this)) and - not isDisallowedPrefix(super.getAnArgument()) + not isDisallowedPrefix(super.getAnArgument()) and + not isDisallowedWord(super.getAnArgument()) } Expr getCheckedExpr() { result = super.getQualifier() } @@ -86,11 +87,13 @@ private class AllowListGuard extends Guard instanceof MethodAccess { private predicate allowListGuard(Guard g, Expr e, boolean branch) { branch = true and TaintTracking::localExprTaint(e, g.(AllowListGuard).getCheckedExpr()) and - exists(MethodAccess previousGuard | + exists(Expr previousGuard | TaintTracking::localExprTaint(previousGuard.(PathNormalizeSanitizer), g.(AllowListGuard).getCheckedExpr()) or - previousGuard.(PathTraversalGuard).controls(g.getBasicBlock().(ConditionBlock), false) + previousGuard + .(PathTraversalGuard) + .controls(g.getBasicBlock().(ConditionBlock), previousGuard.(PathTraversalGuard).getBranch()) ) } @@ -106,9 +109,9 @@ private class AllowListSanitizer extends PathInjectionSanitizer { * been checked for a trusted prefix. */ private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) { - branch = false and + branch = g.(PathTraversalGuard).getBranch() and TaintTracking::localExprTaint(e, g.(PathTraversalGuard).getCheckedExpr()) and - exists(MethodAccess previousGuard | + exists(Guard previousGuard | previousGuard.(AllowListGuard).controls(g.getBasicBlock().(ConditionBlock), true) or previousGuard.(BlockListGuard).controls(g.getBasicBlock().(ConditionBlock), false) @@ -142,11 +145,13 @@ private class BlockListGuard extends Guard instanceof MethodAccess { private predicate blockListGuard(Guard g, Expr e, boolean branch) { branch = false and TaintTracking::localExprTaint(e, g.(BlockListGuard).getCheckedExpr()) and - exists(MethodAccess previousGuard | + exists(Expr previousGuard | TaintTracking::localExprTaint(previousGuard.(PathNormalizeSanitizer), g.(BlockListGuard).getCheckedExpr()) or - previousGuard.(PathTraversalGuard).controls(g.getBasicBlock().(ConditionBlock), false) + previousGuard + .(PathTraversalGuard) + .controls(g.getBasicBlock().(ConditionBlock), previousGuard.(PathTraversalGuard).getBranch()) ) } @@ -200,14 +205,35 @@ private predicate isDisallowedWord(CompileTimeConstantExpr word) { } /** A complementary guard that protects against path traversal, by looking for the literal `..`. */ -private class PathTraversalGuard extends Guard instanceof MethodAccess { +private class PathTraversalGuard extends Guard { PathTraversalGuard() { - super.getMethod().getDeclaringType() instanceof TypeString and - super.getMethod().hasName(["contains", "indexOf"]) and - super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." + exists(MethodAccess ma | + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." + | + this = ma and + ma.getMethod().hasName("contains") + or + exists(EqualityTest eq | + this = eq and + ma.getMethod().hasName(["indexOf", "lastIndexOf"]) and + eq.getAnOperand() = ma and + eq.getAnOperand().(CompileTimeConstantExpr).getIntValue() = -1 + ) + ) } - Expr getCheckedExpr() { result = super.getQualifier() } + Expr getCheckedExpr() { + exists(MethodAccess ma | ma = this.(BinaryExpr).getAnOperand() or ma = this | + result = ma.getQualifier() + ) + } + + boolean getBranch() { + this instanceof MethodAccess and result = false + or + result = this.(EqualityTest).polarity() + } } /** A complementary sanitizer that protects against path traversal using path normalization. */ diff --git a/java/ql/test/library-tests/pathsanitizer/Test.java b/java/ql/test/library-tests/pathsanitizer/Test.java new file mode 100644 index 00000000000..69334125a46 --- /dev/null +++ b/java/ql/test/library-tests/pathsanitizer/Test.java @@ -0,0 +1,459 @@ +import java.io.File; +import java.net.URI; +import java.nio.file.Path; +import java.nio.file.Paths; +import android.net.Uri; + +public class Test { + + Object source() { + return null; + } + + void sink(Object o) {} + + private void exactPathMatchGuardValidation(String path) throws Exception { + if (!path.equals("/safe/path")) + throw new Exception(); + } + + public void exactPathMatchGuard() throws Exception { + { + String source = (String) source(); + if (source.equals("/safe/path")) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + { + URI source = (URI) source(); + if (source.equals(new URI("http://safe/uri"))) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + { + File source = (File) source(); + if (source.equals(new File("/safe/file"))) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + { + Uri source = (Uri) source(); + if (source.equals(Uri.parse("http://safe/uri"))) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + { + String source = (String) source(); + exactPathMatchGuardValidation(source); + sink(source); // Safe + } + } + + private void allowListGuardValidation(String path) throws Exception { + if (path.contains("..") || !path.startsWith("/safe")) + throw new Exception(); + } + + public void allowListGuard() throws Exception { + // Prefix check by itself is not enough + { + String source = (String) source(); + if (source.startsWith("/safe")) { + sink(source); // $ hasTaintFlow + } else + sink(source); // $ hasTaintFlow + } + // PathTraversalGuard + allowListGuard + { + String source = (String) source(); + if (!source.contains("..") && source.startsWith("/safe")) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + { + String source = (String) source(); + if (source.indexOf("..") == -1 && source.startsWith("/safe")) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + { + String source = (String) source(); + if (source.lastIndexOf("..") == -1 && source.startsWith("/safe")) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + // PathTraversalSanitizer + allowListGuard + { + File source = (File) source(); + String normalized = source.getCanonicalPath(); + if (normalized.startsWith("/safe")) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + File source = (File) source(); + String normalized = source.getCanonicalFile().toString(); + if (normalized.startsWith("/safe")) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + Path normalized = Paths.get(source).normalize(); + if (normalized.startsWith("/safe")) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (normalized.startsWith("/safe")) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (normalized.regionMatches(0, "/safe", 0, 5)) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (normalized.matches("/safe/.*")) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + // validation method + { + String source = (String) source(); + allowListGuardValidation(source); + sink(source); // Safe + } + // PathInjectionSanitizer + partial string match is considered unsafe + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (normalized.contains("/safe")) { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (normalized.regionMatches(1, "/safe", 0, 5)) { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (normalized.matches(".*/safe/.*")) { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + } + + private void dotDotCheckGuardValidation(String path) throws Exception { + if (!path.startsWith("/safe") || path.contains("..")) + throw new Exception(); + } + + public void dotDotCheckGuard() throws Exception { + // dot dot check by itself is not enough + { + String source = (String) source(); + if (!source.contains("..")) { + sink(source); // $ hasTaintFlow + } else + sink(source); // $ hasTaintFlow + } + // allowListGuard + dotDotCheckGuard + { + String source = (String) source(); + if (source.startsWith("/safe") && !source.contains("..")) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + { + String source = (String) source(); + if (source.startsWith("/safe") && source.indexOf("..") == -1) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + { + String source = (String) source(); + if (!source.startsWith("/safe") || source.indexOf("..") != -1) + sink(source); // $ hasTaintFlow + else + sink(source); // Safe + } + { + String source = (String) source(); + if (source.startsWith("/safe") && source.lastIndexOf("..") == -1) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + // blockListGuard + dotDotCheckGuard + { + String source = (String) source(); + if (!source.startsWith("/data") && !source.contains("..")) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + { + String source = (String) source(); + if (!source.startsWith("/data") && source.indexOf("..") == -1) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + { + String source = (String) source(); + if (!source.startsWith("/data") && source.indexOf("..") == -1) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + { + String source = (String) source(); + if (!source.startsWith("/data") && source.lastIndexOf("..") == -1) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + // validation method + { + String source = (String) source(); + dotDotCheckGuardValidation(source); + sink(source); // Safe + } + } + + private void blockListGuardValidation(String path) throws Exception { + if (path.contains("..") || !path.startsWith("/data")) + throw new Exception(); + } + + public void blockListGuard() throws Exception { + // Prefix check by itself is not enough + { + String source = (String) source(); + if (!source.startsWith("/data")) { + sink(source); // $ hasTaintFlow + } else + sink(source); // $ hasTaintFlow + } + // PathTraversalGuard + blockListGuard + { + String source = (String) source(); + if (!source.contains("..") && !source.startsWith("/data")) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + { + String source = (String) source(); + if (source.indexOf("..") == -1 && !source.startsWith("/data")) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + { + String source = (String) source(); + if (source.lastIndexOf("..") == -1 && !source.startsWith("/data")) + sink(source); // Safe + else + sink(source); // $ hasTaintFlow + } + // PathTraversalSanitizer + blockListGuard + { + File source = (File) source(); + String normalized = source.getCanonicalPath(); + if (!normalized.startsWith("/data")) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + File source = (File) source(); + String normalized = source.getCanonicalFile().toString(); + if (!normalized.startsWith("/data")) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + Path normalized = Paths.get(source).normalize(); + if (!normalized.startsWith("/data")) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (!normalized.startsWith("/data")) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (!normalized.regionMatches(0, "/data", 0, 5)) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (!normalized.matches("/data/.*")) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + // validation method + { + String source = (String) source(); + blockListGuardValidation(source); + sink(source); // Safe + } + // PathInjectionSanitizer + partial string match with disallowed words + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (!normalized.contains("/")) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (!normalized.regionMatches(1, "/", 0, 5)) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (!normalized.matches("/")) { + sink(source); // Safe + sink(normalized); // Safe + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + // PathInjectionSanitizer + partial string match with prefixes is considered unsafe + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (normalized.contains("/data")) { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (normalized.regionMatches(1, "/data", 0, 5)) { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (normalized.matches(".*/data/.*")) { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } else { + sink(source); // $ hasTaintFlow + sink(normalized); // $ hasTaintFlow + } + } + } +} diff --git a/java/ql/test/library-tests/pathsanitizer/options b/java/ql/test/library-tests/pathsanitizer/options new file mode 100644 index 00000000000..1e6e9ea2bf1 --- /dev/null +++ b/java/ql/test/library-tests/pathsanitizer/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -cp ${testdir}/../../stubs/google-android-9.0.0 diff --git a/java/ql/test/library-tests/pathsanitizer/test.expected b/java/ql/test/library-tests/pathsanitizer/test.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/java/ql/test/library-tests/pathsanitizer/test.ql b/java/ql/test/library-tests/pathsanitizer/test.ql new file mode 100644 index 00000000000..c3b8ca70367 --- /dev/null +++ b/java/ql/test/library-tests/pathsanitizer/test.ql @@ -0,0 +1,15 @@ +import java +import semmle.code.java.security.PathSanitizer +import TestUtilities.InlineFlowTest + +class PathSanitizerConf extends DefaultTaintFlowConf { + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer instanceof PathInjectionSanitizer + } +} + +class Test extends InlineFlowTest { + override DataFlow::Configuration getValueFlowConfig() { none() } + + override DataFlow::Configuration getTaintFlowConfig() { result = any(PathSanitizerConf config) } +}