diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql b/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql index 1d7b39956ea..01d89cc8e06 100644 --- a/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql +++ b/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql @@ -16,6 +16,7 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.PathCreation import DataFlow::PathGraph +import TaintedPathCommon class ContainsDotDotSanitizer extends DataFlow::BarrierGuard { ContainsDotDotSanitizer() { diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll b/java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll new file mode 100644 index 00000000000..beea2d5f666 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll @@ -0,0 +1,33 @@ +/** + * Models a very basic guard for the tainted path queries. + */ + +import java +import semmle.code.java.controlflow.Guards +import semmle.code.java.security.PathCreation + +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 c12230a9922..a64f88997e8 100644 --- a/java/ql/src/Security/CWE/CWE-022/TaintedPathLocal.ql +++ b/java/ql/src/Security/CWE/CWE-022/TaintedPathLocal.ql @@ -16,13 +16,16 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.PathCreation import DataFlow::PathGraph +import TaintedPathCommon class TaintedPathLocalConfig extends TaintTracking::Configuration { TaintedPathLocalConfig() { this = "TaintedPathLocalConfig" } override predicate isSource(DataFlow::Node source) { source instanceof LocalUserInput } - override predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(PathCreation p).getAnInput() } + override predicate isSink(DataFlow::Node sink) { + sink.asExpr() = any(PathCreation p).getAnInput() + } } from diff --git a/java/ql/src/semmle/code/java/security/PathCreation.qll b/java/ql/src/semmle/code/java/security/PathCreation.qll index ee5b5aed009..2954cfa1332 100644 --- a/java/ql/src/semmle/code/java/security/PathCreation.qll +++ b/java/ql/src/semmle/code/java/security/PathCreation.qll @@ -3,7 +3,6 @@ */ import java -import semmle.code.java.controlflow.Guards /** Models the creation of a path. */ abstract class PathCreation extends Expr { @@ -140,29 +139,3 @@ private class FileOutputStreamCreation extends PathCreation, ClassInstanceExpr { result.getType() instanceof TypeString } } - -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) - ) -}