hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-16 16:53:25 +01:00
Code Issues Packages Projects Releases Wiki Activity

add support for String.prototype.replaceAll

Browse Source
This commit is contained in:
Erik Krogh Kristensen
2020-09-17 14:00:44 +02:00
parent 0dbdbfa659
commit b09015380a
17 changed files with 41 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
javascript/ql/src/Expressions/StringInsteadOfRegex.ql
View File

@@ -31,7 +31,7 @@ predicate isStringSplitOrReplace(MethodCallExpr mce) {
mce.getMethodName() = name and
mce.getNumArgument() = arity
|
name = "replace" and arity = 2
name = ["replace", "replaceAll"] and arity = 2
or
name = "split" and
(arity = 1 or arity = 2)

2
javascript/ql/src/RegExp/IdentityReplacement.ql
View File

@@ -70,7 +70,7 @@ predicate regExpMatchesString(RegExpTerm t, string s) {
from MethodCallExpr repl, string s, string friendly
where
repl.getMethodName() = "replace" and
repl.getMethodName() = ["replace", "replaceAll"] and
matchesString(repl.getArgument(0), s) and
repl.getArgument(1).getStringValue() = s and
(if s = "" then friendly = "the empty string" else friendly = "'" + s + "'")

2
javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql
View File

@@ -75,7 +75,7 @@ DataFlow::Node schemeCheck(DataFlow::Node nd, DangerousScheme scheme) {
exists(DataFlow::MethodCallNode stringop |
stringop.getMethodName().matches("trim%") or
stringop.getMethodName().matches("to%Case") or
stringop.getMethodName() = "replace"
stringop.getMethodName() = ["replace", "replaceAll"]
|
result = schemeCheck(stringop, scheme) and
nd = stringop.getReceiver()

1
javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql
View File

@@ -79,6 +79,7 @@ predicate allBackslashesEscaped(DataFlow::Node nd) {
// flow through string methods
exists(DataFlow::MethodCallNode mc, string m |
m = "replace" or
m = "replaceAll" or
m = "slice" or
m = "substr" or
m = "substring" or

6
javascript/ql/src/semmle/javascript/StandardLibrary.qll
View File

@@ -102,7 +102,7 @@ private class IteratorExceptionStep extends DataFlow::MethodCallNode, DataFlow::
*/
class StringReplaceCall extends DataFlow::MethodCallNode {
StringReplaceCall() {
getMethodName() = "replace" and
getMethodName() = ["replace", "replaceAll"] and
(getNumArgument() = 2 or getReceiver().mayHaveStringValue(_))
}
@@ -128,9 +128,9 @@ class StringReplaceCall extends DataFlow::MethodCallNode {
/**
* Holds if this is a global replacement, that is, the first argument is a regular expression
* with the `g` flag.
* with the `g` flag, or this is a call to `.replaceAll()`.
*/
predicate isGlobal() { getRegExp().isGlobal() }
predicate isGlobal() { getRegExp().isGlobal() or getMethodName() = "replaceAll" }
/**
* Holds if this call to `replace` replaces `old` with `new`.

6
javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
View File

@@ -427,6 +427,7 @@ module TaintTracking {
name = "padStart" or
name = "repeat" or
name = "replace" or
name = "replaceAll" or
name = "slice" or
name = "small" or
name = "split" or
@@ -452,7 +453,7 @@ module TaintTracking {
exists(int i | pred.asExpr() = succ.getAstNode().(MethodCallExpr).getArgument(i) |
name = "concat"
or
name = "replace" and i = 1
name = ["replace", "replaceAll"] and i = 1
)
)
or
@@ -707,7 +708,8 @@ module TaintTracking {
*/
private ControlFlowNode getACaptureSetter(DataFlow::Node input) {
exists(DataFlow::MethodCallNode call | result = call.asExpr() |
call.getMethodName() = ["search", "replace", "match"] and input = call.getReceiver()
call.getMethodName() = ["search", "replace", "replaceAll", "match"] and
input = call.getReceiver()
or
call.getMethodName() = ["test", "exec"] and input = call.getArgument(0)
)

4
javascript/ql/src/semmle/javascript/heuristics/AdditionalTaintSteps.qll
View File

@@ -15,9 +15,7 @@ abstract class HeuristicAdditionalTaintStep extends DataFlow::ValueNode { }
* A call to `tainted.replace(x, y)` that preserves taint.
*/
private class HeuristicStringManipulationTaintStep extends HeuristicAdditionalTaintStep,
TaintTracking::AdditionalTaintStep, DataFlow::MethodCallNode {
HeuristicStringManipulationTaintStep() { getMethodName() = "replace" }
TaintTracking::AdditionalTaintStep, StringReplaceCall {
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
pred = getReceiver() and succ = this
}

12
javascript/ql/src/semmle/javascript/security/dataflow/CleartextLoggingCustomizations.qll
View File

@@ -34,15 +34,11 @@ module CleartextLogging {
/**
* A call to `.replace()` that seems to mask sensitive information.
*/
class MaskingReplacer extends Barrier, DataFlow::MethodCallNode {
class MaskingReplacer extends Barrier, StringReplaceCall {
MaskingReplacer() {
this.getCalleeName() = "replace" and
exists(RegExpLiteral reg |
reg = this.getArgument(0).getALocalSource().asExpr() and
reg.isGlobal() and
any(RegExpDot term).getLiteral() = reg
) and
exists(this.getArgument(1).getStringValue())
this.isGlobal() and
exists(this.getRawReplacement().getStringValue()) and
any(RegExpDot term).getLiteral() = getRegExp().asExpr()
}
}

18
javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
View File

@@ -218,12 +218,12 @@ module TaintedPath {
output = this
or
// non-global replace or replace of something other than /\.\./g, /[/]/g, or /[\.]/g.
this.getCalleeName() = "replace" and
this instanceof StringReplaceCall and
input = getReceiver() and
output = this and
not exists(RegExpLiteral literal, RegExpTerm term |
getArgument(0).getALocalSource().asExpr() = literal and
literal.isGlobal() and
this.(StringReplaceCall).getRegExp().asExpr() = literal and
this.(StringReplaceCall).isGlobal() and
literal.getRoot() = term
|
term.getAMatchedString() = "/" or
@@ -247,16 +247,15 @@ module TaintedPath {
/**
* A call that removes all instances of "../" in the prefix of the string.
*/
class DotDotSlashPrefixRemovingReplace extends DataFlow::CallNode {
class DotDotSlashPrefixRemovingReplace extends StringReplaceCall {
DataFlow::Node input;
DataFlow::Node output;
DotDotSlashPrefixRemovingReplace() {
this.getCalleeName() = "replace" and
input = getReceiver() and
output = this and
exists(RegExpLiteral literal, RegExpTerm term |
getArgument(0).getALocalSource().asExpr() = literal and
getRegExp().asExpr() = literal and
(term instanceof RegExpStar or term instanceof RegExpPlus) and
term.getChild(0) = getADotDotSlashMatcher()
|
@@ -298,17 +297,16 @@ module TaintedPath {
/**
* A call that removes all "." or ".." from a path, without also removing all forward slashes.
*/
class DotRemovingReplaceCall extends DataFlow::CallNode {
class DotRemovingReplaceCall extends StringReplaceCall {
DataFlow::Node input;
DataFlow::Node output;
DotRemovingReplaceCall() {
this.getCalleeName() = "replace" and
input = getReceiver() and
output = this and
isGlobal() and
exists(RegExpLiteral literal, RegExpTerm term |
getArgument(0).getALocalSource().asExpr() = literal and
literal.isGlobal() and
getRegExp().asExpr() = literal and
literal.getRoot() = term and
not term.getAMatchedString() = "/"
|

5
javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPlugin.qll
View File

@@ -39,10 +39,9 @@ module UnsafeJQueryPlugin {
StringConcatenation::taintStep(pred, succ, _, any(int i | i >= 1))
or
// prefixing through a poor-mans templating system:
exists(DataFlow::MethodCallNode replace |
exists(StringReplaceCall replace |
replace = succ and
pred = replace.getArgument(1) and
replace.getMethodName() = "replace"
pred = replace.getRawReplacement()
)
}

5
javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll
View File

@@ -33,11 +33,10 @@ module Shared {
* The XSS queries do not attempt to reason about correctness or completeness of sanitizers,
* so any such replacement stops taint propagation.
*/
class MetacharEscapeSanitizer extends Sanitizer, DataFlow::MethodCallNode {
class MetacharEscapeSanitizer extends Sanitizer, StringReplaceCall {
MetacharEscapeSanitizer() {
getMethodName() = "replace" and
exists(RegExpConstant c |
c.getLiteral() = getArgument(0).getALocalSource().asExpr() and
c.getLiteral() = getRegExp().asExpr() and
c.getValue().regexpMatch("['\"&<>]")
)
}

1
javascript/ql/src/semmle/javascript/security/performance/PolynomialReDoSCustomizations.qll
View File

@@ -51,6 +51,7 @@ module PolynomialReDoS {
name = "split" or
name = "matchAll" or
name = "replace" or
name = "replaceAll" or
name = "search"
)
or

2
javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected
View File

@@ -86,6 +86,8 @@
| tst.js:2:17:2:22 | "src1" | tst.js:61:16:61:18 | o.r |
| tst.js:2:17:2:22 | "src1" | tst.js:68:16:68:22 | inner() |
| tst.js:2:17:2:22 | "src1" | tst.js:80:16:80:22 | outer() |
| tst.js:2:17:2:22 | "src1" | tst.js:87:16:87:43 | source1 ... /g, "") |
| tst.js:2:17:2:22 | "src1" | tst.js:88:16:88:46 | "foo".r ... ource1) |
| underscore.js:2:17:2:22 | "src1" | underscore.js:3:15:3:28 | _.max(source1) |
| underscore.js:5:17:5:22 | "src2" | underscore.js:6:15:6:34 | _.union([], source2) |
| underscore.js:5:17:5:22 | "src2" | underscore.js:7:15:7:32 | _.zip(source2, []) |

3
javascript/ql/test/library-tests/InterProceduralFlow/tst.js
View File

@@ -83,4 +83,7 @@
o.notTracked = source1;
var sink22 = o.notTracked;
var sink23 = source1.replaceAll(/f/g, "");
var sink24 = "foo".replaceAll(/f/g, source1);
})();

1
javascript/ql/test/query-tests/Performance/ReDoS/PolynomialBackTracking.expected
View File

@@ -17,6 +17,7 @@
| polynomial-redos.js:31:42:31:43 | -+ | it can start matching anywhere |
| polynomial-redos.js:32:45:32:47 | \\n* | it can start matching anywhere |
| polynomial-redos.js:33:17:33:20 | (.)* | it can start matching anywhere |
| polynomial-redos.js:48:22:48:24 | \\s* | it can start matching anywhere |
| regexplib/address.js:18:26:18:31 | [ \\w]* | it can start matching anywhere after the start of the preceeding '[ \\w]{3,}' |
| regexplib/address.js:20:144:20:147 | [ ]+ | it can start matching anywhere after the start of the preceeding '[a-zA-Z0-9 \\-.]{6,}' |
| regexplib/address.js:24:26:24:31 | [ \\w]* | it can start matching anywhere after the start of the preceeding '[ \\w]{3,}' |

5
javascript/ql/test/query-tests/Performance/ReDoS/PolynomialReDoS.expected
View File

@@ -28,6 +28,8 @@ nodes
| polynomial-redos.js:30:2:30:8 | tainted |
| polynomial-redos.js:33:2:33:8 | tainted |
| polynomial-redos.js:33:2:33:8 | tainted |
| polynomial-redos.js:48:2:48:8 | tainted |
| polynomial-redos.js:48:2:48:8 | tainted |
edges
| polynomial-redos.js:5:6:5:32 | tainted | polynomial-redos.js:7:2:7:8 | tainted |
| polynomial-redos.js:5:6:5:32 | tainted | polynomial-redos.js:7:2:7:8 | tainted |
@@ -55,6 +57,8 @@ edges
| polynomial-redos.js:5:6:5:32 | tainted | polynomial-redos.js:30:2:30:8 | tainted |
| polynomial-redos.js:5:6:5:32 | tainted | polynomial-redos.js:33:2:33:8 | tainted |
| polynomial-redos.js:5:6:5:32 | tainted | polynomial-redos.js:33:2:33:8 | tainted |
| polynomial-redos.js:5:6:5:32 | tainted | polynomial-redos.js:48:2:48:8 | tainted |
| polynomial-redos.js:5:6:5:32 | tainted | polynomial-redos.js:48:2:48:8 | tainted |
| polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:5:6:5:32 | tainted |
| polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:5:6:5:32 | tainted |
#select
@@ -72,3 +76,4 @@ edges
| polynomial-redos.js:27:77:27:83 | tainted | polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:27:77:27:83 | tainted | This expensive $@ use depends on $@. | polynomial-redos.js:27:14:27:22 | [A-Z]{2,} | regular expression | polynomial-redos.js:5:16:5:32 | req.query.tainted | a user-provided value |
| polynomial-redos.js:30:2:30:8 | tainted | polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:30:2:30:8 | tainted | This expensive $@ use depends on $@. | polynomial-redos.js:30:19:30:22 | [?]+ | regular expression | polynomial-redos.js:5:16:5:32 | req.query.tainted | a user-provided value |
| polynomial-redos.js:33:2:33:8 | tainted | polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:33:2:33:8 | tainted | This expensive $@ use depends on $@. | polynomial-redos.js:33:17:33:20 | (.)* | regular expression | polynomial-redos.js:5:16:5:32 | req.query.tainted | a user-provided value |
| polynomial-redos.js:48:2:48:8 | tainted | polynomial-redos.js:5:16:5:32 | req.query.tainted | polynomial-redos.js:48:2:48:8 | tainted | This expensive $@ use depends on $@. | polynomial-redos.js:48:22:48:24 | \\s* | regular expression | polynomial-redos.js:5:16:5:32 | req.query.tainted | a user-provided value |

2
javascript/ql/test/query-tests/Performance/ReDoS/polynomial-redos.js
View File

@@ -45,5 +45,5 @@ app.use(function(req, res) {
tainted.match(/^(?:\.?[a-zA-Z_][a-zA-Z_0-9]*)+$/); // NOT OK - but not flagged
tainted.match(/^(?:\.?[a-zA-Z_][a-zA-Z_0-9]*)(?:\.[a-zA-Z_][a-zA-Z_0-9]*)*$/); // OK
tainted.replaceAll(/\s*\n\s*/g, ' '); // NOT OK
});