Skip to content

Commit 2a2a4d2

Browse files
asgerfasgerf
committed
JS: Add TaintedUrlSuffixCustomizations
Importing TaintedUrlSuffix.qll causes the flow label to materialised in unrelated queries, so: - Renames TaintedUrlSuffix.qll to TaintedUrlSuffixCustomizations.qll - Make the flow label class abstract - Adds a new TaintedUrlSuffix.qll that re-exports the above file and also materialises the flow label - Import the *Customizations.qll file from contexts where we don't want to materialise the flow label
1 parent d169401 commit 2a2a4d2

File tree

4 files changed

+121
-104
lines changed

4 files changed

+121
-104
lines changed

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

Lines changed: 3 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -4,114 +4,15 @@
44
*/
55

66
import javascript
7-
private import semmle.javascript.dataflow.internal.DataFlowPrivate as DataFlowPrivate
87

98
/**
109
* Provides a flow label for reasoning about URLs with a tainted query and fragment part,
1110
* which we collectively refer to as the "suffix" of the URL.
1211
*/
1312
module TaintedUrlSuffix {
14-
private import DataFlow
13+
import TaintedUrlSuffixCustomizations::TaintedUrlSuffix
1514

16-
/**
17-
* The flow label representing a URL with a tainted query and fragment part.
18-
*
19-
* Can also be accessed using `TaintedUrlSuffix::label()`.
20-
*/
21-
class TaintedUrlSuffixLabel extends FlowLabel {
22-
TaintedUrlSuffixLabel() { this = "tainted-url-suffix" }
23-
}
24-
25-
/**
26-
* Gets the flow label representing a URL with a tainted query and fragment part.
27-
*/
28-
FlowLabel label() { result instanceof TaintedUrlSuffixLabel }
29-
30-
/** Gets a remote flow source that is a tainted URL query or fragment part from `window.location`. */
31-
ClientSideRemoteFlowSource source() {
32-
result = DOM::locationRef().getAPropertyRead(["search", "hash"])
33-
or
34-
result = DOM::locationSource()
35-
or
36-
result.getKind().isUrl()
37-
}
38-
39-
/**
40-
* Holds if `node` should be a barrier for the given `label`.
41-
*
42-
* This should be used in the `isBarrier` predicate of a configuration that uses the tainted-url-suffix
43-
* label.
44-
*/
45-
predicate isBarrier(Node node, FlowLabel label) {
46-
label = label() and
47-
DataFlowPrivate::optionalBarrier(node, "split-url-suffix")
48-
}
49-
50-
/**
51-
* Holds if there is a flow step `src -> dst` involving the URL suffix taint label.
52-
*
53-
* This handles steps through string operations, promises, URL parsers, and URL accessors.
54-
*/
55-
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)
64-
srclbl = label() and
65-
dstlbl.isTaint() and
66-
DataFlowPrivate::optionalStep(src, "split-url-suffix-post", dst)
67-
or
68-
// Transition from URL suffix to full taint when extracting the query/fragment part.
69-
srclbl = label() and
70-
dstlbl.isTaint() and
71-
(
72-
exists(MethodCallNode call, string name |
73-
src = call.getReceiver() and
74-
dst = call and
75-
name = call.getMethodName()
76-
|
77-
// Substring that is not a prefix
78-
name = StringOps::substringMethodName() and
79-
not call.getArgument(0).getIntValue() = 0
80-
or
81-
// Replace '#' and '?' with nothing
82-
name = "replace" and
83-
call.getArgument(0).getStringValue() = ["#", "?"] and
84-
call.getArgument(1).getStringValue() = ""
85-
or
86-
// The `get` call in `url.searchParams.get(x)` and `url.hashParams.get(x)`
87-
// The step should be safe since nothing else reachable by this flow label supports a method named 'get'.
88-
name = "get"
89-
or
90-
// Methods on URL objects from the Closure library
91-
name = "getDecodedQuery"
92-
or
93-
name = "getFragment"
94-
or
95-
name = "getParameterValue"
96-
or
97-
name = "getParameterValues"
98-
or
99-
name = "getQueryData"
100-
)
101-
or
102-
exists(PropRead read |
103-
src = read.getBase() and
104-
dst = read and
105-
// Unlike the `search` property, the `query` property from `url.parse` does not include the `?`.
106-
read.getPropertyName() = "query"
107-
)
108-
or
109-
// Assume calls to regexp.exec always extract query/fragment parameters.
110-
exists(MethodCallNode call |
111-
call = any(DataFlow::RegExpCreationNode re).getAMethodCall("exec") and
112-
src = call.getArgument(0) and
113-
dst = call
114-
)
115-
)
15+
private class ConcreteTaintedUrlSuffixLabel extends TaintedUrlSuffixLabel {
16+
ConcreteTaintedUrlSuffixLabel() { this = this }
11617
}
11718
}

