hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-25 08:45:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #5432 from asgerf/js/more-string-steps

Browse Source 
Approved by erik-krogh
This commit is contained in:
CodeQL CI
2021-03-18 04:16:07 -07:00
committed by GitHub
parent 1d9f8c2d37 e30fa89405
commit 3b34bfd1c6
11 changed files with 132 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
javascript/change-notes/2021-03-17-precise-regex-replace.md Normal file
View File

@@ -0,0 +1,3 @@
lgtm,codescanning
* The analysis of regular expression-based sanitization patterns has improved,
leading to more true-positive results, in particular for the XSS queries.

60
javascript/ql/src/semmle/javascript/Regexp.qll
View File

@@ -1114,4 +1114,64 @@ module RegExp {
or
result = node.asExpr().(StringLiteral).asRegExp()
}
/**
* A character that will be analyzed by `RegExp::alwaysMatchesMetaCharacter`.
*
* Currently only `<`, `'`, and `"` are considered to be meta-characters, but new meta-characters
* can be added by subclassing this class.
*/
abstract class MetaCharacter extends string {
bindingset[this]
MetaCharacter() { any() }
/**
* Holds if the given atomic term matches this meta-character.
*
* Does not hold for derived terms like alternatives and groups.
*
* By default, `.`, `\W`, `\S`, and `\D` are considered to match any meta-character,
* but the predicate can be overridden for meta-characters where this is not the case.
*/
predicate matchedByAtom(RegExpTerm term) {
term.(RegExpConstant).getConstantValue() = this
or
term instanceof RegExpDot
or
term.(RegExpCharacterClassEscape).getValue() = ["\\W", "\\S", "\\D"]
or
exists(string lo, string hi |
term.(RegExpCharacterRange).isRange(lo, hi) and
lo <= this and
this <= hi
)
}
}
private class DefaultMetaCharacter extends MetaCharacter {
DefaultMetaCharacter() { this = ["<", "'", "\""] }
}
/**
* Holds if `term` can match any occurence of `char` within a string (not taking into account
* the context in which `term` appears).
*
* This predicate is under-approximate and never considers sequences to guarantee a match.
*/
predicate alwaysMatchesMetaCharacter(RegExpTerm term, MetaCharacter char) {
not term.getParent() instanceof RegExpSequence and // restrict size of predicate
char.matchedByAtom(term)
or
alwaysMatchesMetaCharacter(term.(RegExpGroup).getAChild(), char)
or
alwaysMatchesMetaCharacter(term.(RegExpAlt).getAlternative(), char)
or
exists(RegExpCharacterClass class_ | term = class_ |
not class_.isInverted() and
char.matchedByAtom(class_.getAChild())
or
class_.isInverted() and
not char.matchedByAtom(class_.getAChild())
)
}
}

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

@@ -107,7 +107,7 @@ class StringReplaceCall extends DataFlow::MethodCallNode {
}
/** Gets the regular expression passed as the first argument to `replace`, if any. */
DataFlow::RegExpLiteralNode getRegExp() { result.flowsTo(getArgument(0)) }
DataFlow::RegExpCreationNode getRegExp() { result.flowsTo(getArgument(0)) }
/** Gets a string that is being replaced by this call. */
string getAReplacedString() {

3
javascript/ql/src/semmle/javascript/dataflow/Nodes.qll
View File

@@ -1624,6 +1624,9 @@ class RegExpCreationNode extends DataFlow::SourceNode {
result = this.(RegExpLiteralNode).getFlags()
}
/** Holds if the constructed predicate has the `g` flag. */
predicate isGlobal() { RegExp::isGlobal(getFlags()) }
/** Gets a data flow node referring to this regular expression. */
private DataFlow::SourceNode getAReference(DataFlow::TypeTracker t) {
t.start() and

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

@@ -697,10 +697,28 @@ module TaintTracking {
name = "encodeURIComponent" or
name = "decodeURIComponent"
)
or
// In and out of .replace callbacks
exists(StringReplaceCall call |
// Into the callback if the regexp does not sanitize matches
hasWildcardReplaceRegExp(call) and
pred = call.getReceiver() and
succ = call.getReplacementCallback().getParameter(0)
or
// Out of the callback
pred = call.getReplacementCallback().getReturnNode() and
succ = call
)
)
}
}
/** Holds if the given call takes a regexp containing a wildcard. */
pragma[noinline]
private predicate hasWildcardReplaceRegExp(StringReplaceCall call) {
RegExp::isWildcardLike(call.getRegExp().getRoot().getAChild*())
}
/**
* A taint propagating data flow edge arising from string formatting.
*/

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

