Skip to content

Commit 8fe2a43

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#6433 from asgerf/js/tainted-url-suffix
Approved by erik-krogh
2 parents 00466e4 + d83f5a9 commit 8fe2a43

File tree

14 files changed

+292
-36
lines changed

14 files changed

+292
-36
lines changed

‎javascript/change-notes/2021-08-05-tainted-url-suffix.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
lgtm,codescanning
2+
* The `js/xss` query now reports fewer false positives in cases where
3+
`location.hash` flows to a jQuery `$()` call in a way that preserves
4+
the `#` prefix.

‎javascript/ql/src/semmle/javascript/StringConcatenation.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ module StringConcatenation {
3434
or
3535
exists(DataFlow::ArrayCreationNode array, DataFlow::MethodCallNode call |
3636
call = array.getAMethodCall("join") and
37-
call.getArgument(0).mayHaveStringValue("") and
37+
(call.getArgument(0).mayHaveStringValue("") or call.getNumArgument() = 0) and
3838
(
3939
// step from array element to array
4040
result = array.getElement(n) and

‎javascript/ql/src/semmle/javascript/StringOps.qll

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -802,4 +802,11 @@ module StringOps {
802802
override boolean getPolarity() { result = polarity }
803803
}
804804
}
805+
806+
/**
807+
* Gets the name of a string method which performs substring extraction.
808+
*
809+
* These are `substring`, `substr`, and `slice`.
810+
*/
811+
string substringMethodName() { result = ["substring", "substr", "slice"] }
805812
}

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

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
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+
8+
/**
9+
* Provides a flow label for reasoning about URLs with a tainted query and fragment part,
10+
* which we collectively refer to as the "suffix" of the URL.
11+
*/
12+
module TaintedUrlSuffix {
13+
private import DataFlow
14+
15+
/**
16+
* The flow label representing a URL with a tainted query and fragment part.
17+
*
18+
* Can also be accessed using `TaintedUrlSuffix::label()`.
19+
*/
20+
class TaintedUrlSuffixLabel extends FlowLabel {
21+
TaintedUrlSuffixLabel() { this = "tainted-url-suffix" }
22+
}
23+
24+
/**
25+
* Gets the flow label representing a URL with a tainted query and fragment part.
26+
*/
27+
FlowLabel label() { result instanceof TaintedUrlSuffixLabel }
28+
29+
/** Holds for `pred -> succ` is a step of form `x -> x.p` */
30+
private predicate isSafeLocationProp(DataFlow::PropRead read) {
31+
// Ignore properties that refer to the scheme, domain, port, auth, or path.
32+
exists(string name | name = read.getPropertyName() |
33+
name = "protocol" or
34+
name = "scheme" or
35+
name = "host" or
36+
name = "hostname" or
37+
name = "domain" or
38+
name = "origin" or
39+
name = "port" or
40+
name = "path" or
41+
name = "pathname" or
42+
name = "username" or
43+
name = "password" or
44+
name = "auth"
45+
)
46+
}
47+
48+
/**
49+
* Holds if there is a flow step `src -> dst` involving the URL suffix taint label.
50+
*
51+
* This handles steps through string operations, promises, URL parsers, and URL accessors.
52+
*/
53+
predicate step(Node src, Node dst, FlowLabel srclbl, FlowLabel dstlbl) {
54+
// Inherit all ordinary taint steps except `x -> x.p` steps
55+
srclbl = label() and
56+
dstlbl = label() and
57+
TaintTracking::sharedTaintStep(src, dst) and
58+
not isSafeLocationProp(dst)
59+
or
60+
// Transition from URL suffix to full taint when extracting the query/fragment part.
61+
srclbl = label() and
62+
dstlbl.isTaint() and
63+
(
64+
exists(MethodCallNode call, string name |
65+
src = call.getReceiver() and
66+
dst = call and
67+
name = call.getMethodName()
68+
|
69+
// Substring that is not a prefix
70+
name = StringOps::substringMethodName() and
71+
not call.getArgument(0).getIntValue() = 0
72+
or
73+
// Split around '#' or '?' and extract the suffix
74+
name = "split" and
75+
call.getArgument(0).getStringValue() = ["#", "?"] and
76+
not exists(call.getAPropertyRead("0")) // Avoid false flow to the prefix
77+
or
78+
// Replace '#' and '?' with nothing
79+
name = "replace" and
80+
call.getArgument(0).getStringValue() = ["#", "?"] and
81+
call.getArgument(1).getStringValue() = ""
82+
or
83+
// The `get` call in `url.searchParams.get(x)` and `url.hashParams.get(x)`
84+
// The step should be safe since nothing else reachable by this flow label supports a method named 'get'.
85+
name = "get"
86+
or
87+
// Methods on URL objects from the Closure library
88+
name = "getDecodedQuery"
89+
or
90+
name = "getFragment"
91+
or
92+
name = "getParameterValue"
93+
or
94+
name = "getParameterValues"
95+
or
96+
name = "getQueryData"
97+
)
98+
or
99+
exists(PropRead read |
100+
src = read.getBase() and
101+
dst = read and
102+
// Unlike the `search` property, the `query` property from `url.parse` does not include the `?`.
103+
read.getPropertyName() = "query"
104+
)
105+
or
106+
// Assume calls to regexp.exec always extract query/fragment parameters.
107+
exists(MethodCallNode call |
108+
call = any(RegExpLiteral re).flow().(DataFlow::SourceNode).getAMethodCall("exec") and
109+
src = call.getArgument(0) and
110+
dst = call
111+
)
112+
)
113+
}
114+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ module ClientSideUrlRedirect {
6969
// exclude all splits where only the prefix is accessed, which is safe for url-redirects.
7070
not exists(PropAccess pacc | mce = pacc.getBase() | pacc.getPropertyName() = "0")
7171
or
72-
(methodName = "substring" or methodName = "substr" or methodName = "slice") and
72+
methodName = StringOps::substringMethodName() and
7373
// exclude `location.href.substring(0, ...)` and similar, which can
7474
// never refer to the query string
7575
not mce.getArgument(0).(NumberLiteral).getIntValue() = 0

‎javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*/
55

66
import javascript
7+
private import semmle.javascript.security.TaintedUrlSuffix
78

89
module DomBasedXss {
910
import DomBasedXssCustomizations::DomBasedXss
@@ -61,26 +62,14 @@ module DomBasedXss {
6162
not source = [DOM::locationRef(), DOM::locationRef().getAPropertyRead()] and
6263
label.isTaint()
6364
or
64-
source = DOM::locationSource() and
65-
label.isData() // Require transformation before reaching sink
66-
or
67-
source = DOM::locationRef().getAPropertyRead(["hash", "search"]) and
68-
label.isData() // Require transformation before reaching sink
65+
source = [DOM::locationSource(), DOM::locationRef().getAPropertyRead(["hash", "search"])] and
66+
label = TaintedUrlSuffix::label()
6967
}
7068

7169
override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
7270
sink instanceof JQueryHtmlOrSelectorSink and label.isTaint()
7371
}
7472

75-
override predicate isAdditionalFlowStep(
76-
DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel predlbl,
77-
DataFlow::FlowLabel succlbl
78-
) {
79-
TaintTracking::sharedTaintStep(pred, succ) and
80-
predlbl.isData() and
81-
succlbl.isTaint()
82-
}
83-
8473
override predicate isSanitizer(DataFlow::Node node) {
8574
super.isSanitizer(node)
8675
or
@@ -91,15 +80,16 @@ module DomBasedXss {
9180
guard instanceof SanitizerGuard
9281
}
9382

94-
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
95-
DomBasedXss::isOptionallySanitizedEdge(pred, succ)
83+
override predicate isAdditionalFlowStep(
84+
DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
85+
) {
86+
TaintedUrlSuffix::step(src, trg, inlbl, outlbl)
9687
or
97-
// Avoid stepping from location -> location.hash, as the .hash is already treated as a source
98-
// (with a different flow label)
99-
exists(DataFlow::PropRead read |
100-
read = DOM::locationRef().getAPropertyRead(["hash", "search"]) and
101-
pred = read.getBase() and
102-
succ = read
88+
exists(DataFlow::Node operator |
89+
StringConcatenation::taintStep(src, trg, operator, _) and
90+
StringConcatenation::getOperand(operator, 0).getStringValue() = "<" + any(string s) and
91+
inlbl = TaintedUrlSuffix::label() and
92+
outlbl.isTaint()
10393
)
10494
}
10595
}

‎javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -734,15 +734,9 @@ module TaintedPath {
734734
exists(DataFlow::MethodCallNode mcn, string name |
735735
srclabel = dstlabel and dst = mcn and mcn.calls(src, name)
736736
|
737-
exists(string substringMethodName |
738-
substringMethodName = "substr" or
739-
substringMethodName = "substring" or
740-
substringMethodName = "slice"
741-
|
742-
name = substringMethodName and
743-
// to avoid very dynamic transformations, require at least one fixed index
744-
exists(mcn.getAnArgument().asExpr().getIntValue())
745-
)
737+
name = StringOps::substringMethodName() and
738+
// to avoid very dynamic transformations, require at least one fixed index
739+
exists(mcn.getAnArgument().asExpr().getIntValue())
746740
or
747741
exists(string argumentlessMethodName |
748742
argumentlessMethodName = "toLocaleLowerCase" or

‎javascript/ql/src/semmle/javascript/security/performance/PolynomialReDoSCustomizations.qll

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,9 +100,7 @@ module PolynomialReDoS {
100100
root instanceof RegExpCharacterClassEscape
101101
)
102102
or
103-
exists(string name | name = "slice" or name = "substring" or name = "substr" |
104-
this.(DataFlow::MethodCallNode).getMethodName() = name
105-
)
103+
this.(DataFlow::MethodCallNode).getMethodName() = StringOps::substringMethodName()
106104
}
107105
}
108106

‎javascript/ql/test/library-tests/StringConcatenation/ClassContainsTwo.expected

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,10 @@
3636
| tst.js:53:10:53:34 | `one ${ ... three` |
3737
| tst.js:53:19:53:23 | two |
3838
| tst.js:71:14:71:18 | "two" |
39+
| tst.js:77:15:77:37 | ["one", ... three"] |
3940
| tst.js:77:23:77:27 | "two" |
41+
| tst.js:79:12:79:16 | array |
42+
| tst.js:79:12:79:23 | array.join() |
4043
| tst.js:87:5:87:14 | x += 'two' |
4144
| tst.js:87:10:87:14 | 'two' |
4245
| tst.js:89:3:89:3 | x |

‎javascript/ql/test/library-tests/StringConcatenation/ContainsTwo.expected

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,10 @@
3636
| tst.js:53:10:53:34 | `one ${ ... three` |
3737
| tst.js:53:19:53:23 | two |
3838
| tst.js:71:14:71:18 | "two" |
39+
| tst.js:77:15:77:37 | ["one", ... three"] |
3940
| tst.js:77:23:77:27 | "two" |
41+
| tst.js:79:12:79:16 | array |
42+
| tst.js:79:12:79:23 | array.join() |
4043
| tst.js:87:5:87:14 | x += 'two' |
4144
| tst.js:87:10:87:14 | 'two' |
4245
| tst.js:89:3:89:3 | x |

0 commit comments

Comments
 (0)