Skip to content

Java: Refactor more dataflow queries to the new API #12476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand All @@ -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<PolynomialRedosConfig>;
34 changes: 19 additions & 15 deletions java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql
Original file line number Diff line number Diff line change
Expand Up @@ -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<RemoteUserInputOverflowConfig>;

override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
module RemoteUserInputUnderflow = TaintTracking::Make<RemoteUserInputUnderflowConfig>;

override predicate isSink(DataFlow::Node sink) { underflowSink(_, sink.asExpr()) }
module Flow =
DataFlow::MergePathGraph<RemoteUserInputOverflow::PathNode, RemoteUserInputUnderflow::PathNode, RemoteUserInputOverflow::PathGraph, RemoteUserInputUnderflow::PathGraph>;

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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down Expand Up @@ -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)
)
Expand All @@ -148,6 +145,9 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf
}
}

module TempDirSystemGetPropertyToCreate =
TaintTracking::Make<TempDirSystemGetPropertyToCreateConfig>;

/**
* Configuration that tracks calls to to `mkdir` or `mkdirs` that are are directly on the temp directory system property.
* Examples:
Expand All @@ -158,30 +158,29 @@ 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)
|
isFileConstructorArgument(callSite.asExpr(), node.asExpr(), 1)
)
}

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<TempDirSystemGetPropertyDirectlyToMkdirConfig>;

//
// Begin configuration for tracking single-method calls that are vulnerable.
//
Expand All @@ -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 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed because MethodAccessInsecureFileCreation is used as the type parameter of PathGraphSig? I suppose we can't make MethodAccessInsecureFileCreation implement PathNodeSig in some way to enforce that, can we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

}

/**
Expand Down Expand Up @@ -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<MethodAccessInsecureFileCreation> {
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<TempDirSystemGetPropertyToCreate::PathNode, MethodAccessInsecureFileCreation, TempDirSystemGetPropertyToCreate::PathGraph, InsecureMethodPathGraph>;

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."

Check warning

Code scanning / CodeQL

QL-for-QL encountered an internal consistency error

PredConsistency::noResolveCall
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a problem of QL-for-QL not being able to resolve the concrete type of PathNode2, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds plausible.

) and
not isPermissionsProtectedTempDirUse(sink.getNode())
select source.getNode(), source, sink, message, source.getNode(), "system temp directory"
8 changes: 5 additions & 3 deletions java/ql/src/Security/CWE/CWE-730/PolynomialReDoS.ql
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ""
)
}
Expand Down