Skip to content

Commit 1df69ec

Browse files
committed
JS: Actually don't propagate into array element 0
Preserving tainted-url-suffix into array element 0 seemed like a good idea, but didn't work out so well.
1 parent 0e4e0f4 commit 1df69ec

File tree

5 files changed

+34
-216
lines changed

5 files changed

+34
-216
lines changed

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,14 +53,18 @@ module TaintedUrlSuffix {
5353
* This handles steps through string operations, promises, URL parsers, and URL accessors.
5454
*/
5555
predicate step(Node src, Node dst, FlowLabel srclbl, FlowLabel dstlbl) {
56+
// Transition from tainted-url-suffix to general taint when entering the second array element
57+
// of a split('#') or split('?') array.
58+
//
59+
// x [tainted-url-suffix] --> x.split('#') [array element 1] [taint]
60+
//
61+
// Technically we should also preverse tainted-url-suffix when entering the first array element of such
62+
// a split, but this mostly leads to FPs since we currently don't track if the taint has been through URI-decoding.
63+
// (The query/fragment parts are often URI-decoded in practice, but not the other URL parts are not)
5664
srclbl = label() and
5765
dstlbl.isTaint() and
5866
DataFlowPrivate::optionalStep(src, "split-url-suffix-post", dst)
5967
or
60-
srclbl = label() and
61-
dstlbl = label() and
62-
DataFlowPrivate::optionalStep(src, "split-url-suffix-pre", dst)
63-
or
6468
// Transition from URL suffix to full taint when extracting the query/fragment part.
6569
srclbl = label() and
6670
dstlbl.isTaint() and

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,14 @@ function t1() {
55

66
sink(href); // $ flow=tainted-url-suffix
77

8-
sink(href.split('#')[0]); // $ flow=tainted-url-suffix
8+
sink(href.split('#')[0]); // could be 'tainted-url-suffix', but omitted due to FPs from URI-encoding
99
sink(href.split('#')[1]); // $ flow=taint
10-
sink(href.split('#').pop()); // $ flow=taint flow=tainted-url-suffix
10+
sink(href.split('#').pop()); // $ flow=taint
1111
sink(href.split('#')[2]); // $ MISSING: flow=taint // currently the split() summary only propagates to index 1
1212

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

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

0 commit comments

Comments
 (0)