Skip to content

Commit 3b34bfd

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#5432 from asgerf/js/more-string-steps
Approved by erik-krogh
2 parents 1d9f8c2 + e30fa89 commit 3b34bfd

File tree

11 files changed

+132
-10
lines changed

11 files changed

+132
-10
lines changed

‎javascript/change-notes/2021-03-17-precise-regex-replace.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
lgtm,codescanning
2+
* The analysis of regular expression-based sanitization patterns has improved,
3+
leading to more true-positive results, in particular for the XSS queries.

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

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1114,4 +1114,64 @@ module RegExp {
11141114
or
11151115
result = node.asExpr().(StringLiteral).asRegExp()
11161116
}
1117+
1118+
/**
1119+
* A character that will be analyzed by `RegExp::alwaysMatchesMetaCharacter`.
1120+
*
1121+
* Currently only `<`, `'`, and `"` are considered to be meta-characters, but new meta-characters
1122+
* can be added by subclassing this class.
1123+
*/
1124+
abstract class MetaCharacter extends string {
1125+
bindingset[this]
1126+
MetaCharacter() { any() }
1127+
1128+
/**
1129+
* Holds if the given atomic term matches this meta-character.
1130+
*
1131+
* Does not hold for derived terms like alternatives and groups.
1132+
*
1133+
* By default, `.`, `\W`, `\S`, and `\D` are considered to match any meta-character,
1134+
* but the predicate can be overridden for meta-characters where this is not the case.
1135+
*/
1136+
predicate matchedByAtom(RegExpTerm term) {
1137+
term.(RegExpConstant).getConstantValue() = this
1138+
or
1139+
term instanceof RegExpDot
1140+
or
1141+
term.(RegExpCharacterClassEscape).getValue() = ["\\W", "\\S", "\\D"]
1142+
or
1143+
exists(string lo, string hi |
1144+
term.(RegExpCharacterRange).isRange(lo, hi) and
1145+
lo <= this and
1146+
this <= hi
1147+
)
1148+
}
1149+
}
1150+
1151+
private class DefaultMetaCharacter extends MetaCharacter {
1152+
DefaultMetaCharacter() { this = ["<", "'", "\""] }
1153+
}
1154+
1155+
/**
1156+
* Holds if `term` can match any occurence of `char` within a string (not taking into account
1157+
* the context in which `term` appears).
1158+
*
1159+
* This predicate is under-approximate and never considers sequences to guarantee a match.
1160+
*/
1161+
predicate alwaysMatchesMetaCharacter(RegExpTerm term, MetaCharacter char) {
1162+
not term.getParent() instanceof RegExpSequence and // restrict size of predicate
1163+
char.matchedByAtom(term)
1164+
or
1165+
alwaysMatchesMetaCharacter(term.(RegExpGroup).getAChild(), char)
1166+
or
1167+
alwaysMatchesMetaCharacter(term.(RegExpAlt).getAlternative(), char)
1168+
or
1169+
exists(RegExpCharacterClass class_ | term = class_ |
1170+
not class_.isInverted() and
1171+
char.matchedByAtom(class_.getAChild())
1172+
or
1173+
class_.isInverted() and
1174+
not char.matchedByAtom(class_.getAChild())
1175+
)
1176+
}
11171177
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ class StringReplaceCall extends DataFlow::MethodCallNode {
107107
}
108108

109109
/** Gets the regular expression passed as the first argument to `replace`, if any. */
110-
DataFlow::RegExpLiteralNode getRegExp() { result.flowsTo(getArgument(0)) }
110+
DataFlow::RegExpCreationNode getRegExp() { result.flowsTo(getArgument(0)) }
111111

112112
/** Gets a string that is being replaced by this call. */
113113
string getAReplacedString() {

‎javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1624,6 +1624,9 @@ class RegExpCreationNode extends DataFlow::SourceNode {
16241624
result = this.(RegExpLiteralNode).getFlags()
16251625
}
16261626

1627+
/** Holds if the constructed predicate has the `g` flag. */
1628+
predicate isGlobal() { RegExp::isGlobal(getFlags()) }
1629+
16271630
/** Gets a data flow node referring to this regular expression. */
16281631
private DataFlow::SourceNode getAReference(DataFlow::TypeTracker t) {
16291632
t.start() and

‎javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -697,10 +697,28 @@ module TaintTracking {
697697
name = "encodeURIComponent" or
698698
name = "decodeURIComponent"
699699
)
700+
or
701+
// In and out of .replace callbacks
702+
exists(StringReplaceCall call |
703+
// Into the callback if the regexp does not sanitize matches
704+
hasWildcardReplaceRegExp(call) and
705+
pred = call.getReceiver() and
706+
succ = call.getReplacementCallback().getParameter(0)
707+
or
708+
// Out of the callback
709+
pred = call.getReplacementCallback().getReturnNode() and
710+
succ = call
711+
)
700712
)
701713
}
702714
}
703715

