From e60628d4633511b2c65998b46bcd43b294481a6c Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 27 Apr 2021 10:33:19 +0200 Subject: [PATCH 1/4] add global replacements using inverted char classes as a sanitizer for DOM based XSS --- .../ql/src/semmle/javascript/security/dataflow/Xss.qll | 9 ++++++++- .../Security/CWE-079/XssThroughDom/xss-through-dom.js | 4 ++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll index 6a7bd111a9fe..899c1c17e879 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll @@ -34,7 +34,14 @@ module Shared { class MetacharEscapeSanitizer extends Sanitizer, StringReplaceCall { MetacharEscapeSanitizer() { isGlobal() and - RegExp::alwaysMatchesMetaCharacter(getRegExp().getRoot(), ["<", "'", "\""]) + ( + RegExp::alwaysMatchesMetaCharacter(getRegExp().getRoot(), ["<", "'", "\""]) + or + // or it's a global inverted char class. + getRegExp().getRoot().(RegExpCharacterClass).isInverted() + or + getRegExp().getRoot().(RegExpQuantifier).getAChild().(RegExpCharacterClass).isInverted() + ) } } diff --git a/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/xss-through-dom.js b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/xss-through-dom.js index 0e6555fe5685..656f233ca9e3 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/xss-through-dom.js +++ b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/xss-through-dom.js @@ -85,4 +85,8 @@ $("#id").html(anser.ansiToHtml(text)); // NOT OK $("#id").html(new anser().process(text)); // NOT OK + + $("section h1").each(function(){ + $("nav ul").append("Section"); // OK + }); })(); \ No newline at end of file From 160fa148f11d0f43f4d9ee5a19e32e93597b2e88 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Wed, 28 Apr 2021 11:39:28 +0200 Subject: [PATCH 2/4] move `InfiniteRepetitionQuantifier` to Regexp.qll --- javascript/ql/src/semmle/javascript/Regexp.qll | 13 +++++++++++++ .../javascript/security/performance/ReDoSUtil.qll | 13 ------------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/Regexp.qll b/javascript/ql/src/semmle/javascript/Regexp.qll index 842cf0705c72..0d600f8f8c46 100644 --- a/javascript/ql/src/semmle/javascript/Regexp.qll +++ b/javascript/ql/src/semmle/javascript/Regexp.qll @@ -191,6 +191,19 @@ class RegExpQuantifier extends RegExpTerm, @regexp_quantifier { predicate isGreedy() { is_greedy(this) } } +/** + * A regular expression term that permits unlimited repetitions. + */ +class InfiniteRepetitionQuantifier extends RegExpQuantifier { + InfiniteRepetitionQuantifier() { + this instanceof RegExpPlus + or + this instanceof RegExpStar + or + this instanceof RegExpRange and not exists(this.(RegExpRange).getUpperBound()) + } +} + /** * An escaped regular expression term, that is, a regular expression * term starting with a backslash. diff --git a/javascript/ql/src/semmle/javascript/security/performance/ReDoSUtil.qll b/javascript/ql/src/semmle/javascript/security/performance/ReDoSUtil.qll index ae2917920d7b..a6f91c595dd0 100644 --- a/javascript/ql/src/semmle/javascript/security/performance/ReDoSUtil.qll +++ b/javascript/ql/src/semmle/javascript/security/performance/ReDoSUtil.qll @@ -53,19 +53,6 @@ private predicate isReDoSCandidate(State state, string pump) { ) } -/** - * A regular expression term that permits unlimited repetitions. - */ -class InfiniteRepetitionQuantifier extends RegExpQuantifier { - InfiniteRepetitionQuantifier() { - this instanceof RegExpPlus - or - this instanceof RegExpStar - or - this instanceof RegExpRange and not exists(this.(RegExpRange).getUpperBound()) - } -} - /** * Gets the char after `c` (from a simplified ASCII table). */ From d07c71c99de5f318230bedca605d43afd799e8a2 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Wed, 28 Apr 2021 11:46:35 +0200 Subject: [PATCH 3/4] unlimited repetition of a wildcard is also a wildcard --- javascript/ql/src/semmle/javascript/Regexp.qll | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/Regexp.qll b/javascript/ql/src/semmle/javascript/Regexp.qll index 0d600f8f8c46..1a27f001d600 100644 --- a/javascript/ql/src/semmle/javascript/Regexp.qll +++ b/javascript/ql/src/semmle/javascript/Regexp.qll @@ -1078,6 +1078,12 @@ module RegExp { not cls.isInverted() and cls.getAChild().(RegExpCharacterClassEscape).getValue().isUppercase() ) + or + // an unlimited number of wildcards, is also a wildcard. + exists(InfiniteRepetitionQuantifier q | + term = q and + isWildcardLike(q.getAChild()) + ) } /** From d5450f1df6d82f7e3457e23fbc8c6b6da7467147 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Wed, 28 Apr 2021 11:46:50 +0200 Subject: [PATCH 4/4] use `isWildcardLike` in `MetacharEscapeSanitizer` --- .../ql/src/semmle/javascript/security/dataflow/Xss.qll | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll index 899c1c17e879..81e69a5229b6 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll @@ -37,10 +37,8 @@ module Shared { ( RegExp::alwaysMatchesMetaCharacter(getRegExp().getRoot(), ["<", "'", "\""]) or - // or it's a global inverted char class. - getRegExp().getRoot().(RegExpCharacterClass).isInverted() - or - getRegExp().getRoot().(RegExpQuantifier).getAChild().(RegExpCharacterClass).isInverted() + // or it's like a wild-card. + RegExp::isWildcardLike(getRegExp().getRoot()) ) } }