‎javascript/ql/lib/semmle/javascript/security/TaintedUrlSuffixCustomizations.qll

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
/**
2+
* Provides a flow label for reasoning about URLs with a tainted query and fragment part,
3+
* which we collectively refer to as the "suffix" of the URL.
4+
*/
5+
6+
import javascript
7+
private import semmle.javascript.dataflow.internal.DataFlowPrivate as DataFlowPrivate
8+
9+
/**
10+
* Provides a flow label for reasoning about URLs with a tainted query and fragment part,
11+
* which we collectively refer to as the "suffix" of the URL.
12+
*/
13+
module TaintedUrlSuffix {
14+
private import DataFlow
15+
16+
/**
17+
* The flow label representing a URL with a tainted query and fragment part.
18+
*
19+
* Can also be accessed using `TaintedUrlSuffix::label()`.
20+
*/
21+
abstract class TaintedUrlSuffixLabel extends FlowLabel {
22+
TaintedUrlSuffixLabel() { this = "tainted-url-suffix" }
23+
}
24+
25+
/**
26+
* Gets the flow label representing a URL with a tainted query and fragment part.
27+
*/
28+
FlowLabel label() { result instanceof TaintedUrlSuffixLabel }
29+
30+
/** Gets a remote flow source that is a tainted URL query or fragment part from `window.location`. */
31+
ClientSideRemoteFlowSource source() {
32+
result = DOM::locationRef().getAPropertyRead(["search", "hash"])
33+
or
34+
result = DOM::locationSource()
35+
or
36+
result.getKind().isUrl()
37+
}
38+
39+
/**
40+
* Holds if `node` should be a barrier for the given `label`.
41+
*
42+
* This should be used in the `isBarrier` predicate of a configuration that uses the tainted-url-suffix
43+
* label.
44+
*/
45+
predicate isBarrier(Node node, FlowLabel label) {
46+
label = label() and
47+
DataFlowPrivate::optionalBarrier(node, "split-url-suffix")
48+
}
49+
50+
/**
51+
* Holds if there is a flow step `src -> dst` involving the URL suffix taint label.
52+
*
53+
* This handles steps through string operations, promises, URL parsers, and URL accessors.
54+
*/
55+
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)
64+
srclbl = label() and
65+
dstlbl.isTaint() and
66+
DataFlowPrivate::optionalStep(src, "split-url-suffix-post", dst)
67+
or
68+
// Transition from URL suffix to full taint when extracting the query/fragment part.
69+
srclbl = label() and
70+
dstlbl.isTaint() and
71+
(
72+
exists(MethodCallNode call, string name |
73+
src = call.getReceiver() and
74+
dst = call and
75+
name = call.getMethodName()
76+
|
77+
// Substring that is not a prefix
78+
name = StringOps::substringMethodName() and
79+
not call.getArgument(0).getIntValue() = 0
80+
or
81+
// Replace '#' and '?' with nothing
82+
name = "replace" and
83+
call.getArgument(0).getStringValue() = ["#", "?"] and
84+
call.getArgument(1).getStringValue() = ""
85+
or
86+
// The `get` call in `url.searchParams.get(x)` and `url.hashParams.get(x)`
87+
// The step should be safe since nothing else reachable by this flow label supports a method named 'get'.
88+
name = "get"
89+
or
90+
// Methods on URL objects from the Closure library
91+
name = "getDecodedQuery"
92+
or
93+
name = "getFragment"
94+
or
95+
name = "getParameterValue"
96+
or
97+
name = "getParameterValues"
98+
or
99+
name = "getQueryData"
100+
)
101+
or
102+
exists(PropRead read |
103+
src = read.getBase() and
104+
dst = read and
105+
// Unlike the `search` property, the `query` property from `url.parse` does not include the `?`.
106+
read.getPropertyName() = "query"
107+
)
108+
or
109+
// Assume calls to regexp.exec always extract query/fragment parameters.
110+
exists(MethodCallNode call |
111+
call = any(DataFlow::RegExpCreationNode re).getAMethodCall("exec") and
112+
src = call.getArgument(0) and
113+
dst = call
114+
)
115+
)
116+
}
117+
}

‎javascript/ql/lib/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
*/
66

77
import javascript
8-
private import semmle.javascript.security.TaintedUrlSuffix
8+
private import semmle.javascript.security.TaintedUrlSuffixCustomizations
99

1010
module ClientSideUrlRedirect {
1111
/**

‎javascript/ql/test/library-tests/FlowLabels/DefaultFlowLabels.expected

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,2 @@
11
| data |
22
| taint |
3-
| tainted-url-suffix |

0 commit comments

Comments
 (0)