From 6db0db431fd094ee667660232006ae10f113ab76 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 5 Oct 2022 12:02:05 +0200 Subject: [PATCH] Java: Add pruning for local taint flow. --- .../dataflow/internal/TaintTrackingUtil.qll | 51 +++++++++++++++++++ .../code/java/security/PathSanitizer.qll | 38 +++++++++----- 2 files changed, 76 insertions(+), 13 deletions(-) diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll b/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll index d52b340e67a..a0acc69cbff 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll @@ -33,6 +33,57 @@ predicate localExprTaint(Expr src, Expr sink) { localTaint(DataFlow::exprNode(src), DataFlow::exprNode(sink)) } +/** Holds if `node` is an endpoint for local taint flow. */ +signature predicate nodeSig(DataFlow::Node node); + +/** Provides local taint flow restricted to a given set of sources and sinks. */ +module LocalTaintFlow { + private predicate reachRev(DataFlow::Node n) { + sink(n) + or + exists(DataFlow::Node mid | + localTaintStep(n, mid) and + reachRev(mid) + ) + } + + private predicate reachFwd(DataFlow::Node n) { + reachRev(n) and + ( + source(n) + or + exists(DataFlow::Node mid | + localTaintStep(mid, n) and + reachFwd(mid) + ) + ) + } + + private predicate step(DataFlow::Node n1, DataFlow::Node n2) { + localTaintStep(n1, n2) and + reachFwd(n1) and + reachFwd(n2) + } + + /** + * Holds if taint can flow from `n1` to `n2` in zero or more local + * (intra-procedural) steps that are restricted to be part of a path between + * `source` and `sink`. + */ + pragma[inline] + predicate hasFlow(DataFlow::Node n1, DataFlow::Node n2) { step*(n1, n2) } + + /** + * Holds if taint can flow from `n1` to `n2` in zero or more local + * (intra-procedural) steps that are restricted to be part of a path between + * `source` and `sink`. + */ + pragma[inline] + predicate hasExprFlow(Expr n1, Expr n2) { + hasFlow(DataFlow::exprNode(n1), DataFlow::exprNode(n2)) + } +} + cached private module Cached { private import DataFlowImplCommon as DataFlowImplCommon diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index 92e429dc2c2..bd15b0581b7 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -69,13 +69,25 @@ private class ExactPathMatchSanitizer extends PathInjectionSanitizer { } } -private class AllowedPrefixGuard extends Guard instanceof MethodAccess { +abstract private class PathGuard extends Guard { + abstract Expr getCheckedExpr(); +} + +private predicate anyNode(DataFlow::Node n) { any() } + +private predicate pathGuardNode(DataFlow::Node n) { n.asExpr() = any(PathGuard g).getCheckedExpr() } + +private predicate localTaintFlowToPathGuard(Expr e, PathGuard g) { + TaintTracking::LocalTaintFlow::hasExprFlow(e, g.getCheckedExpr()) +} + +private class AllowedPrefixGuard extends PathGuard instanceof MethodAccess { AllowedPrefixGuard() { (isStringPrefixMatch(this) or isPathPrefixMatch(this)) and not isDisallowedWord(super.getAnArgument()) } - Expr getCheckedExpr() { result = super.getQualifier() } + override Expr getCheckedExpr() { result = super.getQualifier() } } /** @@ -91,10 +103,10 @@ private predicate allowedPrefixGuard(Guard g, Expr e, boolean branch) { // String strPath = file.getCanonicalPath(); // if (strPath.startsWith("/safe/dir")) // sink(file); - TaintTracking::localExprTaint(e, g.(AllowedPrefixGuard).getCheckedExpr()) and + g instanceof AllowedPrefixGuard and + localTaintFlowToPathGuard(e, g) and exists(Expr previousGuard | - TaintTracking::localExprTaint(previousGuard.(PathNormalizeSanitizer), - g.(AllowedPrefixGuard).getCheckedExpr()) + localTaintFlowToPathGuard(previousGuard.(PathNormalizeSanitizer), g) or previousGuard .(PathTraversalGuard) @@ -121,7 +133,7 @@ private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) { // if (!strPath.contains("..") && strPath.startsWith("/safe/dir")) // sink(path); branch = g.(PathTraversalGuard).getBranch() and - TaintTracking::localExprTaint(e, g.(PathTraversalGuard).getCheckedExpr()) and + localTaintFlowToPathGuard(e, g) and exists(Guard previousGuard | previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true) or @@ -136,13 +148,13 @@ private class DotDotCheckSanitizer extends PathInjectionSanitizer { } } -private class BlockListGuard extends Guard instanceof MethodAccess { +private class BlockListGuard extends PathGuard instanceof MethodAccess { BlockListGuard() { (isStringPartialMatch(this) or isPathPrefixMatch(this)) and isDisallowedWord(super.getAnArgument()) } - Expr getCheckedExpr() { result = super.getQualifier() } + override Expr getCheckedExpr() { result = super.getQualifier() } } /** @@ -158,10 +170,10 @@ private predicate blockListGuard(Guard g, Expr e, boolean branch) { // String strPath = file.getCanonicalPath(); // if (!strPath.contains("..") && !strPath.startsWith("/dangerous/dir")) // sink(file); - TaintTracking::localExprTaint(e, g.(BlockListGuard).getCheckedExpr()) and + g instanceof BlockListGuard and + localTaintFlowToPathGuard(e, g) and exists(Expr previousGuard | - TaintTracking::localExprTaint(previousGuard.(PathNormalizeSanitizer), - g.(BlockListGuard).getCheckedExpr()) + localTaintFlowToPathGuard(previousGuard.(PathNormalizeSanitizer), g) or previousGuard .(PathTraversalGuard) @@ -217,7 +229,7 @@ private predicate isDisallowedWord(CompileTimeConstantExpr word) { } /** A complementary guard that protects against path traversal, by looking for the literal `..`. */ -private class PathTraversalGuard extends Guard { +private class PathTraversalGuard extends PathGuard { PathTraversalGuard() { exists(MethodAccess ma | ma.getMethod().getDeclaringType() instanceof TypeString and @@ -235,7 +247,7 @@ private class PathTraversalGuard extends Guard { ) } - Expr getCheckedExpr() { + override Expr getCheckedExpr() { exists(MethodAccess ma | ma = this.(EqualityTest).getAnOperand() or ma = this | result = ma.getQualifier() )