Skip to content

JS: Make sanitizers no longer block taint inside an object #2919

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.

Sign up for GitHub

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 18 commits into from
Mar 23, 2020
Merged

JS: Make sanitizers no longer block taint inside an object #2919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
8da0584
JS: Add test
asgerf Feb 21, 2020
83606e7
JS: Dont use data label in taint-tracking configs
asgerf Feb 20, 2020
a195429
JS: Add test with non-guard sanitizer
asgerf Feb 21, 2020
ad2b150
JS: Add change note
asgerf Feb 26, 2020
08ad4f7
JS: Tweak other parts of change note for consistency
asgerf Feb 26, 2020
4e75fe3
JS: Update some qldoc comments
asgerf Feb 26, 2020
0edb765
JS: Split test case function in two
asgerf Mar 4, 2020
de7fbce
JS: Adjust whitespace in change notes
asgerf Mar 4, 2020
a9901a4
JS: Update TaintBarriers/isBarrier test
asgerf Mar 4, 2020
18eea96
JS: Autoformat
asgerf Mar 7, 2020
3e68072
JS: Accept test case change
asgerf Mar 8, 2020
a7e337a
JS: Add some lines in test case
asgerf Mar 8, 2020
0280221
JS: Add variant of test that passes
asgerf Mar 8, 2020
506ddaf
JS: Add explanation for test failure
asgerf Mar 8, 2020
7393844
JS: Update some queries that used data as source
asgerf Mar 18, 2020
b6ca4fb
JS: Add getDefaultSourceLabel()
asgerf Mar 18, 2020
3ae33e3
JS: Update prototype pollution query
asgerf Mar 18, 2020
4f42675
JS: Autformat
asgerf Mar 19, 2020
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
21 changes: 14 additions & 7 deletions change-notes/1.24/analysis-javascript.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,18 @@

* Alert suppression can now be done with single-line block comments (`/* ... */`) as well as line comments (`// ...`).

* Imports with the `.js` extension can now be resolved to a TypeScript file,
* Resolution of imports has improved, leading to more results from the security queries:
- Imports with the `.js` extension can now be resolved to a TypeScript file,
when the import refers to a file generated by TypeScript.
- Imports that rely on path-mappings from a `tsconfig.json` file can now be resolved.
- Export declarations of the form `export * as ns from "x"` are now analyzed more precisely.

* Imports that rely on path-mappings from a `tsconfig.json` file can now be resolved.
* The analysis of sanitizers has improved, leading to more accurate results from the security queries.
In particular:
- Sanitizer guards now act across function boundaries in more cases.
- Sanitizers can now better distinguish between a tainted value and an object _containing_ a tainted value.

* Export declarations of the form `export * as ns from "x"` are now analyzed more precisely.

* The analysis of sanitizer guards has improved, leading to fewer false-positive results from the security queries.

* The call graph construction has been improved, leading to more results from the security queries:
* Call graph construction has been improved, leading to more results from the security queries:
- Calls can now be resolved to indirectly-defined class members in more cases.
- Calls through partial invocations such as `.bind` can now be resolved in more cases.

Expand Down Expand Up @@ -85,3 +87,8 @@

