Skip to content

Commit 74ab346

Browse files
committed
JS: Do not include taint steps in TaintedUrlSuffix::step
TaintedUrlSuffix is currently only used in TaintTracking configs meaning it is already propagated by taint steps. The inclusion of these taint steps here however meant that implicit reads could appear prior to any of these steps. This was is problematic for PropRead steps as an expression like x[0] could spuriously read from array element 1 via the path: x [element 1] x [empty access path] (after implicit read) x[0] (taint step through PropRead)
1 parent 2712bf8 commit 74ab346

File tree

2 files changed

+2
-18
lines changed

2 files changed

+2
-18
lines changed

javascript/ql/lib/semmle/javascript/security/TaintedUrlSuffix.qll

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -36,16 +36,6 @@ module TaintedUrlSuffix {
3636
result.getKind().isUrl()
3737
}
3838

39-
/** Holds for `pred -> succ` is a step of form `x -> x.p` */
40-
private predicate isSafeLocationProp(DataFlow::PropRead read) {
41-
// Ignore properties that refer to the scheme, domain, port, auth, or path.
42-
read.getPropertyName() =
43-
[
44-
"protocol", "scheme", "host", "hostname", "domain", "origin", "port", "path", "pathname",
45-
"username", "password", "auth"
46-
]
47-
}
48-
4939
/**
5040
* Holds if `node` should be a barrier for the given `label`.
5141
*
@@ -63,12 +53,6 @@ module TaintedUrlSuffix {
6353
* This handles steps through string operations, promises, URL parsers, and URL accessors.
6454
*/
6555
predicate step(Node src, Node dst, FlowLabel srclbl, FlowLabel dstlbl) {
66-
// Inherit all ordinary taint steps except `x -> x.p` steps
67-
srclbl = label() and
68-
dstlbl = label() and
69-
TaintTracking::AdditionalTaintStep::step(src, dst) and
70-
not isSafeLocationProp(dst)
71-
or
7256
srclbl = label() and
7357
dstlbl.isTaint() and
7458
DataFlowPrivate::optionalStep(src, "tainted-url-suffix", dst)

javascript/ql/test/library-tests/TaintedUrlSuffix/tst.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,12 @@ function t1() {
88
sink(href.split('#')[0]); // $ MISSING: flow=tainted-url-suffix
99
sink(href.split('#')[1]); // $ flow=taint
1010
sink(href.split('#').pop()); // $ flow=taint
11-
sink(href.split('#')[2]); // $ flow=taint
11+
sink(href.split('#')[2]); // $ MISSING: flow=taint // currently the split() summary only propagates to index 1
1212

1313
sink(href.split('?')[0]); // $ MISSING: flow=tainted-url-suffix
1414
sink(href.split('?')[1]); // $ flow=taint
1515
sink(href.split('?').pop()); // $ flow=taint
16-
sink(href.split('?')[2]); // $ flow=taint
16+
sink(href.split('?')[2]); // $ MISSING: flow=taint
1717

1818
sink(href.split(blah())[0]); // $ flow=tainted-url-suffix
1919
sink(href.split(blah())[1]); // $ flow=tainted-url-suffix

0 commit comments

Comments
 (0)