Skip to content

Java: make all code-scanning queries diff-informed #17846

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
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
2 changes: 1 addition & 1 deletion java/ql/lib/semmle/code/java/frameworks/JaxWS.qll
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ class JaxRSConsumesAnnotation extends JaxRSAnnotation {
}

/** A default sink representing methods susceptible to XSS attacks. */
private class JaxRSXssSink extends XssSink {
private class JaxRSXssSink extends AbstractXssSink {
JaxRSXssSink() {
exists(JaxRsResourceMethod resourceMethod, ReturnStmt rs |
resourceMethod = any(JaxRsResourceClass resourceClass).getAResourceMethod() and
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ private predicate specifiesContentType(SpringRequestMappingMethod method) {
exists(method.getAProducesExpr())
}

private class SpringXssSink extends XSS::XssSink {
private class SpringXssSink extends XSS::AbstractXssSink {
SpringXssSink() {
exists(SpringRequestMappingMethod requestMappingMethod, ReturnStmt rs |
requestMappingMethod = rs.getEnclosingCallable() and
Expand Down
28 changes: 28 additions & 0 deletions java/ql/lib/semmle/code/java/regex/RegexDiffInformed.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
private import java

Check warning on line 1 in java/ql/lib/semmle/code/java/regex/RegexDiffInformed.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for file RegexDiffInformed
private import semmle.code.java.dataflow.DataFlow
private import codeql.util.Unit

/**
* An extension point to allow a query to detect only the regular expressions
* it needs in diff-informed incremental mode. The data-flow analysis that's
* modified by this class has its sources as (certain) string literals and its
* sinks as regular-expression matches.
*/
class RegexDiffInformedConfig instanceof Unit {
/**
* Holds if discovery of regular expressions should be diff-informed, which
* is possible when there cannot be any elements selected by the query in the
* diff range except the regular expressions and (locations derived from) the
* places where they are matched against.
*/
abstract predicate observeDiffInformedIncrementalMode();

/**
* Gets a location of a regex match that will be part of the query results.
* If the query does not select the match locations, this predicate can be
* `none()` for performance.
*/
abstract Location getASelectedSinkLocation(DataFlow::Node sink);

string toString() { result = "RegexDiffInformedConfig" }

Check warning on line 27 in java/ql/lib/semmle/code/java/regex/RegexDiffInformed.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for member-predicate RegexDiffInformed::RegexDiffInformedConfig::toString/0
}
9 changes: 9 additions & 0 deletions java/ql/lib/semmle/code/java/regex/RegexFlowConfigs.qll
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import java
import semmle.code.java.dataflow.ExternalFlow
private import semmle.code.java.dataflow.DataFlow
private import semmle.code.java.security.SecurityTests
private import RegexDiffInformed

private class ExploitableStringLiteral extends StringLiteral {
ExploitableStringLiteral() { this.getValue().matches(["%+%", "%*%", "%{%}%"]) }
Expand Down Expand Up @@ -157,6 +158,14 @@ private module RegexFlowConfig implements DataFlow::ConfigSig {
}

int fieldFlowBranchLimit() { result = 1 }

predicate observeDiffInformedIncrementalMode() {
exists(RegexDiffInformedConfig c | c.observeDiffInformedIncrementalMode())
}

Location getASelectedSinkLocation(DataFlow::Node sink) {
exists(RegexDiffInformedConfig c | result = c.getASelectedSinkLocation(sink))
}
}

private module RegexFlow = DataFlow::Global<RegexFlowConfig>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,17 @@ private import semmle.code.java.dataflow.FlowSinks
private import semmle.code.java.dataflow.FlowSources

private class CookieCleartextStorageSink extends CleartextStorageSink {
CookieCleartextStorageSink() { this.asExpr() = cookieInput(_) }
Cookie cookie;

CookieCleartextStorageSink() { this.asExpr() = cookieInput(cookie) }

override Location getASelectedLocation() {
result = this.getLocation()
or
result = cookie.getLocation()
or
result = cookie.getAStore().getLocation()
}
}

/** The instantiation of a cookie, which can act as storage. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,14 @@ private import semmle.code.java.dataflow.TaintTracking
private import semmle.code.java.security.SensitiveActions

/** A sink representing persistent storage that saves data in clear text. */
abstract class CleartextStorageSink extends DataFlow::Node { }
abstract class CleartextStorageSink extends DataFlow::Node {
/**
* Gets a location that will be selected in the diff-informed query where
* this sink is found. If this has no results for any sink, that's taken to
* mean the query is not diff-informed.
*/
Location getASelectedLocation() { none() }
}

/** A sanitizer for flows tracking sensitive data being stored in persistent storage. */
abstract class CleartextStorageSanitizer extends DataFlow::Node { }
Expand Down Expand Up @@ -46,6 +53,17 @@ private module SensitiveSourceFlowConfig implements DataFlow::ConfigSig {
predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
any(CleartextStorageAdditionalTaintStep c).step(n1, n2)
}

predicate observeDiffInformedIncrementalMode() {
// This configuration is used by several queries. A query can opt in to
// diff-informed mode by implementing `getASelectedLocation` on its sinks,
// indicating that it has considered which sinks are selected.
exists(CleartextStorageSink sink | exists(sink.getASelectedLocation()))
}

Location getASelectedSinkLocation(DataFlow::Node sink) {
result = sink.(CleartextStorageSink).getASelectedLocation()
}
}

private module SensitiveSourceFlow = TaintTracking::Global<SensitiveSourceFlowConfig>;
Expand Down
43 changes: 37 additions & 6 deletions java/ql/lib/semmle/code/java/security/InformationLeak.qll
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,44 @@ import semmle.code.java.dataflow.DataFlow
private import semmle.code.java.dataflow.ExternalFlow
import semmle.code.java.security.XSS

/** A sink that represent a method that outputs data to an HTTP response. */
abstract class InformationLeakSink extends DataFlow::Node { }
/**
* A sink that represent a method that outputs data to an HTTP response. Extend
* this class to add more sinks that should be considered information leak
* points by every query. To find the full set of information-leak sinks, use
* `InformationLeakSink` instead.
*/
abstract class AbstractInformationLeakSink extends DataFlow::Node { }

/**
* A sink that represent a method that outputs data to an HTTP response. To add
* more sinks, extend `AbstractInformationLeakSink` rather than this class.
*/
final class InformationLeakSink extends DataFlow::Node instanceof InformationLeakDiffInformed<xssNotDiffInformed/0>::InformationLeakSink
{ }

/** A default sink representing methods outputing data to an HTTP response. */
private class DefaultInformationLeakSink extends InformationLeakSink {
DefaultInformationLeakSink() {
sinkNode(this, "information-leak") or
this instanceof XssSink
private class DefaultInformationLeakSink extends AbstractInformationLeakSink {
DefaultInformationLeakSink() { sinkNode(this, "information-leak") }
}

/**
* A module for finding information-leak sinks faster in a diff-informed query.
* The `hasSourceInDiffRange` parameter should hold if the overall data-flow
* configuration of the query has any sources in the diff range.
*/
module InformationLeakDiffInformed<xssNullaryPredicate/0 hasSourceInDiffRange> {
final private class Node = DataFlow::Node;

/**
* A diff-informed replacement for the top-level `InformationLeakSink`,
* omitting for efficiency some sinks that would never be reported by a
* diff-informed query.
*/
final class InformationLeakSink extends Node {
InformationLeakSink() {
this instanceof AbstractInformationLeakSink
or
this instanceof XssDiffInformed<hasSourceInDiffRange/0>::XssSink
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@ private class GetMessageFlowSource extends ApiSourceNode {
private module GetMessageFlowSourceToHttpResponseSinkFlowConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node src) { src instanceof GetMessageFlowSource }

predicate isSink(DataFlow::Node sink) { sink instanceof InformationLeakSink }
predicate isSink(DataFlow::Node sink) {
sink instanceof
InformationLeakDiffInformed<GetMessageFlowSourceToHttpResponseSinkFlow::hasSourceInDiffRange/0>::InformationLeakSink
}

predicate observeDiffInformedIncrementalMode() { any() }
}

private module GetMessageFlowSourceToHttpResponseSinkFlow =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@ private module ServletWriterSourceToPrintStackTraceMethodFlowConfig implements D
sink.asExpr() = ma.getAnArgument() and ma.getMethod() instanceof PrintStackTraceMethod
)
}

predicate observeDiffInformedIncrementalMode() { any() }

Location getASelectedSinkLocation(DataFlow::Node sink) {
exists(MethodCall ma | result = ma.getLocation() |
sink.asExpr() = ma.getAnArgument() and ma.getMethod() instanceof PrintStackTraceMethod
)
}
}

private module ServletWriterSourceToPrintStackTraceMethodFlow =
Expand Down Expand Up @@ -69,7 +77,16 @@ private predicate stackTraceExpr(Expr exception, MethodCall stackTraceString) {
private module StackTraceStringToHttpResponseSinkFlowConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node src) { stackTraceExpr(_, src.asExpr()) }

predicate isSink(DataFlow::Node sink) { sink instanceof InformationLeakSink }
predicate isSink(DataFlow::Node sink) {
sink instanceof
InformationLeakDiffInformed<StackTraceStringToHttpResponseSinkFlow::hasSourceInDiffRange/0>::InformationLeakSink
}

predicate observeDiffInformedIncrementalMode() { any() }

Location getASelectedSourceLocation(DataFlow::Node source) {
exists(Expr e | stackTraceExpr(e, source.asExpr()) and result = e.getLocation())
}
}

private module StackTraceStringToHttpResponseSinkFlow =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@
}

private predicate arrayUpdateSrc(DataFlow::Node source) {
source.asExpr() instanceof StaticByteArrayCreation
StaticInitializationVectorFlow::flow(source, _)
}

private predicate arrayUpdateSink(DataFlow::Node sink) {
Expand All @@ -92,7 +92,7 @@

private module ArrayUpdateFlow = ArrayUpdateFlowFwd::Graph<arrayUpdateSink/1>;

private predicate arrayReachesUpdate(StaticByteArrayCreation array) {
predicate arrayReachesUpdate(StaticByteArrayCreation array) {

Check warning on line 95 in java/ql/lib/semmle/code/java/security/StaticInitializationVectorQuery.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for classless-predicate StaticInitializationVectorQuery::arrayReachesUpdate/1
exists(ArrayUpdateFlow::PathNode src | src.isSource() and src.getNode().asExpr() = array)
}

Expand All @@ -102,7 +102,6 @@
private class StaticInitializationVectorSource extends DataFlow::Node {
StaticInitializationVectorSource() {
exists(StaticByteArrayCreation array | array = this.asExpr() |
not arrayReachesUpdate(array) and
// Reduce FPs from utility methods that return an empty array in an exceptional case
not exists(ReturnStmt ret |
array.getADimension().(CompileTimeConstantExpr).getIntValue() = 0 and
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -379,8 +379,12 @@ predicate looksLikeResolveClassStep(DataFlow::Node fromNode, DataFlow::Node toNo

/** A sink representing an argument of a deserialization method */
private class UnsafeTypeSink extends DataFlow::Node {
MethodCall ma;

MethodCall getMethodCall() { result = ma }

UnsafeTypeSink() {
exists(MethodCall ma, int i, Expr arg | i > 0 and ma.getArgument(i) = arg |
exists(int i, Expr arg | i > 0 and ma.getArgument(i) = arg |
(
ma.getMethod() instanceof ObjectMapperReadMethod
or
Expand Down Expand Up @@ -425,6 +429,25 @@ module UnsafeTypeConfig implements DataFlow::ConfigSig {
predicate isAdditionalFlowStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
isUnsafeTypeAdditionalTaintStep(fromNode, toNode)
}

predicate observeDiffInformedIncrementalMode() {
// Since this configuration is for finding sinks to be used in a main
// data-flow configuration, this configuration should only restrict the
// sinks to be found if there are no main-configuration sources in the diff
// range. That's because if there is such a source, we need to report query
// results for it even with sinks outside the diff range.
not UnsafeDeserializationFlow::hasSourceInDiffRange()
}

// The query does not select the sources of this configuration
Location getASelectedSourceLocation(DataFlow::Node source) { none() }

Location getASelectedSinkLocation(DataFlow::Node sink) {
// Match by the surrounding method call since the sink of the overall
// query will be contained in that (see the body of
// `unsafeDeserialization/2`).
result = sink.(UnsafeTypeSink).getMethodCall().getLocation()
}
}

/**
Expand Down
Loading
Loading