716+
/** Holds if the given call takes a regexp containing a wildcard. */
717+
pragma[noinline]
718+
private predicate hasWildcardReplaceRegExp(StringReplaceCall call) {
719+
RegExp::isWildcardLike(call.getRegExp().getRoot().getAChild*())
720+
}
721+
704722
/**
705723
* A taint propagating data flow edge arising from string formatting.
706724
*/

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

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,19 +28,13 @@ module Shared {
2828
abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode { }
2929

3030
/**
31-
* A global regexp replacement involving an HTML meta-character, viewed as a sanitizer for
31+
* A global regexp replacement involving the `<`, `'`, or `"` meta-character, viewed as a sanitizer for
3232
* XSS vulnerabilities.
33-
*
34-
* The XSS queries do not attempt to reason about correctness or completeness of sanitizers,
35-
* so any such replacement stops taint propagation.
3633
*/
3734
class MetacharEscapeSanitizer extends Sanitizer, StringReplaceCall {
3835
MetacharEscapeSanitizer() {
39-
this.isGlobal() and
40-
exists(RegExpConstant c |
41-
c.getLiteral() = getRegExp().asExpr() and
42-
c.getValue().regexpMatch("['\"&<>]")
43-
)
36+
isGlobal() and
37+
RegExp::alwaysMatchesMetaCharacter(getRegExp().getRoot(), ["<", "'", "\""])
4438
}
4539
}
4640

‎javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,11 @@ typeInferenceMismatch
124124
| static-capture-groups.js:2:17:2:24 | source() | static-capture-groups.js:27:14:27:22 | RegExp.$1 |
125125
| static-capture-groups.js:32:17:32:24 | source() | static-capture-groups.js:38:10:38:18 | RegExp.$1 |
126126
| static-capture-groups.js:42:12:42:19 | source() | static-capture-groups.js:43:14:43:22 | RegExp.$1 |
127+
| string-replace.js:3:13:3:20 | source() | string-replace.js:14:10:14:13 | data |
128+
| string-replace.js:3:13:3:20 | source() | string-replace.js:18:10:18:13 | data |
129+
| string-replace.js:3:13:3:20 | source() | string-replace.js:21:6:21:41 | safe(). ... taint) |
130+
| string-replace.js:3:13:3:20 | source() | string-replace.js:22:6:22:48 | safe(). ... taint) |
131+
| string-replace.js:3:13:3:20 | source() | string-replace.js:24:6:24:45 | taint.r ... + '!') |
127132
| thisAssignments.js:4:17:4:24 | source() | thisAssignments.js:5:10:5:18 | obj.field |
128133
| thisAssignments.js:7:19:7:26 | source() | thisAssignments.js:8:10:8:20 | this.field2 |
129134
| tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x |

