diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index 0eebbe48f50..92e429dc2c2 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -85,6 +85,12 @@ private class AllowedPrefixGuard extends Guard instanceof MethodAccess { */ private predicate allowedPrefixGuard(Guard g, Expr e, boolean branch) { branch = true and + // Local taint-flow is used here to handle cases where the validated expression comes from the + // expression reaching the sink, but it's not the same one, e.g.: + // File file = source(); + // String strPath = file.getCanonicalPath(); + // if (strPath.startsWith("/safe/dir")) + // sink(file); TaintTracking::localExprTaint(e, g.(AllowedPrefixGuard).getCheckedExpr()) and exists(Expr previousGuard | TaintTracking::localExprTaint(previousGuard.(PathNormalizeSanitizer), @@ -92,7 +98,7 @@ private predicate allowedPrefixGuard(Guard g, Expr e, boolean branch) { or previousGuard .(PathTraversalGuard) - .controls(g.getBasicBlock().(ConditionBlock), previousGuard.(PathTraversalGuard).getBranch()) + .controls(g.getBasicBlock(), previousGuard.(PathTraversalGuard).getBranch()) ) } @@ -108,12 +114,18 @@ private class AllowedPrefixSanitizer extends PathInjectionSanitizer { * been checked for a trusted prefix. */ private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) { + // Local taint-flow is used here to handle cases where the validated expression comes from the + // expression reaching the sink, but it's not the same one, e.g.: + // Path path = source(); + // String strPath = path.toString(); + // if (!strPath.contains("..") && strPath.startsWith("/safe/dir")) + // sink(path); branch = g.(PathTraversalGuard).getBranch() and TaintTracking::localExprTaint(e, g.(PathTraversalGuard).getCheckedExpr()) and exists(Guard previousGuard | - previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock().(ConditionBlock), true) + previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true) or - previousGuard.(BlockListGuard).controls(g.getBasicBlock().(ConditionBlock), false) + previousGuard.(BlockListGuard).controls(g.getBasicBlock(), false) ) } @@ -140,6 +152,12 @@ private class BlockListGuard extends Guard instanceof MethodAccess { */ private predicate blockListGuard(Guard g, Expr e, boolean branch) { branch = false and + // Local taint-flow is used here to handle cases where the validated expression comes from the + // expression reaching the sink, but it's not the same one, e.g.: + // File file = source(); + // String strPath = file.getCanonicalPath(); + // if (!strPath.contains("..") && !strPath.startsWith("/dangerous/dir")) + // sink(file); TaintTracking::localExprTaint(e, g.(BlockListGuard).getCheckedExpr()) and exists(Expr previousGuard | TaintTracking::localExprTaint(previousGuard.(PathNormalizeSanitizer), @@ -147,7 +165,7 @@ private predicate blockListGuard(Guard g, Expr e, boolean branch) { or previousGuard .(PathTraversalGuard) - .controls(g.getBasicBlock().(ConditionBlock), previousGuard.(PathTraversalGuard).getBranch()) + .controls(g.getBasicBlock(), previousGuard.(PathTraversalGuard).getBranch()) ) } @@ -245,10 +263,3 @@ private class PathNormalizeSanitizer extends MethodAccess { this.getMethod().hasName(["getCanonicalPath", "getCanonicalFile"]) } } - -/** A node with path normalization. */ -class NormalizedPathNode extends DataFlow::Node { - NormalizedPathNode() { - TaintTracking::localExprTaint(this.asExpr(), any(PathNormalizeSanitizer ma)) - } -} diff --git a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql index b7ce4c330be..fff9a3b3613 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql @@ -18,6 +18,29 @@ import JFinalController import semmle.code.java.security.PathSanitizer import DataFlow::PathGraph +/** A complementary sanitizer that protects against path traversal using path normalization. */ +class PathNormalizeSanitizer extends MethodAccess { + PathNormalizeSanitizer() { + exists(RefType t | + t instanceof TypePath or + t.hasQualifiedName("kotlin.io", "FilesKt") + | + this.getMethod().getDeclaringType() = t and + this.getMethod().hasName("normalize") + ) + or + this.getMethod().getDeclaringType() instanceof TypeFile and + this.getMethod().hasName(["getCanonicalPath", "getCanonicalFile"]) + } +} + +/** A node with path normalization. */ +class NormalizedPathNode extends DataFlow::Node { + NormalizedPathNode() { + TaintTracking::localExprTaint(this.asExpr(), any(PathNormalizeSanitizer ma)) + } +} + class InjectFilePathConfig extends TaintTracking::Configuration { InjectFilePathConfig() { this = "InjectFilePathConfig" }