* The predicates `RegExpTerm.getSuccessor` and `RegExpTerm.getPredecessor` have been changed to reflect textual, not operational, matching order. This only makes a difference in lookbehind assertions, which are operationally matched backwards. Previously, `getSuccessor` would mimick this, so in an assertion `(?<=ab)` the term `b` would be considered the predecessor, not the successor, of `a`. Textually, however, `a` is still matched before `b`, and this is the order we now follow.
* An extensible model of the `EventEmitter` pattern has been implemented.
* Taint-tracking configurations now interact differently with the `data` flow label, which may affect queries
that combine taint-tracking and flow labels.
- Sources added by the 1-argument `isSource` predicate are associated with the `taint` label now, instead of the `data` label.
- Sanitizers now only block the `taint` label. As a result, sanitizers no longer block the flow of tainted values wrapped inside a property of an object.
To retain the old behavior, instead use a barrier, or block the `data` flow label using a labeled sanitizer.
21 changes: 17 additions & 4 deletions javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,17 @@ abstract class Configuration extends string {
*/
predicate isSource(DataFlow::Node source) { none() }

/**
* Gets the flow label to associate with sources added by the 1-argument `isSource` predicate.
*
* For taint-tracking configurations, this defaults to `taint` and for other data-flow configurations
* it defaults to `data`.
*
* Overriding this predicate is rarely needed, and overriding the 2-argument `isSource` predicate
* should be preferred when possible.
*/
FlowLabel getDefaultSourceLabel() { result = FlowLabel::data() }

/**
* Holds if `source` is a source of flow labeled with `lbl` that is relevant
* for this configuration.
Expand Down Expand Up @@ -256,9 +267,11 @@ abstract class Configuration extends string {
/**
* A label describing the kind of information tracked by a flow configuration.
*
* There are two standard labels "data" and "taint", the former describing values
* that directly originate from a flow source, the latter values that are derived
* from a flow source via one or more transformations (such as string operations).
* There are two standard labels "data" and "taint".
* - "data" only propagates along value-preserving data flow, such as assignments
* and parameter-passing, and is the default flow source for a `DataFlow::Configuration`.
* - "taint" additionally permits flow through transformations such as string operations,
* and is the default flow source for a `TaintTracking::Configuration`.
*/
abstract class FlowLabel extends string {
bindingset[this]
Expand Down Expand Up @@ -666,7 +679,7 @@ private predicate exploratoryFlowStep(
*/
private predicate isSource(DataFlow::Node nd, DataFlow::Configuration cfg, FlowLabel lbl) {
(cfg.isSource(nd) or nd.(AdditionalSource).isSourceFor(cfg)) and
lbl = FlowLabel::data()
lbl = cfg.getDefaultSourceLabel()
or
nd.(AdditionalSource).isSourceFor(cfg, lbl)
or
Expand Down
47 changes: 33 additions & 14 deletions javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,16 @@ module TaintTracking {
// overridden to provide taint-tracking specific qldoc
override predicate isSink(DataFlow::Node sink) { super.isSink(sink) }

/** Holds if the intermediate node `node` is a taint sanitizer. */
/**
* Holds if the intermediate node `node` is a taint sanitizer, that is,
* tainted values can not flow into or out of `node`.
*
* Note that this only blocks flow through nodes that operate directly on the tainted value.
* An object _containing_ a tainted value in a property can still flow into and out of `node`.
* To block such objects, override `isBarrier` or use a labeled sanitizer to block the `data` flow label.
*
* For operations that _check_ if a value is tainted or safe, use `isSanitizerGuard` instead.
*/
predicate isSanitizer(DataFlow::Node node) { none() }

/**
Expand Down Expand Up @@ -84,25 +93,35 @@ module TaintTracking {
* For example, if `guard` is the comparison expression in
* `if(x == 'some-constant'){ ... x ... }`, it could sanitize flow of
* `x` into the "then" branch.
*
* Node that this only handles checks that operate directly on the tainted value.
* Objects that _contain_ a tainted value in a property may still flow across the check.
* To block such objects, use a labeled sanitizer guard to block the `data` label.
*/
predicate isSanitizerGuard(SanitizerGuardNode guard) { none() }

final override predicate isBarrier(DataFlow::Node node) {
super.isBarrier(node) or
isSanitizer(node) or
node instanceof DataFlow::VarAccessBarrier
override predicate isLabeledBarrier(DataFlow::Node node, DataFlow::FlowLabel lbl) {
super.isLabeledBarrier(node, lbl)
or
isSanitizer(node) and lbl.isTaint()
}

final override predicate isBarrierEdge(DataFlow::Node source, DataFlow::Node sink) {
super.isBarrierEdge(source, sink) or
isSanitizerEdge(source, sink)
override predicate isBarrier(DataFlow::Node node) {
super.isBarrier(node)
or
// For variable accesses we block both the data and taint label, as a falsy value
// can't be an object, and thus can't have any tainted properties.
node instanceof DataFlow::VarAccessBarrier
}

final override predicate isBarrierEdge(
DataFlow::Node source, DataFlow::Node sink, DataFlow::FlowLabel lbl
) {
super.isBarrierEdge(source, sink, lbl) or
super.isBarrierEdge(source, sink, lbl)
or
isSanitizerEdge(source, sink, lbl)
or
isSanitizerEdge(source, sink) and lbl.isTaint()
}

final override predicate isBarrierGuard(DataFlow::BarrierGuardNode guard) {
Expand All @@ -127,6 +146,8 @@ module TaintTracking {
) {
isAdditionalFlowStep(pred, succ) and valuePreserving = false
}

override DataFlow::FlowLabel getDefaultSourceLabel() { result.isTaint() }
}

/**
Expand Down Expand Up @@ -157,7 +178,7 @@ module TaintTracking {
* them.
*/
abstract class SanitizerGuardNode extends DataFlow::BarrierGuardNode {
override predicate blocks(boolean outcome, Expr e) { sanitizes(outcome, e) }
override predicate blocks(boolean outcome, Expr e) { none() }

/**
* Holds if this node sanitizes expression `e`, provided it evaluates
Expand All @@ -166,6 +187,8 @@ module TaintTracking {
abstract predicate sanitizes(boolean outcome, Expr e);

override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
sanitizes(outcome, e) and label.isTaint()
or
sanitizes(outcome, e, label)
}

Expand All @@ -180,10 +203,6 @@ module TaintTracking {
* A sanitizer guard node that only blocks specific flow labels.
*/
abstract class LabeledSanitizerGuardNode extends SanitizerGuardNode, DataFlow::BarrierGuardNode {
final override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
sanitizes(outcome, e, label)
}

override predicate sanitizes(boolean outcome, Expr e) { none() }
}

Expand Down
10 changes: 5 additions & 5 deletions javascript/ql/src/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ module CleartextLogging {
* A data flow sink for clear-text logging of sensitive information.
*/
abstract class Sink extends DataFlow::Node {
DataFlow::FlowLabel getLabel() { result.isDataOrTaint() }
DataFlow::FlowLabel getLabel() { result.isTaint() }
}

/**
Expand Down Expand Up @@ -127,7 +127,7 @@ module CleartextLogging {

override string describe() { result = "an access to " + name }

override DataFlow::FlowLabel getLabel() { result.isData() }
override DataFlow::FlowLabel getLabel() { result.isTaint() }
}

/** An access to a variable or property that might contain a password. */
Expand All @@ -153,7 +153,7 @@ module CleartextLogging {

override string describe() { result = "an access to " + name }

override DataFlow::FlowLabel getLabel() { result.isData() }
override DataFlow::FlowLabel getLabel() { result.isTaint() }
}

/** A call that might return a password. */
Expand All @@ -167,7 +167,7 @@ module CleartextLogging {

override string describe() { result = "a call to " + name }

override DataFlow::FlowLabel getLabel() { result.isData() }
override DataFlow::FlowLabel getLabel() { result.isTaint() }
}

/** An access to the sensitive object `process.env`. */
Expand All @@ -177,7 +177,7 @@ module CleartextLogging {
override string describe() { result = "process environment" }

override DataFlow::FlowLabel getLabel() {
result.isData() or
result.isTaint() or
result instanceof PartiallySensitiveMap
}
}
Expand Down
2 changes: 1 addition & 1 deletion javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutionCustomizations.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ module PrototypePollution {
private class RemoteFlowAsSource extends Source {
RemoteFlowAsSource() { this instanceof RemoteFlowSource }

override DataFlow::FlowLabel getAFlowLabel() { result.isData() }
override DataFlow::FlowLabel getAFlowLabel() { result.isTaint() }
}

/**
Expand Down
4 changes: 2 additions & 2 deletions javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ module UnsafeDynamicMethodAccess {
hasUnsafeMethods(read.getBase().getALocalSource()) and
src = read.getPropertyNameExpr().flow() and
dst = read and
(srclabel = data() or srclabel = taint()) and
srclabel.isTaint() and
dstlabel = unsafeFunction()
)
or
Expand All @@ -62,7 +62,7 @@ module UnsafeDynamicMethodAccess {
not PropertyInjection::isPrototypeLessObject(proj.getObject().getALocalSource()) and
src = proj.getASelector() and
dst = proj and
(srclabel = data() or srclabel = taint()) and
srclabel.isTaint() and
dstlabel = unsafeFunction()
)
}
Expand Down
2 changes: 1 addition & 1 deletion ...pt/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccessCustomizations.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ module UnsafeDynamicMethodAccess {
/**
* Gets the flow label relevant for this source.
*/
DataFlow::FlowLabel getFlowLabel() { result = data() }
DataFlow::FlowLabel getFlowLabel() { result = taint() }
}

/**
Expand Down
2 changes: 1 addition & 1 deletion javascript/ql/src/semmle/javascript/security/dataflow/UnvalidatedDynamicMethodCall.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ module UnvalidatedDynamicMethodCall {
exists(DataFlow::PropRead read |
src = read.getPropertyNameExpr().flow() and
dst = read and
(srclabel = data() or srclabel = taint()) and
srclabel.isTaint() and
(
dstlabel instanceof MaybeNonFunction
or
Expand Down
2 changes: 1 addition & 1 deletion ...ql/src/semmle/javascript/security/dataflow/UnvalidatedDynamicMethodCallCustomizations.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ module UnvalidatedDynamicMethodCall {
/**
* Gets the flow label relevant for this source.
*/
DataFlow::FlowLabel getFlowLabel() { result = data() }
DataFlow::FlowLabel getFlowLabel() { result = taint() }
}

/**
Expand Down
88 changes: 45 additions & 43 deletions javascript/ql/test/library-tests/TaintBarriers/isBarrier.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,43 +1,45 @@
| tst.js:6:14:6:14 | v | ExampleConfiguration |
| tst.js:14:14:14:14 | v | ExampleConfiguration |
| tst.js:24:14:24:14 | v | ExampleConfiguration |
| tst.js:36:14:36:14 | v | ExampleConfiguration |
| tst.js:50:14:50:14 | v | ExampleConfiguration |
| tst.js:56:14:56:14 | v | ExampleConfiguration |
| tst.js:60:14:60:14 | v | ExampleConfiguration |
| tst.js:74:14:74:14 | v | ExampleConfiguration |
| tst.js:80:14:80:14 | v | ExampleConfiguration |
| tst.js:84:14:84:14 | v | ExampleConfiguration |
| tst.js:96:14:96:14 | v | ExampleConfiguration |
| tst.js:108:14:108:14 | v | ExampleConfiguration |
| tst.js:120:14:120:14 | v | ExampleConfiguration |
| tst.js:132:14:132:14 | v | ExampleConfiguration |
| tst.js:134:14:134:16 | v.p | ExampleConfiguration |
| tst.js:136:14:136:18 | v.p.q | ExampleConfiguration |
| tst.js:148:9:148:27 | v | ExampleConfiguration |
| tst.js:149:14:149:14 | v | ExampleConfiguration |
| tst.js:154:9:154:27 | v | ExampleConfiguration |
| tst.js:157:14:157:14 | v | ExampleConfiguration |
| tst.js:160:9:160:30 | v | ExampleConfiguration |
| tst.js:160:35:160:56 | v | ExampleConfiguration |
| tst.js:167:14:167:14 | v | ExampleConfiguration |
| tst.js:176:18:176:18 | v | ExampleConfiguration |
| tst.js:185:14:185:14 | v | ExampleConfiguration |
| tst.js:193:14:193:14 | v | ExampleConfiguration |
| tst.js:205:14:205:14 | v | ExampleConfiguration |
| tst.js:209:14:209:14 | v | ExampleConfiguration |
| tst.js:217:14:217:14 | v | ExampleConfiguration |
| tst.js:221:14:221:14 | v | ExampleConfiguration |
| tst.js:229:14:229:14 | v | ExampleConfiguration |
| tst.js:237:14:237:14 | v | ExampleConfiguration |
| tst.js:241:14:241:14 | v | ExampleConfiguration |
| tst.js:255:14:255:14 | v | ExampleConfiguration |
| tst.js:265:14:265:14 | v | ExampleConfiguration |
| tst.js:284:14:284:14 | v | ExampleConfiguration |
| tst.js:331:14:331:14 | v | ExampleConfiguration |
| tst.js:350:14:350:14 | v | ExampleConfiguration |
| tst.js:356:16:356:27 | x10 | ExampleConfiguration |
| tst.js:356:32:356:34 | x10 | ExampleConfiguration |
| tst.js:361:14:361:14 | v | ExampleConfiguration |
| tst.js:371:14:371:16 | o.p | ExampleConfiguration |
| tst.js:378:14:378:17 | o[p] | ExampleConfiguration |
isBarrier
isLabeledBarrier
| ExampleConfiguration | tst.js:6:14:6:14 | v | taint |
| ExampleConfiguration | tst.js:14:14:14:14 | v | taint |
| ExampleConfiguration | tst.js:24:14:24:14 | v | taint |
| ExampleConfiguration | tst.js:36:14:36:14 | v | taint |
| ExampleConfiguration | tst.js:50:14:50:14 | v | taint |
| ExampleConfiguration | tst.js:56:14:56:14 | v | taint |
| ExampleConfiguration | tst.js:60:14:60:14 | v | taint |
| ExampleConfiguration | tst.js:74:14:74:14 | v | taint |
| ExampleConfiguration | tst.js:80:14:80:14 | v | taint |
| ExampleConfiguration | tst.js:84:14:84:14 | v | taint |
| ExampleConfiguration | tst.js:96:14:96:14 | v | taint |
| ExampleConfiguration | tst.js:108:14:108:14 | v | taint |
| ExampleConfiguration | tst.js:120:14:120:14 | v | taint |
| ExampleConfiguration | tst.js:132:14:132:14 | v | taint |
| ExampleConfiguration | tst.js:134:14:134:16 | v.p | taint |
| ExampleConfiguration | tst.js:136:14:136:18 | v.p.q | taint |
| ExampleConfiguration | tst.js:148:9:148:27 | v | taint |
| ExampleConfiguration | tst.js:149:14:149:14 | v | taint |
| ExampleConfiguration | tst.js:154:9:154:27 | v | taint |
| ExampleConfiguration | tst.js:157:14:157:14 | v | taint |
| ExampleConfiguration | tst.js:160:9:160:30 | v | taint |
| ExampleConfiguration | tst.js:160:35:160:56 | v | taint |
| ExampleConfiguration | tst.js:167:14:167:14 | v | taint |
| ExampleConfiguration | tst.js:176:18:176:18 | v | taint |
| ExampleConfiguration | tst.js:185:14:185:14 | v | taint |
| ExampleConfiguration | tst.js:193:14:193:14 | v | taint |
| ExampleConfiguration | tst.js:205:14:205:14 | v | taint |
| ExampleConfiguration | tst.js:209:14:209:14 | v | taint |
| ExampleConfiguration | tst.js:217:14:217:14 | v | taint |
| ExampleConfiguration | tst.js:221:14:221:14 | v | taint |
| ExampleConfiguration | tst.js:229:14:229:14 | v | taint |
| ExampleConfiguration | tst.js:237:14:237:14 | v | taint |
| ExampleConfiguration | tst.js:241:14:241:14 | v | taint |
| ExampleConfiguration | tst.js:255:14:255:14 | v | taint |
| ExampleConfiguration | tst.js:265:14:265:14 | v | taint |
| ExampleConfiguration | tst.js:284:14:284:14 | v | taint |
| ExampleConfiguration | tst.js:331:14:331:14 | v | taint |
| ExampleConfiguration | tst.js:350:14:350:14 | v | taint |
| ExampleConfiguration | tst.js:356:16:356:27 | x10 | taint |
| ExampleConfiguration | tst.js:356:32:356:34 | x10 | taint |
| ExampleConfiguration | tst.js:361:14:361:14 | v | taint |
| ExampleConfiguration | tst.js:371:14:371:16 | o.p | taint |
| ExampleConfiguration | tst.js:378:14:378:17 | o[p] | taint |
10 changes: 7 additions & 3 deletions javascript/ql/test/library-tests/TaintBarriers/isBarrier.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import javascript
import ExampleConfiguration

from ExampleConfiguration cfg, DataFlow::Node n
where cfg.isBarrier(n)
select n, cfg
query predicate isBarrier(ExampleConfiguration cfg, DataFlow::Node n) { cfg.isBarrier(n) }

query predicate isLabeledBarrier(
ExampleConfiguration cfg, DataFlow::Node n, DataFlow::FlowLabel label
) {
cfg.isLabeledBarrier(n, label)
}
2 changes: 2 additions & 0 deletions javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ typeInferenceMismatch
| nested-props.js:43:13:43:20 | source() | nested-props.js:44:10:44:18 | id(obj).x |
| nested-props.js:67:31:67:38 | source() | nested-props.js:68:10:68:10 | x |
| nested-props.js:77:36:77:43 | source() | nested-props.js:78:10:78:10 | x |
| object-bypass-sanitizer.js:35:29:35:36 | source() | object-bypass-sanitizer.js:23:14:23:20 | obj.foo |
| object-bypass-sanitizer.js:35:29:35:36 | source() | object-bypass-sanitizer.js:28:10:28:30 | sanitiz ... bj).foo |
| partialCalls.js:4:17:4:24 | source() | partialCalls.js:17:14:17:14 | x |
| partialCalls.js:4:17:4:24 | source() | partialCalls.js:20:14:20:14 | y |
| partialCalls.js:4:17:4:24 | source() | partialCalls.js:30:14:30:20 | x.value |
Expand Down
4 changes: 4 additions & 0 deletions javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ class BasicConfig extends TaintTracking::Configuration {
node instanceof UntaintableNode
}

override predicate isSanitizer(DataFlow::Node node) {
node.(DataFlow::InvokeNode).getCalleeName().matches("sanitizer_%")
}

override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode node) {
node instanceof BasicSanitizerGuard
}
Expand Down
Loading