‎javascript/ql/test/library-tests/TaintTracking/string-replace.js

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import 'dummy';
2+
3+
let taint = source();
4+
5+
taint.replace('foo', data => {
6+
sink(data); // OK - can only be the value 'foo'
7+
});
8+
9+
taint.replace(/\d+/, data => {
10+
sink(data); // OK - can only be digits
11+
});
12+
13+
taint.replace(/[^a-z]+/, data => {
14+
sink(data); // NOT OK
15+
});
16+
17+
taint.replace(/&[^&]+;/, data => {
18+
sink(data); // NOT OK
19+
});
20+
21+
sink(safe().replace('foo', data => taint)); // NOT OK
22+
sink(safe().replace('foo', data => data + taint)); // NOT OK
23+
24+
sink(taint.replace('foo', data => data + '!')); // NOT OK -- propagates through replace call

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,9 @@ nodes
269269
| sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
270270
| sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
271271
| sanitiser.js:45:29:45:35 | tainted |
272+
| sanitiser.js:48:19:48:25 | tainted |
273+
| sanitiser.js:48:19:48:46 | tainted ... /g, '') |
274+
| sanitiser.js:48:19:48:46 | tainted ... /g, '') |
272275
| stored-xss.js:2:39:2:62 | documen ... .search |
273276
| stored-xss.js:2:39:2:62 | documen ... .search |
274277
| stored-xss.js:3:35:3:58 | documen ... .search |
@@ -889,6 +892,7 @@ edges
889892
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:33:29:33:35 | tainted |
890893
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:38:29:38:35 | tainted |
891894
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:45:29:45:35 | tainted |
895+
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:48:19:48:25 | tainted |
892896
| sanitiser.js:16:17:16:27 | window.name | sanitiser.js:16:7:16:27 | tainted |
893897
| sanitiser.js:16:17:16:27 | window.name | sanitiser.js:16:7:16:27 | tainted |
894898
| sanitiser.js:23:29:23:35 | tainted | sanitiser.js:23:21:23:44 | '<b>' + ... '</b>' |
@@ -901,6 +905,8 @@ edges
901905
| sanitiser.js:38:29:38:35 | tainted | sanitiser.js:38:21:38:44 | '<b>' + ... '</b>' |
902906
| sanitiser.js:45:29:45:35 | tainted | sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
903907
| sanitiser.js:45:29:45:35 | tainted | sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
908+
| sanitiser.js:48:19:48:25 | tainted | sanitiser.js:48:19:48:46 | tainted ... /g, '') |
909+
| sanitiser.js:48:19:48:25 | tainted | sanitiser.js:48:19:48:46 | tainted ... /g, '') |
904910
| stored-xss.js:2:39:2:62 | documen ... .search | stored-xss.js:5:20:5:52 | session ... ssion') |
905911
| stored-xss.js:2:39:2:62 | documen ... .search | stored-xss.js:5:20:5:52 | session ... ssion') |
906912
| stored-xss.js:2:39:2:62 | documen ... .search | stored-xss.js:5:20:5:52 | session ... ssion') |
@@ -1310,6 +1316,7 @@ edges
13101316
| sanitiser.js:33:21:33:44 | '<b>' + ... '</b>' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:33:21:33:44 | '<b>' + ... '</b>' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value |
13111317
| sanitiser.js:38:21:38:44 | '<b>' + ... '</b>' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:38:21:38:44 | '<b>' + ... '</b>' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value |
13121318
| sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value |
1319+
| sanitiser.js:48:19:48:46 | tainted ... /g, '') | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:48:19:48:46 | tainted ... /g, '') | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value |
13131320
| stored-xss.js:5:20:5:52 | session ... ssion') | stored-xss.js:2:39:2:62 | documen ... .search | stored-xss.js:5:20:5:52 | session ... ssion') | Cross-site scripting vulnerability due to $@. | stored-xss.js:2:39:2:62 | documen ... .search | user-provided value |
13141321
| stored-xss.js:8:20:8:48 | localSt ... local') | stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:8:20:8:48 | localSt ... local') | Cross-site scripting vulnerability due to $@. | stored-xss.js:3:35:3:58 | documen ... .search | user-provided value |
13151322
| stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" | stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:12:20:12:54 | "<a hre ... ar</a>" | Cross-site scripting vulnerability due to $@. | stored-xss.js:3:35:3:58 | documen ... .search | user-provided value |

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,9 @@ nodes
276276
| sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
277277
| sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
278278
| sanitiser.js:45:29:45:35 | tainted |
279+
| sanitiser.js:48:19:48:25 | tainted |
280+
| sanitiser.js:48:19:48:46 | tainted ... /g, '') |
281+
| sanitiser.js:48:19:48:46 | tainted ... /g, '') |
279282
| stored-xss.js:2:39:2:62 | documen ... .search |
280283
| stored-xss.js:2:39:2:62 | documen ... .search |
281284
| stored-xss.js:3:35:3:58 | documen ... .search |
@@ -913,6 +916,7 @@ edges
913916
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:33:29:33:35 | tainted |
914917
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:38:29:38:35 | tainted |
915918
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:45:29:45:35 | tainted |
919+
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:48:19:48:25 | tainted |
916920
| sanitiser.js:16:17:16:27 | window.name | sanitiser.js:16:7:16:27 | tainted |
917921
| sanitiser.js:16:17:16:27 | window.name | sanitiser.js:16:7:16:27 | tainted |
918922
| sanitiser.js:23:29:23:35 | tainted | sanitiser.js:23:21:23:44 | '<b>' + ... '</b>' |
@@ -925,6 +929,8 @@ edges
925929
| sanitiser.js:38:29:38:35 | tainted | sanitiser.js:38:21:38:44 | '<b>' + ... '</b>' |
926930
| sanitiser.js:45:29:45:35 | tainted | sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
927931
| sanitiser.js:45:29:45:35 | tainted | sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
932+
| sanitiser.js:48:19:48:25 | tainted | sanitiser.js:48:19:48:46 | tainted ... /g, '') |
933+
| sanitiser.js:48:19:48:25 | tainted | sanitiser.js:48:19:48:46 | tainted ... /g, '') |
928934
| stored-xss.js:2:39:2:62 | documen ... .search | stored-xss.js:5:20:5:52 | session ... ssion') |
929935
| stored-xss.js:2:39:2:62 | documen ... .search | stored-xss.js:5:20:5:52 | session ... ssion') |
930936
| stored-xss.js:2:39:2:62 | documen ... .search | stored-xss.js:5:20:5:52 | session ... ssion') |

0 commit comments

Comments
 (0)