Skip to content

Commit 0230b79

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3391 from erik-krogh/SplitFPs
Approved by esbena
2 parents 8d41ce1 + 945fe45 commit 0230b79

File tree

17 files changed

+232
-25
lines changed

17 files changed

+232
-25
lines changed

‎change-notes/1.25/analysis-javascript.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@
2424
| Misspelled variable name (`js/misspelled-variable-name`) | Message changed | The message for this query now correctly identifies the misspelled variable in additional cases. |
2525
| Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. |
2626
| Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. |
27+
| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | Less results | This query now recognizes additional safe patterns of doing URL redirects. |
28+
| Client-side cross-site scripting (`js/xss`) | Less results | This query now recognizes additional safe strings based on URLs. |
29+
| Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes additional url scheme checks. |
30+
| Prototype pollution in utility function (`js/prototype-pollution-utility`) | More results | This query now recognizes additional utility functions as vulnerable to prototype polution. |
2731
| Expression has no effect (`js/useless-expression`) | Less results | This query no longer flags an expression when that expression is the only content of the containing file. |
2832
| Unknown directive (`js/unknown-directive`) | Less results | This query no longer flags directives generated by the Babel compiler. |
2933
| Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving NoSQL code operators are now recognized. |

‎javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,10 @@ class DangerousScheme extends string {
2727
/** Returns a node that refers to the scheme of `url`. */
2828
DataFlow::SourceNode schemeOf(DataFlow::Node url) {
2929
// url.split(":")[0]
30-
exists(DataFlow::MethodCallNode split |
31-
split.getMethodName() = "split" and
32-
split.getArgument(0).getStringValue() = ":" and
33-
result = split.getAPropertyRead("0") and
34-
url = split.getReceiver()
30+
exists(StringSplitCall split |
31+
split.getSeparator() = ":" and
32+
result = split.getASubstringRead(0) and
33+
url = split.getBaseString()
3534
)
3635
or
3736
// url.getScheme(), url.getProtocol(), getScheme(url), getProtocol(url)

‎javascript/ql/src/Security/CWE-400/PrototypePollutionUtility.ql

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,10 @@ import semmle.javascript.DynamicPropertyAccess
2121
*
2222
* We restrict this to parameter nodes to focus on "deep assignment" functions.
2323
*/
24-
class SplitCall extends MethodCallNode {
24+
class SplitCall extends StringSplitCall {
2525
SplitCall() {
26-
getMethodName() = "split" and
27-
getArgument(0).mayHaveStringValue(".") and
28-
getReceiver().getALocalSource() instanceof ParameterNode
26+
getSeparator() = "." and
27+
getBaseString().getALocalSource() instanceof ParameterNode
2928
}
3029
}
3130

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,3 +146,37 @@ class StringReplaceCall extends DataFlow::MethodCallNode {
146146
)
147147
}
148148
}
149+
150+
/**
151+
* A call to `String.prototype.split`.
152+
*
153+
* We heuristically include any call to a method called `split`, provided it either
154+
* has one or two arguments, or local data flow suggests that the receiver may be a string.
155+
*/
156+
class StringSplitCall extends DataFlow::MethodCallNode {
157+
StringSplitCall() {
158+
this.getMethodName() = "split" and
159+
(getNumArgument() = [1, 2] or getReceiver().mayHaveStringValue(_))
160+
}
161+
162+
/**
163+
* Gets a string that determines where the string is split.
164+
*/
165+
string getSeparator() {
166+
getArgument(0).mayHaveStringValue(result)
167+
or
168+
result =
169+
getArgument(0).getALocalSource().(DataFlow::RegExpCreationNode).getRoot().getAMatchedString()
170+
}
171+
172+
/**
173+
* Gets the DataFlow::Node for the base string that is split.
174+
*/
175+
DataFlow::Node getBaseString() { result = getReceiver() }
176+
177+
/**
178+
* Gets a read of the `i`th element from the split string.
179+
*/
180+
bindingset[i]
181+
DataFlow::Node getASubstringRead(int i) { result = getAPropertyRead(i.toString()) }
182+
}

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ import semmle.javascript.security.dataflow.RemoteFlowSources
99
import UrlConcatenation
1010

1111
module ClientSideUrlRedirect {
12+
private import Xss::DomBasedXss as DomBasedXss
13+
1214
/**
1315
* A data flow source for unvalidated URL redirect vulnerabilities.
1416
*/
@@ -52,7 +54,7 @@ module ClientSideUrlRedirect {
5254
mce = queryAccess.asExpr() and mce.calls(nd.asExpr(), methodName)
5355
|
5456
methodName = "split" and
55-
// exclude `location.href.split('?')[0]`, which can never refer to the query string
57+
// exclude all splits where only the prefix is accessed, which is safe for url-redirects.
5658
not exists(PropAccess pacc | mce = pacc.getBase() | pacc.getPropertyName() = "0")
5759
or
5860
(methodName = "substring" or methodName = "substr" or methodName = "slice") and
@@ -68,6 +70,11 @@ module ClientSideUrlRedirect {
6870
)
6971
}
7072

73+
/**
74+
* A sanitizer that reads the first part a location split by "?", e.g. `location.href.split('?')[0]`.
75+
*/
76+
class QueryPrefixSanitizer extends Sanitizer, DomBasedXss::QueryPrefixSanitizer { }
77+
7178
/**
7279
* A sink which is used to set the window location.
7380
*/

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

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -97,23 +97,17 @@ module TaintedPath {
9797
)
9898
)
9999
or
100+
// A `str.split()` call can either split into path elements (`str.split("/")`) or split by some other string.
101+
exists(StringSplitCall mcn | dst = mcn and mcn.getBaseString() = src |
102+
if mcn.getSeparator() = "/"
103+
then
104+
srclabel.(Label::PosixPath).canContainDotDotSlash() and
105+
dstlabel instanceof Label::SplitPath
106+
else srclabel = dstlabel
107+
)
108+
or
100109
// array method calls of interest
101110
exists(DataFlow::MethodCallNode mcn, string name | dst = mcn and mcn.calls(src, name) |
102-
// A `str.split()` call can either split into path elements (`str.split("/")`) or split by some other string.
103-
name = "split" and
104-
(
105-
if
106-
exists(DataFlow::Node splitBy | splitBy = mcn.getArgument(0) |
107-
splitBy.mayHaveStringValue("/") or
108-
any(DataFlow::RegExpCreationNode reg | reg.getRoot().getAMatchedString() = "/")
109-
.flowsTo(splitBy)
110-
)
111-
then
112-
srclabel.(Label::PosixPath).canContainDotDotSlash() and
113-
dstlabel instanceof Label::SplitPath
114-
else srclabel = dstlabel
115-
)
116-
or
117111
(
118112
name = "pop" or
119113
name = "shift"

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,20 @@ module DomBasedXss {
278278
}
279279
}
280280

281+
/**
282+
* A sanitizer that reads the first part a location split by "?", e.g. `location.href.split('?')[0]`.
283+
*/
284+
class QueryPrefixSanitizer extends Sanitizer {
285+
StringSplitCall splitCall;
286+
287+
QueryPrefixSanitizer() {
288+
this = splitCall.getASubstringRead(0) and
289+
splitCall.getSeparator() = "?" and
290+
splitCall.getBaseString().getALocalSource() =
291+
[DOM::locationRef(), DOM::locationRef().getAPropertyRead("href")]
292+
}
293+
}
294+
281295
/**
282296
* A regexp replacement involving an HTML meta-character, viewed as a sanitizer for
283297
* XSS vulnerabilities.

‎javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,4 @@
33
| IncompleteUrlSchemeCheck.js:23:9:23:43 | badProt ... scheme) | This check does not consider vbscript:. |
44
| IncompleteUrlSchemeCheck.js:30:9:30:43 | badProt ... scheme) | This check does not consider vbscript:. |
55
| IncompleteUrlSchemeCheck.js:37:9:37:31 | scheme ... script" | This check does not consider data: and vbscript:. |
6+
| IncompleteUrlSchemeCheck.js:51:9:51:31 | scheme ... script" | This check does not consider data: and vbscript:. |

‎javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,3 +45,10 @@ function test6(url) {
4545
return "about:blank";
4646
return url;
4747
}
48+
49+
function test7(url) {
50+
let scheme = url.split(/:/)[0];
51+
if (scheme === "javascript") // NOT OK
52+
return "about:blank";
53+
return url;
54+
}

‎javascript/ql/test/query-tests/Security/CWE-079/Xss.expected

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,12 @@ nodes
357357
| tst.js:366:21:366:26 | target |
358358
| tst.js:369:18:369:23 | target |
359359
| tst.js:369:18:369:23 | target |
360+
| tst.js:377:7:377:39 | target |
361+
| tst.js:377:16:377:32 | document.location |
362+
| tst.js:377:16:377:32 | document.location |
363+
| tst.js:377:16:377:39 | documen ... .search |
364+
| tst.js:380:18:380:23 | target |
365+
| tst.js:380:18:380:23 | target |
360366
| typeahead.js:20:13:20:45 | target |
361367
| typeahead.js:20:22:20:38 | document.location |
362368
| typeahead.js:20:22:20:38 | document.location |
@@ -689,6 +695,11 @@ edges
689695
| tst.js:361:19:361:35 | document.location | tst.js:361:19:361:42 | documen ... .search |
690696
| tst.js:361:19:361:35 | document.location | tst.js:361:19:361:42 | documen ... .search |
691697
| tst.js:361:19:361:42 | documen ... .search | tst.js:361:10:361:42 | target |
698+
| tst.js:377:7:377:39 | target | tst.js:380:18:380:23 | target |
699+
| tst.js:377:7:377:39 | target | tst.js:380:18:380:23 | target |
700+
| tst.js:377:16:377:32 | document.location | tst.js:377:16:377:39 | documen ... .search |
701+
| tst.js:377:16:377:32 | document.location | tst.js:377:16:377:39 | documen ... .search |
702+
| tst.js:377:16:377:39 | documen ... .search | tst.js:377:7:377:39 | target |
692703
| typeahead.js:20:13:20:45 | target | typeahead.js:21:12:21:17 | target |
693704
| typeahead.js:20:22:20:38 | document.location | typeahead.js:20:22:20:45 | documen ... .search |
694705
| typeahead.js:20:22:20:38 | document.location | typeahead.js:20:22:20:45 | documen ... .search |
@@ -794,6 +805,7 @@ edges
794805
| tst.js:362:16:362:21 | target | tst.js:361:19:361:35 | document.location | tst.js:362:16:362:21 | target | Cross-site scripting vulnerability due to $@. | tst.js:361:19:361:35 | document.location | user-provided value |
795806
| tst.js:366:21:366:26 | target | tst.js:361:19:361:35 | document.location | tst.js:366:21:366:26 | target | Cross-site scripting vulnerability due to $@. | tst.js:361:19:361:35 | document.location | user-provided value |
796807
| tst.js:369:18:369:23 | target | tst.js:361:19:361:35 | document.location | tst.js:369:18:369:23 | target | Cross-site scripting vulnerability due to $@. | tst.js:361:19:361:35 | document.location | user-provided value |
808+
| tst.js:380:18:380:23 | target | tst.js:377:16:377:32 | document.location | tst.js:380:18:380:23 | target | Cross-site scripting vulnerability due to $@. | tst.js:377:16:377:32 | document.location | user-provided value |
797809
| typeahead.js:25:18:25:20 | val | typeahead.js:20:22:20:38 | document.location | typeahead.js:25:18:25:20 | val | Cross-site scripting vulnerability due to $@. | typeahead.js:20:22:20:38 | document.location | user-provided value |
798810
| v-html.vue:2:8:2:23 | v-html=tainted | v-html.vue:6:42:6:58 | document.location | v-html.vue:2:8:2:23 | v-html=tainted | Cross-site scripting vulnerability due to $@. | v-html.vue:6:42:6:58 | document.location | user-provided value |
799811
| winjs.js:3:43:3:49 | tainted | winjs.js:2:17:2:33 | document.location | winjs.js:3:43:3:49 | tainted | Cross-site scripting vulnerability due to $@. | winjs.js:2:17:2:33 | document.location | user-provided value |

0 commit comments

Comments
 (0)