@@ -28,19 +28,13 @@ module Shared {
abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode { }
/**
* A global regexp replacement involving an HTML meta-character, viewed as a sanitizer for
* A global regexp replacement involving the `<`, `'`, or `"` meta-character, viewed as a sanitizer for
* XSS vulnerabilities.
*
* 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, StringReplaceCall {
MetacharEscapeSanitizer() {
this.isGlobal() and
exists(RegExpConstant c |
c.getLiteral() = getRegExp().asExpr() and
c.getValue().regexpMatch("['\"&<>]")
)
isGlobal() and
RegExp::alwaysMatchesMetaCharacter(getRegExp().getRoot(), ["<", "'", "\""])
}
}

5
javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected
View File

@@ -124,6 +124,11 @@ typeInferenceMismatch
| static-capture-groups.js:2:17:2:24 | source() | static-capture-groups.js:27:14:27:22 | RegExp.$1 |
| static-capture-groups.js:32:17:32:24 | source() | static-capture-groups.js:38:10:38:18 | RegExp.$1 |
| static-capture-groups.js:42:12:42:19 | source() | static-capture-groups.js:43:14:43:22 | RegExp.$1 |
| string-replace.js:3:13:3:20 | source() | string-replace.js:14:10:14:13 | data |
| string-replace.js:3:13:3:20 | source() | string-replace.js:18:10:18:13 | data |
| string-replace.js:3:13:3:20 | source() | string-replace.js:21:6:21:41 | safe(). ... taint) |
| string-replace.js:3:13:3:20 | source() | string-replace.js:22:6:22:48 | safe(). ... taint) |
| string-replace.js:3:13:3:20 | source() | string-replace.js:24:6:24:45 | taint.r ... + '!') |
| thisAssignments.js:4:17:4:24 | source() | thisAssignments.js:5:10:5:18 | obj.field |
| thisAssignments.js:7:19:7:26 | source() | thisAssignments.js:8:10:8:20 | this.field2 |
| tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x |

24
javascript/ql/test/library-tests/TaintTracking/string-replace.js Normal file
View File

@@ -0,0 +1,24 @@
import 'dummy';
let taint = source();
taint.replace('foo', data => {
sink(data); // OK - can only be the value 'foo'
});
taint.replace(/\d+/, data => {
sink(data); // OK - can only be digits
});
taint.replace(/[^a-z]+/, data => {
sink(data); // NOT OK
});
taint.replace(/&[^&]+;/, data => {
sink(data); // NOT OK
});
sink(safe().replace('foo', data => taint)); // NOT OK
sink(safe().replace('foo', data => data + taint)); // NOT OK
sink(taint.replace('foo', data => data + '!')); // NOT OK -- propagates through replace call

7
javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected
View File

@@ -269,6 +269,9 @@ nodes
| sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
| sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
| sanitiser.js:45:29:45:35 | tainted |
| sanitiser.js:48:19:48:25 | tainted |
| sanitiser.js:48:19:48:46 | tainted ... /g, '') |
| sanitiser.js:48:19:48:46 | tainted ... /g, '') |
| stored-xss.js:2:39:2:62 | documen ... .search |
| stored-xss.js:2:39:2:62 | documen ... .search |
| stored-xss.js:3:35:3:58 | documen ... .search |
@@ -889,6 +892,7 @@ edges
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:33:29:33:35 | tainted |
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:38:29:38:35 | tainted |
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:45:29:45:35 | tainted |
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:48:19:48:25 | tainted |
| sanitiser.js:16:17:16:27 | window.name | sanitiser.js:16:7:16:27 | tainted |
| sanitiser.js:16:17:16:27 | window.name | sanitiser.js:16:7:16:27 | tainted |
| sanitiser.js:23:29:23:35 | tainted | sanitiser.js:23:21:23:44 | '<b>' + ... '</b>' |
@@ -901,6 +905,8 @@ edges
| sanitiser.js:38:29:38:35 | tainted | sanitiser.js:38:21:38:44 | '<b>' + ... '</b>' |
| sanitiser.js:45:29:45:35 | tainted | sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
| sanitiser.js:45:29:45:35 | tainted | sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
| sanitiser.js:48:19:48:25 | tainted | sanitiser.js:48:19:48:46 | tainted ... /g, '') |
| sanitiser.js:48:19:48:25 | tainted | sanitiser.js:48:19:48:46 | tainted ... /g, '') |
| stored-xss.js:2:39:2:62 | documen ... .search | 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') |
| stored-xss.js:2:39:2:62 | documen ... .search | stored-xss.js:5:20:5:52 | session ... ssion') |
@@ -1310,6 +1316,7 @@ edges
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |

6
javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected
View File

@@ -276,6 +276,9 @@ nodes
| sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
| sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
| sanitiser.js:45:29:45:35 | tainted |
| sanitiser.js:48:19:48:25 | tainted |
| sanitiser.js:48:19:48:46 | tainted ... /g, '') |
| sanitiser.js:48:19:48:46 | tainted ... /g, '') |
| stored-xss.js:2:39:2:62 | documen ... .search |
| stored-xss.js:2:39:2:62 | documen ... .search |
| stored-xss.js:3:35:3:58 | documen ... .search |
@@ -913,6 +916,7 @@ edges
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:33:29:33:35 | tainted |
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:38:29:38:35 | tainted |
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:45:29:45:35 | tainted |
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:48:19:48:25 | tainted |
| sanitiser.js:16:17:16:27 | window.name | sanitiser.js:16:7:16:27 | tainted |
| sanitiser.js:16:17:16:27 | window.name | sanitiser.js:16:7:16:27 | tainted |
| sanitiser.js:23:29:23:35 | tainted | sanitiser.js:23:21:23:44 | '<b>' + ... '</b>' |
@@ -925,6 +929,8 @@ edges
| sanitiser.js:38:29:38:35 | tainted | sanitiser.js:38:21:38:44 | '<b>' + ... '</b>' |
| sanitiser.js:45:29:45:35 | tainted | sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
| sanitiser.js:45:29:45:35 | tainted | sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' |
| sanitiser.js:48:19:48:25 | tainted | sanitiser.js:48:19:48:46 | tainted ... /g, '') |
| sanitiser.js:48:19:48:25 | tainted | sanitiser.js:48:19:48:46 | tainted ... /g, '') |
| stored-xss.js:2:39:2:62 | documen ... .search | 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') |
| stored-xss.js:2:39:2:62 | documen ... .search | stored-xss.js:5:20:5:52 | session ... ssion') |

2
javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/sanitiser.js
View File

@@ -44,4 +44,6 @@ function test() {
} else {
elt.innerHTML = '<b>' + tainted + '</b>'; // NOT OK
}
elt.innerHTML = tainted.replace(/<\w+/g, ''); // NOT OK
}