diff --git a/java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll b/java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll index 2a822ac69de9..b852c8393fbb 100644 --- a/java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll +++ b/java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll @@ -32,8 +32,12 @@ private class LengthRestrictedMethod extends Method { } } -/** A configuration for Polynomial ReDoS queries. */ -class PolynomialRedosConfig extends TaintTracking::Configuration { +/** + * DEPRECATED: Use `PolynomialRedosFlow` instead. + * + * A configuration for Polynomial ReDoS queries. + */ +deprecated class PolynomialRedosConfig extends TaintTracking::Configuration { PolynomialRedosConfig() { this = "PolynomialRedosConfig" } override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource } @@ -47,11 +51,34 @@ class PolynomialRedosConfig extends TaintTracking::Configuration { } } -/** Holds if there is flow from `source` to `sink` that is matched against the regexp term `regexp` that is vulnerable to Polynomial ReDoS. */ -predicate hasPolynomialReDoSResult( +/** + * DEPRECATED: Use `PolynomialRedosFlow` instead. + * + * Holds if there is flow from `source` to `sink` that is matched against the regexp term `regexp` that is vulnerable to Polynomial ReDoS. + */ +deprecated predicate hasPolynomialReDoSResult( DataFlow::PathNode source, DataFlow::PathNode sink, SuperlinearBackTracking::PolynomialBackTrackingTerm regexp ) { any(PolynomialRedosConfig config).hasFlowPath(source, sink) and regexp.getRootTerm() = sink.getNode().(PolynomialRedosSink).getRegExp() } + +/** A configuration for Polynomial ReDoS queries. */ +private module PolynomialRedosConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource } + + predicate isSink(DataFlow::Node sink) { + exists(SuperlinearBackTracking::PolynomialBackTrackingTerm regexp | + regexp.getRootTerm() = sink.(PolynomialRedosSink).getRegExp() + ) + } + + predicate isBarrier(DataFlow::Node node) { + node.getType() instanceof PrimitiveType or + node.getType() instanceof BoxedType or + node.asExpr().(MethodAccess).getMethod() instanceof LengthRestrictedMethod + } +} + +module PolynomialRedosFlow = TaintTracking::Make; diff --git a/java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql b/java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql index 2ace7dcc65aa..99d9879d19e5 100644 --- a/java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql +++ b/java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql @@ -15,35 +15,39 @@ import java import semmle.code.java.dataflow.FlowSources import ArithmeticCommon -import DataFlow::PathGraph -class RemoteUserInputOverflowConfig extends TaintTracking::Configuration { - RemoteUserInputOverflowConfig() { this = "ArithmeticTainted.ql:RemoteUserInputOverflowConfig" } +module RemoteUserInputOverflowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + predicate isSink(DataFlow::Node sink) { overflowSink(_, sink.asExpr()) } - override predicate isSink(DataFlow::Node sink) { overflowSink(_, sink.asExpr()) } + predicate isBarrier(DataFlow::Node n) { overflowBarrier(n) } +} + +module RemoteUserInputUnderflowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSanitizer(DataFlow::Node n) { overflowBarrier(n) } + predicate isSink(DataFlow::Node sink) { underflowSink(_, sink.asExpr()) } + + predicate isBarrier(DataFlow::Node n) { underflowBarrier(n) } } -class RemoteUserInputUnderflowConfig extends TaintTracking::Configuration { - RemoteUserInputUnderflowConfig() { this = "ArithmeticTainted.ql:RemoteUserInputUnderflowConfig" } +module RemoteUserInputOverflow = TaintTracking::Make; - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } +module RemoteUserInputUnderflow = TaintTracking::Make; - override predicate isSink(DataFlow::Node sink) { underflowSink(_, sink.asExpr()) } +module Flow = + DataFlow::MergePathGraph; - override predicate isSanitizer(DataFlow::Node n) { underflowBarrier(n) } -} +import Flow::PathGraph -from DataFlow::PathNode source, DataFlow::PathNode sink, ArithExpr exp, string effect +from Flow::PathNode source, Flow::PathNode sink, ArithExpr exp, string effect where - any(RemoteUserInputOverflowConfig c).hasFlowPath(source, sink) and + RemoteUserInputOverflow::hasFlowPath(source.asPathNode1(), sink.asPathNode1()) and overflowSink(exp, sink.getNode().asExpr()) and effect = "overflow" or - any(RemoteUserInputUnderflowConfig c).hasFlowPath(source, sink) and + RemoteUserInputUnderflow::hasFlowPath(source.asPathNode2(), sink.asPathNode2()) and underflowSink(exp, sink.getNode().asExpr()) and effect = "underflow" select exp, source, sink, diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql index 16e8bb72c930..76998b40b509 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql +++ b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql @@ -14,8 +14,7 @@ import java import semmle.code.java.os.OSCheck import TempDirUtils -import DataFlow::PathGraph -import semmle.code.java.dataflow.TaintTracking2 +import semmle.code.java.dataflow.TaintTracking abstract private class MethodFileSystemFileCreation extends Method { MethodFileSystemFileCreation() { this.getDeclaringType() instanceof TypeFile } @@ -127,19 +126,17 @@ private class IsSpecificWindowsSanitizer extends WindowsOsSanitizer { * A taint tracking configuration tracking the access of the system temporary directory * flowing to the creation of files or directories. */ -private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Configuration { - TempDirSystemGetPropertyToCreateConfig() { this = "TempDirSystemGetPropertyToCreateConfig" } - - override predicate isSource(DataFlow::Node source) { +module TempDirSystemGetPropertyToCreateConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source.asExpr() instanceof ExprSystemGetPropertyTempDirTainted } - override predicate isSink(DataFlow::Node sink) { + predicate isSink(DataFlow::Node sink) { sink instanceof FileCreationSink and - not any(TempDirSystemGetPropertyDirectlyToMkdirConfig config).hasFlowTo(sink) + not TempDirSystemGetPropertyDirectlyToMkdir::hasFlowTo(sink) } - override predicate isSanitizer(DataFlow::Node sanitizer) { + predicate isBarrier(DataFlow::Node sanitizer) { exists(FilesSanitizingCreationMethodAccess sanitisingMethodAccess | sanitizer.asExpr() = sanitisingMethodAccess.getArgument(0) ) @@ -148,6 +145,9 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf } } +module TempDirSystemGetPropertyToCreate = + TaintTracking::Make; + /** * Configuration that tracks calls to to `mkdir` or `mkdirs` that are are directly on the temp directory system property. * Examples: @@ -158,12 +158,8 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf * As such, this code pattern is filtered out as an explicit vulnerability in * `TempDirSystemGetPropertyToCreateConfig::isSink`. */ -private class TempDirSystemGetPropertyDirectlyToMkdirConfig extends TaintTracking2::Configuration { - TempDirSystemGetPropertyDirectlyToMkdirConfig() { - this = "TempDirSystemGetPropertyDirectlyToMkdirConfig" - } - - override predicate isSource(DataFlow::Node node) { +module TempDirSystemGetPropertyDirectlyToMkdirConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node node) { exists(ExprSystemGetPropertyTempDirTainted propertyGetExpr, DataFlow::Node callSite | DataFlow::localFlow(DataFlow::exprNode(propertyGetExpr), callSite) | @@ -171,17 +167,20 @@ private class TempDirSystemGetPropertyDirectlyToMkdirConfig extends TaintTrackin ) } - override predicate isSink(DataFlow::Node node) { + predicate isSink(DataFlow::Node node) { exists(MethodAccess ma | ma.getMethod() instanceof MethodFileDirectoryCreation | ma.getQualifier() = node.asExpr() ) } - override predicate isSanitizer(DataFlow::Node sanitizer) { + predicate isBarrier(DataFlow::Node sanitizer) { isFileConstructorArgument(sanitizer.asExpr(), _, _) } } +module TempDirSystemGetPropertyDirectlyToMkdir = + TaintTracking::Make; + // // Begin configuration for tracking single-method calls that are vulnerable. // @@ -193,6 +192,8 @@ abstract class MethodAccessInsecureFileCreation extends MethodAccess { * Gets the type of entity created (e.g. `file`, `directory`, ...). */ abstract string getFileSystemEntityType(); + + DataFlow::Node getNode() { result.asExpr() = this } } /** @@ -235,39 +236,47 @@ class MethodAccessInsecureGuavaFilesCreateTempFile extends MethodAccessInsecureF } /** - * A hack: we include use of inherently insecure methods, which don't have any associated + * We include use of inherently insecure methods, which don't have any associated * flow path, in with results describing a path from reading `java.io.tmpdir` or similar to use * in a file creation op. * - * We achieve this by making inherently-insecure method invocations both a source and a sink in - * this configuration, resulting in a zero-length path which is type-compatible with the actual - * path-flow results. + * We achieve this by making inherently-insecure method invocations into an edge-less graph, + * resulting in a zero-length paths. */ -class InsecureMethodPseudoConfiguration extends DataFlow::Configuration { - InsecureMethodPseudoConfiguration() { this = "InsecureMethodPseudoConfiguration" } +module InsecureMethodPathGraph implements DataFlow::PathGraphSig { + predicate edges(MethodAccessInsecureFileCreation n1, MethodAccessInsecureFileCreation n2) { + none() + } - override predicate isSource(DataFlow::Node node) { - node.asExpr() instanceof MethodAccessInsecureFileCreation + predicate nodes(MethodAccessInsecureFileCreation n, string key, string val) { + key = "semmle.label" and val = n.toString() } - override predicate isSink(DataFlow::Node node) { - node.asExpr() instanceof MethodAccessInsecureFileCreation + predicate subpaths( + MethodAccessInsecureFileCreation n1, MethodAccessInsecureFileCreation n2, + MethodAccessInsecureFileCreation n3, MethodAccessInsecureFileCreation n4 + ) { + none() } } -from DataFlow::PathNode source, DataFlow::PathNode sink, string message +module Flow = + DataFlow::MergePathGraph; + +import Flow::PathGraph + +from Flow::PathNode source, Flow::PathNode sink, string message where ( - any(TempDirSystemGetPropertyToCreateConfig conf).hasFlowPath(source, sink) and + TempDirSystemGetPropertyToCreate::hasFlowPath(source.asPathNode1(), sink.asPathNode1()) and message = "Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users." or - any(InsecureMethodPseudoConfiguration conf).hasFlowPath(source, sink) and + source = sink and // Note this message has no "$@" placeholder, so the "system temp directory" template parameter below is not used. message = "Local information disclosure vulnerability due to use of " + - source.getNode().asExpr().(MethodAccessInsecureFileCreation).getFileSystemEntityType() + - " readable by other local users." + source.asPathNode2().getFileSystemEntityType() + " readable by other local users." ) and not isPermissionsProtectedTempDirUse(sink.getNode()) select source.getNode(), source, sink, message, source.getNode(), "system temp directory" diff --git a/java/ql/src/Security/CWE/CWE-730/PolynomialReDoS.ql b/java/ql/src/Security/CWE/CWE-730/PolynomialReDoS.ql index a84f1c5213ec..8a1244b93d1d 100644 --- a/java/ql/src/Security/CWE/CWE-730/PolynomialReDoS.ql +++ b/java/ql/src/Security/CWE/CWE-730/PolynomialReDoS.ql @@ -15,12 +15,14 @@ import java import semmle.code.java.security.regexp.PolynomialReDoSQuery -import DataFlow::PathGraph +import PolynomialRedosFlow::PathGraph from - DataFlow::PathNode source, DataFlow::PathNode sink, + PolynomialRedosFlow::PathNode source, PolynomialRedosFlow::PathNode sink, SuperlinearBackTracking::PolynomialBackTrackingTerm regexp -where hasPolynomialReDoSResult(source, sink, regexp) +where + PolynomialRedosFlow::hasFlowPath(source, sink) and + regexp.getRootTerm() = sink.getNode().(PolynomialRedosSink).getRegExp() select sink, source, sink, "This $@ that depends on a $@ may run slow on strings " + regexp.getPrefixMessage() + "with many repetitions of '" + regexp.getPumpString() + "'.", regexp, "regular expression", diff --git a/java/ql/test/query-tests/security/CWE-730/PolynomialReDoS.ql b/java/ql/test/query-tests/security/CWE-730/PolynomialReDoS.ql index 742781d2d585..e31d890d9089 100644 --- a/java/ql/test/query-tests/security/CWE-730/PolynomialReDoS.ql +++ b/java/ql/test/query-tests/security/CWE-730/PolynomialReDoS.ql @@ -8,10 +8,10 @@ class HasPolyRedos extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasPolyRedos" and - exists(DataFlow::PathNode sink | - hasPolynomialReDoSResult(_, sink, _) and - location = sink.getNode().getLocation() and - element = sink.getNode().toString() and + exists(DataFlow::Node sink | + PolynomialRedosFlow::hasFlowTo(sink) and + location = sink.getLocation() and + element = sink.toString() and value = "" ) }