Skip to content

Commit e2e3667

Browse files
tiferettiferet
authored
Merge pull request github#11323 from github/tiferet/simplify-configs
ATM: Simplify query configurations
2 parents 375403f + c5184d3 commit e2e3667

File tree

10 files changed

+104
-132
lines changed

10 files changed

+104
-132
lines changed

‎javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
private import javascript as JS
88
import EndpointTypes
99
import EndpointCharacteristics as EndpointCharacteristics
10+
import AdaptiveThreatModeling::ATM::ResultsInfo as AtmResultsInfo
1011

1112
/**
1213
* EXPERIMENTAL. This API may change in the future.
@@ -29,10 +30,23 @@ import EndpointCharacteristics as EndpointCharacteristics
2930
* `isAdditionalFlowStep` with a more generalised definition of additional edges. See
3031
* `NosqlInjectionATM.qll` for an example of doing this.
3132
*/
32-
abstract class AtmConfig extends string {
33+
abstract class AtmConfig extends JS::TaintTracking::Configuration {
3334
bindingset[this]
3435
AtmConfig() { any() }
3536

37+
/**
38+
* Holds if `source` is a relevant taint source. When sources are not boosted, `isSource` is equivalent to
39+
* `isKnownSource` (i.e there are no "effective" sources to be classified by an ML model).
40+
*/
41+
override predicate isSource(JS::DataFlow::Node source) { this.isKnownSource(source) }
42+
43+
/**
44+
* Holds if `sink` is a known taint sink or an "effective" sink (a candidate to be classified by an ML model).
45+
*/
46+
override predicate isSink(JS::DataFlow::Node sink) {
47+
this.isKnownSink(sink) or this.isEffectiveSink(sink)
48+
}
49+
3650
/**
3751
* EXPERIMENTAL. This API may change in the future.
3852
*
@@ -127,6 +141,17 @@ abstract class AtmConfig extends string {
127141
* A cut-off value of 1 produces all alerts including those that are likely false-positives.
128142
*/
129143
float getScoreCutoff() { result = 0.0 }
144+
145+
/**
146+
* Holds if there's an ATM alert (a flow path from `source` to `sink` with ML-determined likelihood `score`) according
147+
* to this ML-boosted configuration, whereas the unboosted base query does not contain this source and sink
148+
* combination.
149+
*/
150+
predicate getAlerts(JS::DataFlow::PathNode source, JS::DataFlow::PathNode sink, float score) {
151+
this.hasFlowPath(source, sink) and
152+
not AtmResultsInfo::isFlowLikelyInBaseQuery(source.getNode(), sink.getNode()) and
153+
score = AtmResultsInfo::getScoreForFlow(source.getNode(), sink.getNode())
154+
}
130155
}
131156

132157
/** DEPRECATED: Alias for AtmConfig */

‎javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/NosqlInjectionATM.qll

Lines changed: 45 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
22
* For internal use only.
33
*
4+
* A taint-tracking configuration for reasoning about NoSQL injection vulnerabilities.
45
* Defines shared code used by the NoSQL injection boosted query.
56
*/
67

@@ -10,62 +11,19 @@ private import semmle.javascript.security.dataflow.NosqlInjectionCustomizations
1011
import AdaptiveThreatModeling
1112

1213
class NosqlInjectionAtmConfig extends AtmConfig {
13-
NosqlInjectionAtmConfig() { this = "NosqlInjectionATMConfig" }
14+
NosqlInjectionAtmConfig() { this = "NosqlInjectionAtmConfig" }
1415

1516
override predicate isKnownSource(DataFlow::Node source) {
1617
source instanceof NosqlInjection::Source or TaintedObject::isSource(source, _)
1718
}
1819

1920
override EndpointType getASinkEndpointType() { result instanceof NosqlInjectionSinkType }
20-
}
21-
22-
/** DEPRECATED: Alias for NosqlInjectionAtmConfig */
23-
deprecated class NosqlInjectionATMConfig = NosqlInjectionAtmConfig;
24-
25-
/** Holds if src -> trg is an additional flow step in the non-boosted NoSql injection security query. */
26-
predicate isBaseAdditionalFlowStep(
27-
DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
28-
) {
29-
TaintedObject::step(src, trg, inlbl, outlbl)
30-
or
31-
// additional flow step to track taint through NoSQL query objects
32-
inlbl = TaintedObject::label() and
33-
outlbl = TaintedObject::label() and
34-
exists(NoSql::Query query, DataFlow::SourceNode queryObj |
35-
queryObj.flowsTo(query) and
36-
queryObj.flowsTo(trg) and
37-
src = queryObj.getAPropertyWrite().getRhs()
38-
)
39-
}
40-
41-
/**
42-
* Gets a value that is (transitively) written to `query`, where `query` is a NoSQL sink.
43-
*
44-
* This predicate allows us to propagate data flow through property writes and array constructors
45-
* within a query object, enabling the security query to pick up NoSQL injection vulnerabilities
46-
* involving more complex queries.
47-
*/
48-
DataFlow::Node getASubexpressionWithinQuery(DataFlow::Node query) {
49-
any(NosqlInjectionAtmConfig cfg).isEffectiveSink(query) and
50-
exists(DataFlow::SourceNode receiver |
51-
receiver = [getASubexpressionWithinQuery(query), query].getALocalSource()
52-
|
53-
result =
54-
[receiver.getAPropertyWrite().getRhs(), receiver.(DataFlow::ArrayCreationNode).getAnElement()]
55-
)
56-
}
57-
58-
/**
59-
* A taint-tracking configuration for reasoning about NoSQL injection vulnerabilities.
60-
*
61-
* This is largely a copy of the taint tracking configuration for the standard NoSQL injection
62-
* query, except additional ATM sinks have been added and the additional flow step has been
63-
* generalised to cover the sinks predicted by ATM.
64-
*/
65-
class Configuration extends TaintTracking::Configuration {
66-
Configuration() { this = "NosqlInjectionATM" }
6721

68-
override predicate isSource(DataFlow::Node source) { source instanceof NosqlInjection::Source }
22+
/*
23+
* This is largely a copy of the taint tracking configuration for the standard NoSQL injection
24+
* query, except additional ATM sinks have been added and the additional flow step has been
25+
* generalised to cover the sinks predicted by ATM.
26+
*/
6927

7028
override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
7129
TaintedObject::isSource(source, label)
@@ -75,7 +33,7 @@ class Configuration extends TaintTracking::Configuration {
7533
sink.(NosqlInjection::Sink).getAFlowLabel() = label
7634
or
7735
// Allow effective sinks to have any taint label
78-
any(NosqlInjectionAtmConfig cfg).isEffectiveSink(sink)
36+
isEffectiveSink(sink)
7937
}
8038

8139
override predicate isSanitizer(DataFlow::Node node) {
@@ -94,7 +52,43 @@ class Configuration extends TaintTracking::Configuration {
9452
isBaseAdditionalFlowStep(src, trg, inlbl, outlbl)
9553
or
9654
// relaxed version of previous step to track taint through unmodeled NoSQL query objects
97-
any(NosqlInjectionAtmConfig cfg).isEffectiveSink(trg) and
55+
isEffectiveSink(trg) and
9856
src = getASubexpressionWithinQuery(trg)
9957
}
58+
59+
/** Holds if src -> trg is an additional flow step in the non-boosted NoSql injection security query. */
60+
private predicate isBaseAdditionalFlowStep(
61+
DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
62+
) {
63+
TaintedObject::step(src, trg, inlbl, outlbl)
64+
or
65+
// additional flow step to track taint through NoSQL query objects
66+
inlbl = TaintedObject::label() and
67+
outlbl = TaintedObject::label() and
68+
exists(NoSql::Query query, DataFlow::SourceNode queryObj |
69+
queryObj.flowsTo(query) and
70+
queryObj.flowsTo(trg) and
71+
src = queryObj.getAPropertyWrite().getRhs()
72+
)
73+
}
74+
75+
/**
76+
* Gets a value that is (transitively) written to `query`, where `query` is a NoSQL sink.
77+
*
78+
* This predicate allows us to propagate data flow through property writes and array constructors
79+
* within a query object, enabling the security query to pick up NoSQL injection vulnerabilities
80+
* involving more complex queries.
81+
*/
82+
private DataFlow::Node getASubexpressionWithinQuery(DataFlow::Node query) {
83+
isEffectiveSink(query) and
84+
exists(DataFlow::SourceNode receiver |
85+
receiver = [getASubexpressionWithinQuery(query), query].getALocalSource()
86+
|
87+
result =
88+
[
89+
receiver.getAPropertyWrite().getRhs(),
90+
receiver.(DataFlow::ArrayCreationNode).getAnElement()
91+
]
92+
)
93+
}
10094
}

‎javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/SqlInjectionATM.qll

Lines changed: 6 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
22
* For internal use only.
33
*
4+
* A taint-tracking configuration for reasoning about SQL injection vulnerabilities.
45
* Defines shared code used by the SQL injection boosted query.
56
*/
67

@@ -9,30 +10,16 @@ import semmle.javascript.security.dataflow.SqlInjectionCustomizations
910
import AdaptiveThreatModeling
1011

1112
class SqlInjectionAtmConfig extends AtmConfig {
12-
SqlInjectionAtmConfig() { this = "SqlInjectionATMConfig" }
13+
SqlInjectionAtmConfig() { this = "SqlInjectionAtmConfig" }
1314

1415
override predicate isKnownSource(DataFlow::Node source) { source instanceof SqlInjection::Source }
1516

1617
override EndpointType getASinkEndpointType() { result instanceof SqlInjectionSinkType }
17-
}
18-
19-
/** DEPRECATED: Alias for SqlInjectionAtmConfig */
20-
deprecated class SqlInjectionATMConfig = SqlInjectionAtmConfig;
21-
22-
/**
23-
* A taint-tracking configuration for reasoning about SQL injection vulnerabilities.
24-
*
25-
* This is largely a copy of the taint tracking configuration for the standard SQL injection
26-
* query, except additional sinks have been added using the sink endpoint filter.
27-
*/
28-
class Configuration extends TaintTracking::Configuration {
29-
Configuration() { this = "SqlInjectionATM" }
3018

31-
override predicate isSource(DataFlow::Node source) { source instanceof SqlInjection::Source }
32-
33-
override predicate isSink(DataFlow::Node sink) {
34-
sink instanceof SqlInjection::Sink or any(SqlInjectionAtmConfig cfg).isEffectiveSink(sink)
35-
}
19+
/*
20+
* This is largely a copy of the taint tracking configuration for the standard SQL injection
21+
* query, except additional sinks have been added using the sink endpoint filter.
22+
*/
3623

3724
override predicate isSanitizer(DataFlow::Node node) {
3825
super.isSanitizer(node) or

‎javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/TaintedPathATM.qll

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
22
* For internal use only.
33
*
4+
* A taint-tracking configuration for reasoning about path injection vulnerabilities.
45
* Defines shared code used by the path injection boosted query.
56
*/
67

@@ -9,32 +10,22 @@ import semmle.javascript.security.dataflow.TaintedPathCustomizations
910
import AdaptiveThreatModeling
1011

1112
class TaintedPathAtmConfig extends AtmConfig {
12-
TaintedPathAtmConfig() { this = "TaintedPathATMConfig" }
13+
TaintedPathAtmConfig() { this = "TaintedPathAtmConfig" }
1314

1415
override predicate isKnownSource(DataFlow::Node source) { source instanceof TaintedPath::Source }
1516

1617
override EndpointType getASinkEndpointType() { result instanceof TaintedPathSinkType }
17-
}
18-
19-
/** DEPRECATED: Alias for TaintedPathAtmConfig */
20-
deprecated class TaintedPathATMConfig = TaintedPathAtmConfig;
21-
22-
/**
23-
* A taint-tracking configuration for reasoning about path injection vulnerabilities.
24-
*
25-
* This is largely a copy of the taint tracking configuration for the standard path injection
26-
* query, except additional ATM sinks have been added to the `isSink` predicate.
27-
*/
28-
class Configuration extends TaintTracking::Configuration {
29-
Configuration() { this = "TaintedPathATM" }
3018

31-
override predicate isSource(DataFlow::Node source) { source instanceof TaintedPath::Source }
19+
/*
20+
* This is largely a copy of the taint tracking configuration for the standard path injection
21+
* query, except additional ATM sinks have been added to the `isSink` predicate.
22+
*/
3223

3324
override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
3425
label = sink.(TaintedPath::Sink).getAFlowLabel()
3526
or
3627
// Allow effective sinks to have any taint label
37-
any(TaintedPathAtmConfig cfg).isEffectiveSink(sink)
28+
isEffectiveSink(sink)
3829
}
3930

4031
override predicate isSanitizer(DataFlow::Node node) { node instanceof TaintedPath::Sanitizer }
@@ -60,7 +51,7 @@ class Configuration extends TaintTracking::Configuration {
6051
* of barrier guards, we port the barrier guards for the boosted query from the standard library to
6152
* sanitizer guards here.
6253
*/
63-
class BarrierGuardNodeAsSanitizerGuardNode extends TaintTracking::LabeledSanitizerGuardNode {
54+
private class BarrierGuardNodeAsSanitizerGuardNode extends TaintTracking::LabeledSanitizerGuardNode {
6455
BarrierGuardNodeAsSanitizerGuardNode() { this instanceof TaintedPath::BarrierGuardNode }
6556

6657
override predicate sanitizes(boolean outcome, Expr e) {

‎javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll

Lines changed: 6 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
22
* For internal use only.
33
*
4+
* A taint-tracking configuration for reasoning about XSS vulnerabilities.
45
* Defines shared code used by the XSS boosted query.
56
*/
67

@@ -9,31 +10,16 @@ private import semmle.javascript.security.dataflow.DomBasedXssCustomizations
910
import AdaptiveThreatModeling
1011

1112
class DomBasedXssAtmConfig extends AtmConfig {
12-
DomBasedXssAtmConfig() { this = "DomBasedXssATMConfig" }
13+
DomBasedXssAtmConfig() { this = "DomBasedXssAtmConfig" }
1314

1415
override predicate isKnownSource(DataFlow::Node source) { source instanceof DomBasedXss::Source }
1516

1617
override EndpointType getASinkEndpointType() { result instanceof XssSinkType }
17-
}
18-
19-
/** DEPRECATED: Alias for DomBasedXssAtmConfig */
20-
deprecated class DomBasedXssATMConfig = DomBasedXssAtmConfig;
21-
22-
/**
23-
* A taint-tracking configuration for reasoning about XSS vulnerabilities.
24-
*
25-
* This is largely a copy of the taint tracking configuration for the standard XSSThroughDom query,
26-
* except additional ATM sinks have been added to the `isSink` predicate.
27-
*/
28-
class Configuration extends TaintTracking::Configuration {
29-
Configuration() { this = "DomBasedXssATMConfiguration" }
3018

31-
override predicate isSource(DataFlow::Node source) { source instanceof DomBasedXss::Source }
32-
33-
override predicate isSink(DataFlow::Node sink) {
34-
sink instanceof DomBasedXss::Sink or
35-
any(DomBasedXssAtmConfig cfg).isEffectiveSink(sink)
36-
}
19+
/*
20+
* This is largely a copy of the taint tracking configuration for the standard XSSThroughDom query,
21+
* except additional ATM sinks have been added to the `isSink` predicate.
22+
*/
3723

3824
override predicate isSanitizer(DataFlow::Node node) {
3925
super.isSanitizer(node) or

‎javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointDataTraining.qll

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -206,13 +206,14 @@ query predicate reformattedTrainingEndpoints(
206206
* TODO: Delete this once we are no longer surfacing `hasFlowFromSource`.
207207
*/
208208
DataFlow::Configuration getDataFlowCfg(Query query) {
209-
query instanceof NosqlInjectionQuery and result instanceof NosqlInjectionAtm::Configuration
209+
query instanceof NosqlInjectionQuery and
210+
result instanceof NosqlInjectionAtm::NosqlInjectionAtmConfig
210211
or
211-
query instanceof SqlInjectionQuery and result instanceof SqlInjectionAtm::Configuration
212+
query instanceof SqlInjectionQuery and result instanceof SqlInjectionAtm::SqlInjectionAtmConfig
212213
or
213-
query instanceof TaintedPathQuery and result instanceof TaintedPathAtm::Configuration
214+
query instanceof TaintedPathQuery and result instanceof TaintedPathAtm::TaintedPathAtmConfig
214215
or
215-
query instanceof XssQuery and result instanceof XssAtm::Configuration
216+
query instanceof XssQuery and result instanceof XssAtm::DomBasedXssAtmConfig
216217
}
217218

218219
// TODO: Delete this once we are no longer surfacing `hasFlowFromSource`.

‎javascript/ql/experimental/adaptivethreatmodeling/src/NosqlInjectionATM.ql

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,8 @@ import ATM::ResultsInfo
1717
import DataFlow::PathGraph
1818
import experimental.adaptivethreatmodeling.NosqlInjectionATM
1919

20-
from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score
21-
where
22-
cfg.hasFlowPath(source, sink) and
23-
not isFlowLikelyInBaseQuery(source.getNode(), sink.getNode()) and
24-
score = getScoreForFlow(source.getNode(), sink.getNode())
20+
from AtmConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score
21+
where cfg.getAlerts(source, sink, score)
2522
select sink.getNode(), source, sink,
2623
"(Experimental) This may be a database query that depends on $@. Identified using machine learning.",
2724
source.getNode(), "a user-provided value", score

‎javascript/ql/experimental/adaptivethreatmodeling/src/SqlInjectionATM.ql

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,8 @@ import experimental.adaptivethreatmodeling.SqlInjectionATM
1717
import ATM::ResultsInfo
1818
import DataFlow::PathGraph
1919

20-
from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score
21-
where
22-
cfg.hasFlowPath(source, sink) and
23-
not isFlowLikelyInBaseQuery(source.getNode(), sink.getNode()) and
24-
score = getScoreForFlow(source.getNode(), sink.getNode())
20+
from AtmConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score
21+
where cfg.getAlerts(source, sink, score)
2522
select sink.getNode(), source, sink,
2623
"(Experimental) This may be a database query that depends on $@. Identified using machine learning.",
2724
source.getNode(), "a user-provided value", score

‎javascript/ql/experimental/adaptivethreatmodeling/src/TaintedPathATM.ql

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,8 @@ import ATM::ResultsInfo
2121
import DataFlow::PathGraph
2222
import experimental.adaptivethreatmodeling.TaintedPathATM
2323

24-
from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score
25-
where
26-
cfg.hasFlowPath(source, sink) and
27-
not isFlowLikelyInBaseQuery(source.getNode(), sink.getNode()) and
28-
score = getScoreForFlow(source.getNode(), sink.getNode())
24+
from AtmConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink, float score
25+
where cfg.getAlerts(source, sink, score)
2926
select sink.getNode(), source, sink,
3027
"(Experimental) This may be a path that depends on $@. Identified using machine learning.",
3128
source.getNode(), "a user-provided value", score

0 commit comments

Comments
 (0)