diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index 5dada4f8532b..a235f43362da 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -361,19 +361,7 @@ private class SummaryModelCsvBase extends SummaryModelCsv { "java.net;URI;false;toURL;;;Argument[-1];ReturnValue;taint;manual", "java.net;URI;false;toString;;;Argument[-1];ReturnValue;taint;manual", "java.net;URI;false;toAsciiString;;;Argument[-1];ReturnValue;taint;manual", - "java.io;File;true;toURI;;;Argument[-1];ReturnValue;taint;manual", - "java.io;File;true;toPath;;;Argument[-1];ReturnValue;taint;manual", - "java.io;File;true;getAbsoluteFile;;;Argument[-1];ReturnValue;taint;manual", - "java.io;File;true;getCanonicalFile;;;Argument[-1];ReturnValue;taint;manual", - "java.io;File;true;getAbsolutePath;;;Argument[-1];ReturnValue;taint;manual", - "java.io;File;true;getCanonicalPath;;;Argument[-1];ReturnValue;taint;manual", "java.nio;ByteBuffer;false;array;();;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", - "java.nio.file;Path;false;toFile;;;Argument[-1];ReturnValue;taint;manual", - "java.nio.file;Path;true;toString;;;Argument[-1];ReturnValue;taint;manual", - "java.nio.file;Path;true;toUri;;;Argument[-1];ReturnValue;taint;manual", - "java.nio.file;Paths;true;get;;;Argument[0..1];ReturnValue;taint;manual", "java.io;BufferedReader;true;readLine;;;Argument[-1];ReturnValue;taint;manual", "java.io;Reader;true;read;();;Argument[-1];ReturnValue;taint;manual", // arg to return @@ -400,8 +388,6 @@ private class SummaryModelCsvBase extends SummaryModelCsv { // arg to arg "java.lang;System;false;arraycopy;;;Argument[0];Argument[2];taint;manual", // constructor flow - "java.io;File;false;File;;;Argument[0];Argument[-1];taint;manual", - "java.io;File;false;File;;;Argument[1];Argument[-1];taint;manual", "java.net;URI;false;URI;(String);;Argument[0];Argument[-1];taint;manual", "java.net;URL;false;URL;(String);;Argument[0];Argument[-1];taint;manual", "javax.xml.transform.stream;StreamSource;false;StreamSource;;;Argument[0];Argument[-1];taint;manual", 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 d52b340e67a1..a0acc69cbffb 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/Files.qll b/java/ql/lib/semmle/code/java/security/Files.qll index 3d783b93d305..2cc55a1076fa 100644 --- a/java/ql/lib/semmle/code/java/security/Files.qll +++ b/java/ql/lib/semmle/code/java/security/Files.qll @@ -70,3 +70,29 @@ private class WriteFileSinkModels extends SinkModelCsv { ] } } + +private class FileSummaryModels extends SummaryModelCsv { + override predicate row(string row) { + row = + [ + "java.io;File;false;File;;;Argument[0];Argument[-1];taint;manual", + "java.io;File;false;File;;;Argument[1];Argument[-1];taint;manual", + "java.io;File;true;getAbsoluteFile;;;Argument[-1];ReturnValue;taint;manual", + "java.io;File;true;getAbsolutePath;;;Argument[-1];ReturnValue;taint;manual", + "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", + "java.nio.file;Path;true;toAbsolutePath;;;Argument[-1];ReturnValue;taint;manual", + "java.nio.file;Path;false;toFile;;;Argument[-1];ReturnValue;taint;manual", + "java.nio.file;Path;true;toString;;;Argument[-1];ReturnValue;taint;manual", + "java.nio.file;Path;true;toUri;;;Argument[-1];ReturnValue;taint;manual", + "java.nio.file;Paths;true;get;;;Argument[0..1];ReturnValue;taint;manual", + "java.nio.file;FileSystem;true;getPath;;;Argument[0];ReturnValue;taint;manual", + "java.nio.file;FileSystem;true;getRootDirectories;;;Argument[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 new file mode 100644 index 000000000000..bd15b0581b75 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -0,0 +1,277 @@ +/** Provides classes and predicates to reason about sanitization of path injection vulnerabilities. */ + +import java +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 + +/** A sanitizer that protects against path injection vulnerabilities. */ +abstract class PathInjectionSanitizer extends DataFlow::Node { } + +/** + * Provides a set of nodes validated by a method that uses a validation guard. + */ +private module ValidationMethod { + /** Gets a node that is safely guarded by a method that uses the given guard check. */ + DataFlow::Node getAValidatedNode() { + exists(MethodAccess ma, int pos, RValue rv | + validationMethod(ma.getMethod(), pos) and + ma.getArgument(pos) = rv and + adjacentUseUseSameVar(rv, result.asExpr()) and + ma.getBasicBlock().bbDominates(result.asExpr().getBasicBlock()) + ) + } + + /** + * Holds if `m` validates its `arg`th parameter by using `validationGuard`. + */ + private predicate validationMethod(Method m, int arg) { + exists( + Guard g, SsaImplicitInit var, ControlFlowNode exit, ControlFlowNode normexit, boolean branch + | + validationGuard(g, var.getAUse(), branch) and + var.isParameterDefinition(m.getParameter(arg)) and + exit = m and + normexit.getANormalSuccessor() = exit and + 1 = strictcount(ControlFlowNode n | n.getANormalSuccessor() = exit) + | + g.(ConditionNode).getABranchSuccessor(branch) = exit or + g.controls(normexit.getBasicBlock(), branch) + ) + } +} + +/** + * Holds if `g` is guard that compares a path to a trusted value. + */ +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") + | + ma.getMethod().getDeclaringType() = t and + ma = g and + ma.getMethod().getName() = ["equals", "equalsIgnoreCase"] and + e = ma.getQualifier() and + branch = true + ) +} + +private class ExactPathMatchSanitizer extends PathInjectionSanitizer { + ExactPathMatchSanitizer() { + this = DataFlow::BarrierGuard::getABarrierNode() + or + this = ValidationMethod::getAValidatedNode() + } +} + +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()) + } + + override Expr getCheckedExpr() { result = super.getQualifier() } +} + +/** + * Holds if `g` is a guard that considers a path safe because it is checked against trusted prefixes. + * This requires additional protection against path traversal, either another guard (`PathTraversalGuard`) + * or a sanitizer (`PathNormalizeSanitizer`), to ensure any internal `..` components are removed from the path. + */ +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); + g instanceof AllowedPrefixGuard and + localTaintFlowToPathGuard(e, g) and + exists(Expr previousGuard | + localTaintFlowToPathGuard(previousGuard.(PathNormalizeSanitizer), g) + or + previousGuard + .(PathTraversalGuard) + .controls(g.getBasicBlock(), previousGuard.(PathTraversalGuard).getBranch()) + ) +} + +private class AllowedPrefixSanitizer extends PathInjectionSanitizer { + AllowedPrefixSanitizer() { + this = DataFlow::BarrierGuard::getABarrierNode() or + this = ValidationMethod::getAValidatedNode() + } +} + +/** + * Holds if `g` is a guard that considers a path safe because it is checked for `..` components, having previously + * 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 + localTaintFlowToPathGuard(e, g) and + exists(Guard previousGuard | + previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true) + or + previousGuard.(BlockListGuard).controls(g.getBasicBlock(), false) + ) +} + +private class DotDotCheckSanitizer extends PathInjectionSanitizer { + DotDotCheckSanitizer() { + this = DataFlow::BarrierGuard::getABarrierNode() or + this = ValidationMethod::getAValidatedNode() + } +} + +private class BlockListGuard extends PathGuard instanceof MethodAccess { + BlockListGuard() { + (isStringPartialMatch(this) or isPathPrefixMatch(this)) and + isDisallowedWord(super.getAnArgument()) + } + + override Expr getCheckedExpr() { result = super.getQualifier() } +} + +/** + * Holds if `g` is a guard that considers a string safe because it is checked against a blocklist of known dangerous values. + * This requires additional protection against path traversal, either another guard (`PathTraversalGuard`) + * or a sanitizer (`PathNormalizeSanitizer`), to ensure any internal `..` components are removed from the path. + */ +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); + g instanceof BlockListGuard and + localTaintFlowToPathGuard(e, g) and + exists(Expr previousGuard | + localTaintFlowToPathGuard(previousGuard.(PathNormalizeSanitizer), g) + or + previousGuard + .(PathTraversalGuard) + .controls(g.getBasicBlock(), previousGuard.(PathTraversalGuard).getBranch()) + ) +} + +private class BlockListSanitizer extends PathInjectionSanitizer { + BlockListSanitizer() { + this = DataFlow::BarrierGuard::getABarrierNode() or + this = ValidationMethod::getAValidatedNode() + } +} + +private predicate isStringPrefixMatch(MethodAccess ma) { + exists(Method m | m = ma.getMethod() and m.getDeclaringType() instanceof TypeString | + m.hasName("startsWith") + or + m.hasName("regionMatches") and + ma.getArgument(0).(CompileTimeConstantExpr).getIntValue() = 0 + or + m.hasName("matches") and + not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().matches(".*%") + ) +} + +/** + * Holds if `ma` is a call to a method that checks a partial string match. + */ +private predicate isStringPartialMatch(MethodAccess ma) { + isStringPrefixMatch(ma) + or + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"]) +} + +/** + * Holds if `ma` is a call to a method that checks whether a path starts with a prefix. + */ +private predicate isPathPrefixMatch(MethodAccess ma) { + exists(RefType t | + t instanceof TypePath + or + t.hasQualifiedName("kotlin.io", "FilesKt") + | + t = ma.getMethod().getDeclaringType() and + ma.getMethod().hasName("startsWith") + ) +} + +private predicate isDisallowedWord(CompileTimeConstantExpr 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 { + PathTraversalGuard() { + 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 + ) + ) + } + + override Expr getCheckedExpr() { + exists(MethodAccess ma | ma = this.(EqualityTest).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. */ +private 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"]) + } +} diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql b/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql index 7c759f4d26c0..aedc7fc4b2c6 100644 --- a/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql +++ b/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql @@ -17,36 +17,26 @@ import java import semmle.code.java.dataflow.FlowSources private import semmle.code.java.dataflow.ExternalFlow import semmle.code.java.security.PathCreation +import semmle.code.java.security.PathSanitizer import DataFlow::PathGraph import TaintedPathCommon -predicate containsDotDotSanitizer(Guard g, Expr e, boolean branch) { - exists(MethodAccess contains | g = contains | - contains.getMethod().hasName("contains") and - contains.getAnArgument().(StringLiteral).getValue() = ".." and - e = contains.getQualifier() and - branch = false - ) -} - class TaintedPathConfig extends TaintTracking::Configuration { TaintedPathConfig() { this = "TaintedPathConfig" } override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { - ( - sink.asExpr() = any(PathCreation p).getAnInput() - or - sinkNode(sink, "create-file") - ) and - not guarded(sink.asExpr()) + sink.asExpr() = any(PathCreation p).getAnInput() + or + sinkNode(sink, "create-file") } - override predicate isSanitizer(DataFlow::Node node) { - exists(Type t | t = node.getType() | t instanceof BoxedType or t instanceof PrimitiveType) - or - node = DataFlow::BarrierGuard::getABarrierNode() + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer.getType() instanceof BoxedType or + sanitizer.getType() instanceof PrimitiveType or + sanitizer.getType() instanceof NumberType or + sanitizer instanceof PathInjectionSanitizer } override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) { diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll b/java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll index 0e826a6fc014..418931d9a0d6 100644 --- a/java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll +++ b/java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll @@ -3,8 +3,6 @@ */ import java -import semmle.code.java.controlflow.Guards -import semmle.code.java.security.PathCreation import semmle.code.java.frameworks.Networking import semmle.code.java.dataflow.DataFlow @@ -48,29 +46,3 @@ private class TaintPreservingUriCtorParam extends Parameter { ) } } - -private predicate inWeakCheck(Expr e) { - // None of these are sufficient to guarantee that a string is safe. - exists(MethodAccess m, Method def | m.getQualifier() = e and m.getMethod() = def | - def.getName() = "startsWith" or - def.getName() = "endsWith" or - def.getName() = "isEmpty" or - def.getName() = "equals" - ) - or - // Checking against `null` has no bearing on path traversal. - exists(EqualityTest b | b.getAnOperand() = e | b.getAnOperand() instanceof NullLiteral) -} - -// Ignore cases where the variable has been checked somehow, -// but allow some particularly obviously bad cases. -predicate guarded(VarAccess e) { - exists(PathCreation p | e = p.getAnInput()) and - exists(ConditionBlock cb, Expr c | - cb.getCondition().getAChildExpr*() = c and - c = e.getVariable().getAnAccess() and - cb.controls(e.getBasicBlock(), true) and - // Disallow a few obviously bad checks. - not inWeakCheck(c) - ) -} diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPathLocal.ql b/java/ql/src/Security/CWE/CWE-022/TaintedPathLocal.ql index d5bcdd9b244b..506dc2ad65ed 100644 --- a/java/ql/src/Security/CWE/CWE-022/TaintedPathLocal.ql +++ b/java/ql/src/Security/CWE/CWE-022/TaintedPathLocal.ql @@ -15,7 +15,9 @@ import java import semmle.code.java.dataflow.FlowSources +private import semmle.code.java.dataflow.ExternalFlow import semmle.code.java.security.PathCreation +import semmle.code.java.security.PathSanitizer import DataFlow::PathGraph import TaintedPathCommon @@ -26,6 +28,15 @@ class TaintedPathLocalConfig extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(PathCreation p).getAnInput() + or + sinkNode(sink, "create-file") + } + + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer.getType() instanceof BoxedType or + sanitizer.getType() instanceof PrimitiveType or + sanitizer.getType() instanceof NumberType or + sanitizer instanceof PathInjectionSanitizer } override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) { @@ -33,12 +44,21 @@ class TaintedPathLocalConfig extends TaintTracking::Configuration { } } -from - DataFlow::PathNode source, DataFlow::PathNode sink, PathCreation p, Expr e, - TaintedPathLocalConfig conf -where - e = sink.getNode().asExpr() and - e = p.getAnInput() and - conf.hasFlowPath(source, sink) and - not guarded(e) -select p, source, sink, "This path depends on a $@.", source.getNode(), "user-provided value" +/** + * Gets the data-flow node at which to report a path ending at `sink`. + * + * Previously this query flagged alerts exclusively at `PathCreation` sites, + * so to avoid perturbing existing alerts, where a `PathCreation` exists we + * continue to report there; otherwise we report directly at `sink`. + */ +DataFlow::Node getReportingNode(DataFlow::Node sink) { + any(TaintedPathLocalConfig c).hasFlowTo(sink) and + if exists(PathCreation pc | pc.getAnInput() = sink.asExpr()) + then result.asExpr() = any(PathCreation pc | pc.getAnInput() = sink.asExpr()) + else result = sink +} + +from DataFlow::PathNode source, DataFlow::PathNode sink, TaintedPathLocalConfig conf +where conf.hasFlowPath(source, sink) +select getReportingNode(sink.getNode()), source, sink, "This path depends on a $@.", + source.getNode(), "user-provided value" diff --git a/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql b/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql index 3afab0936c02..3f63123cfb62 100644 --- a/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql +++ b/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql @@ -16,6 +16,7 @@ import java import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.SSA import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.PathSanitizer import DataFlow import PathGraph private import semmle.code.java.dataflow.ExternalFlow @@ -35,89 +36,6 @@ class ArchiveEntryNameMethod extends Method { } } -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `String`, - * `File`, and `Path`. - */ -predicate filePathStep(ExprNode n1, ExprNode n2) { - exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeFile | - n1.asExpr() = cc.getAnArgument() and - n2.asExpr() = cc - ) - or - exists(MethodAccess ma, Method m | - ma.getMethod() = m and - n1.asExpr() = ma.getQualifier() and - n2.asExpr() = ma - | - m.getDeclaringType() instanceof TypeFile and m.hasName("toPath") - or - m.getDeclaringType() instanceof TypePath and m.hasName("toAbsolutePath") - or - m.getDeclaringType() instanceof TypePath and m.hasName("toFile") - ) -} - -predicate fileTaintStep(ExprNode n1, ExprNode n2) { - exists(MethodAccess ma, Method m | - n1.asExpr() = ma.getQualifier() or - n1.asExpr() = ma.getAnArgument() - | - n2.asExpr() = ma and - ma.getMethod() = m and - m.getDeclaringType() instanceof TypePath and - m.hasName("resolve") - ) -} - -predicate localFileValueStep(Node n1, Node n2) { - localFlowStep(n1, n2) or - filePathStep(n1, n2) -} - -predicate localFileValueStepPlus(Node n1, Node n2) = fastTC(localFileValueStep/2)(n1, n2) - -/** - * Holds if `check` is a guard that checks whether `var` is a file path with a - * specific prefix when put in canonical form, thus guarding against ZipSlip. - */ -predicate validateFilePath(SsaVariable var, Guard check) { - // `var.getCanonicalFile().toPath().startsWith(...)`, - // `var.getCanonicalPath().startsWith(...)`, or - // `var.toPath().normalize().startsWith(...)` - exists(MethodAccess normalize, MethodAccess startsWith, Node n1, Node n2, Node n3, Node n4 | - n1.asExpr() = var.getAUse() and - n2.asExpr() = normalize.getQualifier() and - (n1 = n2 or localFileValueStepPlus(n1, n2)) and - n3.asExpr() = normalize and - n4.asExpr() = startsWith.getQualifier() and - (n3 = n4 or localFileValueStepPlus(n3, n4)) and - check = startsWith and - startsWith.getMethod().hasName("startsWith") and - ( - normalize.getMethod().hasName("getCanonicalFile") or - normalize.getMethod().hasName("getCanonicalPath") or - normalize.getMethod().hasName("normalize") - ) - ) -} - -/** - * Holds if `m` validates its `arg`th parameter. - */ -predicate validationMethod(Method m, int arg) { - exists(Guard check, SsaImplicitInit var, ControlFlowNode exit, ControlFlowNode normexit | - validateFilePath(var, check) and - var.isParameterDefinition(m.getParameter(arg)) and - exit = m and - normexit.getANormalSuccessor() = exit and - 1 = strictcount(ControlFlowNode n | n.getANormalSuccessor() = exit) - | - check.(ConditionNode).getATrueSuccessor() = exit or - check.controls(normexit.getBasicBlock(), true) - ) -} - class ZipSlipConfiguration extends TaintTracking::Configuration { ZipSlipConfiguration() { this = "ZipSlip" } @@ -127,24 +45,7 @@ class ZipSlipConfiguration extends TaintTracking::Configuration { override predicate isSink(Node sink) { sink instanceof FileCreationSink } - override predicate isAdditionalTaintStep(Node n1, Node n2) { - filePathStep(n1, n2) or fileTaintStep(n1, n2) - } - - override predicate isSanitizer(Node node) { - exists(Guard g, SsaVariable var, RValue varuse | validateFilePath(var, g) | - varuse = node.asExpr() and - varuse = var.getAUse() and - g.controls(varuse.getBasicBlock(), true) - ) - or - exists(MethodAccess ma, int pos, RValue rv | - validationMethod(ma.getMethod(), pos) and - ma.getArgument(pos) = rv and - adjacentUseUseSameVar(rv, node.asExpr()) and - ma.getBasicBlock().bbDominates(node.asExpr().getBasicBlock()) - ) - } + override predicate isSanitizer(Node node) { node instanceof PathInjectionSanitizer } } /** diff --git a/java/ql/src/change-notes/2022-08-25-path-sanitizer.md b/java/ql/src/change-notes/2022-08-25-path-sanitizer.md new file mode 100644 index 000000000000..a883561d59cf --- /dev/null +++ b/java/ql/src/change-notes/2022-08-25-path-sanitizer.md @@ -0,0 +1,6 @@ +--- +category: minorAnalysis +--- + +* `PathSanitizer.qll` has been promoted from experimental to the main query pack. This sanitizer was originally [submitted as part of an experimental query by @luchua-bc](https://github.com/github/codeql/pull/7286). +* The queries `java/path-injection`, `java/path-injection-local` and `java/zipslip` now use the sanitizers provided by `PathSanitizer.qll`. \ No newline at end of file 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 e8ebabba3c6d..fff9a3b3613d 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql @@ -15,9 +15,32 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.PathCreation import JFinalController -import experimental.semmle.code.java.PathSanitizer +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" } @@ -31,7 +54,7 @@ class InjectFilePathConfig extends TaintTracking::Configuration { override predicate isSanitizer(DataFlow::Node node) { exists(Type t | t = node.getType() | t instanceof BoxedType or t instanceof PrimitiveType) or - node instanceof PathTraversalSanitizer + node instanceof PathInjectionSanitizer } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-200/InsecureWebResourceResponse.ql b/java/ql/src/experimental/Security/CWE/CWE-200/InsecureWebResourceResponse.ql index 1c3615e2b3fb..7327f2cebf28 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-200/InsecureWebResourceResponse.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-200/InsecureWebResourceResponse.ql @@ -13,7 +13,7 @@ import java import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking -import experimental.semmle.code.java.PathSanitizer +import semmle.code.java.security.PathSanitizer import AndroidWebResourceResponse import DataFlow::PathGraph @@ -24,7 +24,7 @@ class InsecureWebResourceResponseConfig extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof WebResourceResponseSink } - override predicate isSanitizer(DataFlow::Node node) { node instanceof PathTraversalSanitizer } + override predicate isSanitizer(DataFlow::Node node) { node instanceof PathInjectionSanitizer } } from DataFlow::PathNode source, DataFlow::PathNode sink, InsecureWebResourceResponseConfig conf diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql index 138f07455f1f..e7c70a695b6b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql @@ -15,7 +15,7 @@ import UnsafeUrlForward import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking import experimental.semmle.code.java.frameworks.Jsf -import experimental.semmle.code.java.PathSanitizer +import semmle.code.java.security.PathSanitizer import DataFlow::PathGraph class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { @@ -37,7 +37,7 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer or - node instanceof PathTraversalSanitizer + node instanceof PathInjectionSanitizer } override DataFlow::FlowFeature getAFeature() { diff --git a/java/ql/src/experimental/semmle/code/java/PathSanitizer.qll b/java/ql/src/experimental/semmle/code/java/PathSanitizer.qll deleted file mode 100644 index 768d35504f70..000000000000 --- a/java/ql/src/experimental/semmle/code/java/PathSanitizer.qll +++ /dev/null @@ -1,222 +0,0 @@ -import java -private import semmle.code.java.controlflow.Guards -private import semmle.code.java.dataflow.FlowSources -private import semmle.code.java.dataflow.ExternalFlow - -/** - * DEPRECATED: Use `PathTraversalSanitizer` instead. - * - * A barrier guard that protects against path traversal vulnerabilities. - */ -abstract deprecated class PathTraversalBarrierGuard extends DataFlow::BarrierGuard { } - -/** A sanitizer that protects against path traversal vulnerabilities. */ -abstract class PathTraversalSanitizer extends DataFlow::Node { } - -/** - * Holds if `g` is guard that compares a string to a trusted value. - */ -private predicate exactStringPathMatchGuard(Guard g, Expr e, boolean branch) { - exists(MethodAccess ma | - ma = g and - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().getName() = ["equals", "equalsIgnoreCase"] and - e = ma.getQualifier() and - branch = true - ) -} - -private class ExactStringPathMatchSanitizer extends PathTraversalSanitizer { - ExactStringPathMatchSanitizer() { - this = DataFlow::BarrierGuard::getABarrierNode() - } -} - -/** - * Given input `e` = `v.method1(...).method2(...)...`, returns `v` where `v` is a `VarAccess`. - * - * This is used to look through field accessors such as `uri.getPath()`. - */ -private Expr getUnderlyingVarAccess(Expr e) { - result = getUnderlyingVarAccess(e.(MethodAccess).getQualifier()) - or - result = e.(VarAccess) -} - -private class AllowListGuard extends Guard instanceof MethodAccess { - AllowListGuard() { - (isStringPartialMatch(this) or isPathPartialMatch(this)) and - not isDisallowedWord(super.getAnArgument()) - } - - Expr getCheckedExpr() { result = getUnderlyingVarAccess(super.getQualifier()) } -} - -/** - * Holds if `g` is a guard that considers a path safe because it is checked against an allowlist of partial trusted values. - * This requires additional protection against path traversal, either another guard (`PathTraversalGuard`) - * or a sanitizer (`PathNormalizeSanitizer`), to ensure any internal `..` components are removed from the path. - */ -private predicate allowListGuard(Guard g, Expr e, boolean branch) { - e = g.(AllowListGuard).getCheckedExpr() and - branch = true and - ( - // Either a path normalization sanitizer comes before the guard, - exists(PathNormalizeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e)) - or - // or a check like `!path.contains("..")` comes before the guard - exists(PathTraversalGuard previousGuard | - DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and - previousGuard.controls(g.getBasicBlock().(ConditionBlock), false) - ) - ) -} - -private class AllowListSanitizer extends PathTraversalSanitizer { - AllowListSanitizer() { this = DataFlow::BarrierGuard::getABarrierNode() } -} - -/** - * Holds if `g` is a guard that considers a path safe because it is checked for `..` components, having previously - * been checked for a trusted prefix. - */ -private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) { - e = g.(PathTraversalGuard).getCheckedExpr() and - branch = false and - // The same value has previously been checked against a list of allowed prefixes: - exists(AllowListGuard previousGuard | - DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and - previousGuard.controls(g.getBasicBlock().(ConditionBlock), true) - ) -} - -private class DotDotCheckSanitizer extends PathTraversalSanitizer { - DotDotCheckSanitizer() { this = DataFlow::BarrierGuard::getABarrierNode() } -} - -private class BlockListGuard extends Guard instanceof MethodAccess { - BlockListGuard() { - (isStringPartialMatch(this) or isPathPartialMatch(this)) and - isDisallowedWord(super.getAnArgument()) - } - - Expr getCheckedExpr() { result = getUnderlyingVarAccess(super.getQualifier()) } -} - -/** - * Holds if `g` is a guard that considers a string safe because it is checked against a blocklist of known dangerous values. - * This requires a prior check for URL encoding concealing a forbidden value, either a guard (`UrlEncodingGuard`) - * or a sanitizer (`UrlDecodeSanitizer`). - */ -private predicate blockListGuard(Guard g, Expr e, boolean branch) { - e = g.(BlockListGuard).getCheckedExpr() and - branch = false and - ( - // Either `e` has been URL decoded: - exists(UrlDecodeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e)) - or - // or `e` has previously been checked for URL encoding sequences: - exists(UrlEncodingGuard previousGuard | - DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and - previousGuard.controls(g.getBasicBlock(), false) - ) - ) -} - -private class BlockListSanitizer extends PathTraversalSanitizer { - BlockListSanitizer() { this = DataFlow::BarrierGuard::getABarrierNode() } -} - -/** - * Holds if `g` is a guard that considers a string safe because it is checked for URL encoding sequences, - * having previously been checked against a block-list of forbidden values. - */ -private predicate urlEncodingGuard(Guard g, Expr e, boolean branch) { - e = g.(UrlEncodingGuard).getCheckedExpr() and - branch = false and - exists(BlockListGuard previousGuard | - DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and - previousGuard.controls(g.getBasicBlock(), false) - ) -} - -private class UrlEncodingSanitizer extends PathTraversalSanitizer { - UrlEncodingSanitizer() { this = DataFlow::BarrierGuard::getABarrierNode() } -} - -/** - * Holds if `ma` is a call to a method that checks a partial string match. - */ -private predicate isStringPartialMatch(MethodAccess ma) { - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().getName() = - ["contains", "startsWith", "matches", "regionMatches", "indexOf", "lastIndexOf"] -} - -/** - * Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a partial path match. - */ -private predicate isPathPartialMatch(MethodAccess ma) { - ma.getMethod().getDeclaringType() instanceof TypePath and - ma.getMethod().getName() = "startsWith" -} - -private predicate isDisallowedWord(CompileTimeConstantExpr word) { - word.getStringValue().matches(["%WEB-INF%", "%META-INF%", "%..%"]) -} - -/** A complementary guard that protects against path traversal, by looking for the literal `..`. */ -class PathTraversalGuard extends Guard instanceof MethodAccess { - PathTraversalGuard() { - super.getMethod().getDeclaringType() instanceof TypeString and - super.getMethod().hasName(["contains", "indexOf"]) and - super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." - } - - Expr getCheckedExpr() { result = getUnderlyingVarAccess(super.getQualifier()) } -} - -/** A complementary sanitizer that protects against path traversal using path normalization. */ -private class PathNormalizeSanitizer extends MethodAccess { - PathNormalizeSanitizer() { - this.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Path") and - this.getMethod().hasName("normalize") - } -} - -/** A complementary guard that protects against double URL encoding, by looking for the literal `%`. */ -private class UrlEncodingGuard extends Guard instanceof MethodAccess { - UrlEncodingGuard() { - super.getMethod().getDeclaringType() instanceof TypeString and - super.getMethod().hasName(["contains", "indexOf"]) and - super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" - } - - Expr getCheckedExpr() { result = super.getQualifier() } -} - -/** A complementary sanitizer that protects against double URL encoding using URL decoding. */ -private class UrlDecodeSanitizer extends MethodAccess { - UrlDecodeSanitizer() { - this.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and - this.getMethod().hasName("decode") - } -} - -/** A node with path normalization. */ -class NormalizedPathNode extends DataFlow::Node { - NormalizedPathNode() { - TaintTracking::localExprTaint(this.asExpr(), any(PathNormalizeSanitizer ma)) - } -} - -/** Data model related to `java.nio.file.Path`. */ -private class PathDataModel extends SummaryModelCsv { - override predicate row(string row) { - row = - [ - "java.nio.file;Paths;true;get;;;Argument[0];ReturnValue;taint;manual", - "java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint;manual" - ] - } -} diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java index 279764fba8b8..ee63939b209e 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java @@ -100,7 +100,10 @@ protected void doHead4(HttpServletRequest request, HttpServletResponse response) } } - // BAD: Request dispatcher with negation check and path normalization, but without URL decoding + // FN: Request dispatcher with negation check and path normalization, but without URL decoding + // When promoting this query, consider using FlowStates to make `getRequestDispatcher` a sink + // only if a URL-decoding step has NOT been crossed (i.e. make URLDecoder.decode change the + // state to a different value than the one required at the sink). protected void doHead5(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { String path = request.getParameter("path"); diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected index e227c58d7c7b..11a8bc6c2485 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected @@ -20,11 +20,6 @@ edges | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:76:53:76:56 | path | -| UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:107:53:107:56 | path : String | -| UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path | UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path | -| UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path | UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path | -| UnsafeServletRequestDispatch.java:107:53:107:56 | path : String | UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path | -| UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path | UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | | UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url | | UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url | | UnsafeUrlForward.java:25:21:25:30 | url : String | UnsafeUrlForward.java:26:23:26:25 | url | @@ -69,12 +64,6 @@ nodes | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | semmle.label | returnURL | | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | | UnsafeServletRequestDispatch.java:76:53:76:56 | path | semmle.label | path | -| UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | -| UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path | semmle.label | resolve(...) : Path | -| UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path | semmle.label | normalize(...) : Path | -| UnsafeServletRequestDispatch.java:107:53:107:56 | path : String | semmle.label | path : String | -| UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path | semmle.label | requestedPath : Path | -| UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | semmle.label | toString(...) | | UnsafeUrlForward.java:13:27:13:36 | url : String | semmle.label | url : String | | UnsafeUrlForward.java:14:27:14:29 | url | semmle.label | url | | UnsafeUrlForward.java:18:27:18:36 | url : String | semmle.label | url : String | @@ -106,7 +95,6 @@ subpaths | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) | user-provided value | | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) | user-provided value | | UnsafeServletRequestDispatch.java:76:53:76:56 | path | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:76:53:76:56 | path | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) | user-provided value | -| UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) | user-provided value | | UnsafeUrlForward.java:14:27:14:29 | url | UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:13:27:13:36 | url | user-provided value | | UnsafeUrlForward.java:20:28:20:30 | url | UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:18:27:18:36 | url | user-provided value | | UnsafeUrlForward.java:26:23:26:25 | url | UnsafeUrlForward.java:25:21:25:30 | url : String | UnsafeUrlForward.java:26:23:26:25 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:25:21:25:30 | url | user-provided value | 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 000000000000..27a15d867ce1 --- /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); // $ hasTaintFlow + else + sink(source); // Safe + } + { + 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 disallowed prefixes + { + String source = (String) source(); + String normalized = Paths.get(source).normalize().toString(); + if (!normalized.contains("/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(1, "/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 + } + } + } +} diff --git a/java/ql/test/library-tests/pathsanitizer/options b/java/ql/test/library-tests/pathsanitizer/options new file mode 100644 index 000000000000..1e6e9ea2bf15 --- /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 000000000000..e69de29bb2d1 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 000000000000..c3b8ca70367e --- /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) } +} diff --git a/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected b/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected index 48c540306795..7c6a5bffc2ee 100644 --- a/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected +++ b/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected @@ -1,8 +1,5 @@ edges | ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:8:31:8:34 | name : String | -| ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:9:48:9:51 | file | -| ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:10:49:10:52 | file | -| ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:11:36:11:39 | file | | ZipTest.java:8:17:8:35 | new File(...) : File | ZipTest.java:9:48:9:51 | file | | ZipTest.java:8:17:8:35 | new File(...) : File | ZipTest.java:10:49:10:52 | file | | ZipTest.java:8:17:8:35 | new File(...) : File | ZipTest.java:11:36:11:39 | file |