From 89613dbd23c2c9e4845e76a15766d4e120c1d517 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Wed, 22 Apr 2020 10:39:33 +0200 Subject: [PATCH] JS: add query for incomplete HTML attribute sanitization --- .../IncompleteHtmlAttributeSanitization.ql | 41 +++++ .../security/IncompleteBlacklistSanitizer.qll | 157 ++++++++++++++++++ .../IncompleteHtmlAttributeSanitization.qll | 58 +++++++ ...tmlAttributeSanitizationCustomizations.qll | 87 ++++++++++ .../IncompleteBlacklistSanitizer.expected | 29 ++++ .../IncompleteBlacklistSanitizer.ql | 7 + ...completeHtmlAttributeSanitization.expected | 45 +++++ .../IncompleteHtmlAttributeSanitization.qlref | 1 + .../CWE-116/IncompleteSanitization/tst.js | 94 +++++++++++ 9 files changed, 519 insertions(+) create mode 100644 javascript/ql/src/Security/CWE-116/IncompleteHtmlAttributeSanitization.ql create mode 100644 javascript/ql/src/semmle/javascript/security/IncompleteBlacklistSanitizer.qll create mode 100644 javascript/ql/src/semmle/javascript/security/dataflow/IncompleteHtmlAttributeSanitization.qll create mode 100644 javascript/ql/src/semmle/javascript/security/dataflow/IncompleteHtmlAttributeSanitizationCustomizations.qll create mode 100644 javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteBlacklistSanitizer.expected create mode 100644 javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteBlacklistSanitizer.ql create mode 100644 javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteHtmlAttributeSanitization.expected create mode 100644 javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteHtmlAttributeSanitization.qlref diff --git a/javascript/ql/src/Security/CWE-116/IncompleteHtmlAttributeSanitization.ql b/javascript/ql/src/Security/CWE-116/IncompleteHtmlAttributeSanitization.ql new file mode 100644 index 00000000000..41f7ddf30dd --- /dev/null +++ b/javascript/ql/src/Security/CWE-116/IncompleteHtmlAttributeSanitization.ql @@ -0,0 +1,41 @@ +/** + * @name Incomplete HTML attribute sanitization + * @description Writing incompletely sanitized values to HTML + * attribute strings can lead to a cross-site + * scripting vulnerability. + * @kind path-problem + * @problem.severity warning + * @precision high + * @id js/incomplete-html-attribute-sanitization + * @tags security + * external/cwe/cwe-079 + * external/cwe/cwe-116 + * external/cwe/cwe-20 + */ + +import javascript +import DataFlow::PathGraph +import semmle.javascript.security.dataflow.IncompleteHtmlAttributeSanitization::IncompleteHtmlAttributeSanitization +import semmle.javascript.security.IncompleteBlacklistSanitizer + +/** + * Gets a pretty string of the dangerous characters for `sink`. + */ +string prettyPrintDangerousCharaters(Sink sink) { + result = + strictconcat(string s | + s = describeCharacters(sink.getADangerousCharacter()) + | + s, ", " order by s + ).regexpReplaceAll(",(?=[^,]+$)", " or") +} + +from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink +where cfg.hasFlowPath(source, sink) +select sink.getNode(), source, sink, + // this message is slightly sub-optimal as we do not have an easy way + // to get the flow labels that reach the sink, so the message includes + // all of them in a disjunction + "Cross-site scripting vulnerability as the output of $@ may contain " + + prettyPrintDangerousCharaters(sink.getNode()) + " when it reaches this attribute definition.", + source.getNode(), "this final HTML sanitizer step" diff --git a/javascript/ql/src/semmle/javascript/security/IncompleteBlacklistSanitizer.qll b/javascript/ql/src/semmle/javascript/security/IncompleteBlacklistSanitizer.qll new file mode 100644 index 00000000000..fa0d25ffc5e --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/IncompleteBlacklistSanitizer.qll @@ -0,0 +1,157 @@ +/** + * Provides classes and predicates for working with incomplete blacklist sanitizers. + */ + +import javascript + +/** + * An incomplete black-list sanitizer. + */ +abstract class IncompleteBlacklistSanitizer extends DataFlow::Node { + /** + * Gets a relevant character that is not sanitized by this sanitizer. + */ + abstract string getAnUnsanitizedCharacter(); + + /** + * Gets the kind of sanitization this sanitizer performs. + */ + abstract string getKind(); +} + +/** + * Describes the characters represented by `rep`. + */ +string describeCharacters(string rep) { + rep = "\"" and result = "quotes" + or + rep = "&" and result = "ampersands" + or + rep = "<" and result = "less-thans" + or + rep = ">" and result = "greater-thans" +} + +/** + * A local sequence of calls to `String.prototype.replace`, + * represented by the last call. + */ +class StringReplaceCallSequence extends DataFlow::CallNode { + StringReplaceCallSequence() { + this instanceof StringReplaceCall and + not exists(getAStringReplaceMethodCall(this)) // terminal + } + + /** + * Gets a member of this sequence. + */ + StringReplaceCall getAMember() { this = getAStringReplaceMethodCall*(result) } + + /** Gets a string that is the replacement of this call. */ + string getAReplacementString() { + // this is more restrictive than `StringReplaceCall::replaces/2`, in the name of precision + getAMember().getRawReplacement().getStringValue() = result + } + + /** Gets a string that is being replaced by this call. */ + string getAReplacedString() { getAMember().getAReplacedString() = result } +} + +/** + * A specialized version of `DataFlow::Node::getAMethodCall` that is + * restricted to `StringReplaceCall`-nodes. + */ +private StringReplaceCall getAStringReplaceMethodCall(StringReplaceCall n) { + n.getAMethodCall() = result +} + +/** + * Provides predicates and classes for reasoning about HTML sanitization. + */ +module HtmlSanitization { + private predicate fixedGlobalReplacement(StringReplaceCallSequence chain) { + forall(StringReplaceCall member | member = chain.getAMember() | + member.isGlobal() and member.getArgument(0) instanceof DataFlow::RegExpLiteralNode + ) + } + + /** + * Gets a HTML-relevant character that is replaced by `chain`. + */ + private string getALikelyReplacedCharacter(StringReplaceCallSequence chain) { + result = "\"" and + ( + chain.getAReplacedString() = result or + chain.getAReplacementString() = """ or + chain.getAReplacementString() = """ + ) + or + result = "&" and + ( + chain.getAReplacedString() = result or + chain.getAReplacementString() = "&" or + chain.getAReplacementString() = "(" + ) + or + result = "<" and + ( + chain.getAReplacedString() = result or + chain.getAReplacementString() = "<" or + chain.getAReplacementString() = "<" + ) + or + result = ">" and + ( + chain.getAReplacedString() = result or + chain.getAReplacementString() = ">" or + chain.getAReplacementString() = ">" + ) + } + + /** + * An incomplete sanitizer for HTML-relevant characters. + */ + class IncompleteSanitizer extends IncompleteBlacklistSanitizer { + StringReplaceCallSequence chain; + string unsanitized; + + IncompleteSanitizer() { + this = chain and + fixedGlobalReplacement(chain) and + not getALikelyReplacedCharacter(chain) = unsanitized and + ( + // replaces `<` and `>` + getALikelyReplacedCharacter(chain) = "<" and + getALikelyReplacedCharacter(chain) = ">" and + ( + unsanitized = "\"" + or + unsanitized = "&" + ) + or + // replaces '&' and either `<` or `>` + getALikelyReplacedCharacter(chain) = "&" and + ( + getALikelyReplacedCharacter(chain) = ">" and + unsanitized = ">" + or + getALikelyReplacedCharacter(chain) = "<" and + unsanitized = "<" + ) + ) and + // does not replace special characters that the browser doesn't care for + not chain.getAReplacedString() = ["!", "#", "*", "?", "@", "|", "~"] and + /// only replaces explicit characters: exclude character ranges and negated character classes + not exists(RegExpTerm t | t = chain.getAMember().getRegExp().getRoot().getAChild*() | + t.(RegExpCharacterClass).isInverted() or + t instanceof RegExpCharacterRange + ) and + // the replacements are either empty or HTML entities + chain.getAReplacementString().regexpMatch("(?i)(|(&[#a-z0-9]+;))") + } + + override string getKind() { result = "HTML" } + + override string getAnUnsanitizedCharacter() { result = unsanitized } + } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/IncompleteHtmlAttributeSanitization.qll b/javascript/ql/src/semmle/javascript/security/dataflow/IncompleteHtmlAttributeSanitization.qll new file mode 100644 index 00000000000..5ca6f644506 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/IncompleteHtmlAttributeSanitization.qll @@ -0,0 +1,58 @@ +/** + * Provides a taint tracking configuration for reasoning about + * incomplete HTML sanitization vulnerabilities. + * + * Note, for performance reasons: only import this file if + * `IncompleteHtmlAttributeSanitization::Configuration` is needed, otherwise + * `IncompleteHtmlAttributeSanitizationCustomizations` should be imported instead. + */ + +import javascript + +module IncompleteHtmlAttributeSanitization { + import IncompleteHtmlAttributeSanitizationCustomizations::IncompleteHtmlAttributeSanitization + + private module Label { + class Quote extends DataFlow::FlowLabel { + Quote() { this = "\"" } + } + + class Ampersand extends DataFlow::FlowLabel { + Ampersand() { this = "&" } + } + + DataFlow::FlowLabel characterToLabel(string c) { c = result } + } + + /** + * A taint-tracking configuration for reasoning about incomplete HTML sanitization vulnerabilities. + */ + class Configuration extends TaintTracking::Configuration { + Configuration() { this = "IncompleteHtmlAttributeSanitization" } + + override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { + label = Label::characterToLabel(source.(Source).getAnUnsanitizedCharacter()) + } + + override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) { + label = Label::characterToLabel(sink.(Sink).getADangerousCharacter()) + } + + override predicate isAdditionalFlowStep( + DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel, + DataFlow::FlowLabel dstlabel + ) { + super.isAdditionalFlowStep(src, dst) and srclabel = dstlabel + } + + override predicate isLabeledBarrier(DataFlow::Node node, DataFlow::FlowLabel lbl) { + lbl = Label::characterToLabel(node.(StringReplaceCall).getAReplacedString()) or + isSanitizer(node) + } + + override predicate isSanitizer(DataFlow::Node n) { + n instanceof Sanitizer or + super.isSanitizer(n) + } + } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/IncompleteHtmlAttributeSanitizationCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/IncompleteHtmlAttributeSanitizationCustomizations.qll new file mode 100644 index 00000000000..ceb63d013f3 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/IncompleteHtmlAttributeSanitizationCustomizations.qll @@ -0,0 +1,87 @@ +/** + * Provides default sources, sinks and sanitizers for reasoning about + * incomplete HTML sanitization vulnerabilities, as well as extension + * points for adding your own. + */ + +import javascript +import semmle.javascript.security.dataflow.RemoteFlowSources +import semmle.javascript.security.IncompleteBlacklistSanitizer + +module IncompleteHtmlAttributeSanitization { + /** + * A data flow source for incomplete HTML sanitization vulnerabilities. + */ + abstract class Source extends DataFlow::Node { + /** + * Gets a character that may come out of this source. + */ + abstract string getAnUnsanitizedCharacter(); + } + + /** + * A data flow sink for incomplete HTML sanitization vulnerabilities. + */ + abstract class Sink extends DataFlow::Node { + /** + * Gets a character that is dangerous for this sink. + */ + abstract string getADangerousCharacter(); + } + + /** + * A sanitizer for incomplete HTML sanitization vulnerabilities. + */ + abstract class Sanitizer extends DataFlow::Node { } + + /** + * A source of incompletely sanitized characters, considered as a + * flow source for incomplete HTML sanitization vulnerabilities. + */ + class IncompleteHtmlSanitizerAsSource extends Source, HtmlSanitization::IncompleteSanitizer { + override string getAnUnsanitizedCharacter() { + result = HtmlSanitization::IncompleteSanitizer.super.getAnUnsanitizedCharacter() + } + } + + /** + * A concatenation that syntactically looks like a definition of an HTML attribute. + */ + class HtmlAttributeConcatenation extends StringOps::ConcatenationLeaf { + string lhs; + + HtmlAttributeConcatenation() { + lhs = this.getPreviousLeaf().getStringValue().regexpCapture("(.*)=\"[^\"]*", 1) and + this.getNextLeaf().getStringValue().regexpMatch(".*\".*") + } + + /** + * Holds if the attribute value is interpreted as JavaScript source code. + */ + predicate isInterpretedAsJavaScript() { lhs.regexpMatch("(?i)(.* )?on[a-z]+") } + } + + /** + * A concatenation that syntactically looks like a definition of an + * HTML attribute, as a sink for incomplete HTML sanitization + * vulnerabilities. + */ + class HtmlAttributeConcatenationAsSink extends Sink, DataFlow::ValueNode, + HtmlAttributeConcatenation { + override string getADangerousCharacter() { + isInterpretedAsJavaScript() and result = "&" + or + result = "\"" + } + } + + /** + * An encoder for potentially malicious characters, as a sanitizer + * for incomplete HTML sanitization vulnerabilities. + */ + class EncodingSanitizer extends Sanitizer { + EncodingSanitizer() { + this = DataFlow::globalVarRef(["encodeURIComponent", "encodeURI"]).getACall() + } + } +} diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteBlacklistSanitizer.expected b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteBlacklistSanitizer.expected new file mode 100644 index 00000000000..7808ccba13c --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteBlacklistSanitizer.expected @@ -0,0 +1,29 @@ +| tst.js:206:2:206:24 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize ampersands | +| tst.js:206:2:206:24 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes | +| tst.js:207:2:207:26 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes | +| tst.js:208:2:208:26 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands | +| tst.js:209:2:209:40 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands | +| tst.js:209:2:209:40 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes | +| tst.js:210:2:210:58 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes | +| tst.js:211:2:211:58 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes | +| tst.js:212:2:212:58 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes | +| tst.js:215:6:215:24 | s.replace(/>/g, '') | This HTML sanitizer does not sanitize ampersands | +| tst.js:215:6:215:24 | s.replace(/>/g, '') | This HTML sanitizer does not sanitize quotes | +| tst.js:217:2:217:93 | s().rep ... '') | This HTML sanitizer does not sanitize quotes | +| tst.js:243:9:243:31 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize ampersands | +| tst.js:243:9:243:31 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes | +| tst.js:244:9:244:33 | s().rep ... /g, '') | This HTML sanitizer does not sanitize quotes | +| tst.js:245:9:245:33 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands | +| tst.js:249:9:249:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes | +| tst.js:251:9:251:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes | +| tst.js:253:21:253:45 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands | +| tst.js:254:32:254:56 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands | +| tst.js:255:26:255:50 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands | +| tst.js:256:15:256:39 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands | +| tst.js:261:10:261:81 | value.r ... '>') | This HTML sanitizer does not sanitize quotes | +| tst.js:270:61:270:85 | s().rep ... /g, '') | This HTML sanitizer does not sanitize ampersands | +| tst.js:272:28:272:50 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize ampersands | +| tst.js:272:28:272:50 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize quotes | +| tst.js:274:12:274:94 | s().val ... g , '') | This HTML sanitizer does not sanitize ampersands | +| tst.js:274:12:274:94 | s().val ... g , '') | This HTML sanitizer does not sanitize quotes | +| tst.js:277:9:277:29 | arr2.re ... "/g,"") | This HTML sanitizer does not sanitize ampersands | diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteBlacklistSanitizer.ql b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteBlacklistSanitizer.ql new file mode 100644 index 00000000000..3c301c17c5c --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteBlacklistSanitizer.ql @@ -0,0 +1,7 @@ +import javascript +import semmle.javascript.security.IncompleteBlacklistSanitizer + +from IncompleteBlacklistSanitizer sanitizer +select sanitizer, + "This " + sanitizer.getKind() + " sanitizer does not sanitize " + + describeCharacters(sanitizer.getAnUnsanitizedCharacter()) diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteHtmlAttributeSanitization.expected b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteHtmlAttributeSanitization.expected new file mode 100644 index 00000000000..aa9aca72221 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteHtmlAttributeSanitization.expected @@ -0,0 +1,45 @@ +nodes +| tst.js:243:9:243:31 | s().rep ... ]/g,'') | +| tst.js:243:9:243:31 | s().rep ... ]/g,'') | +| tst.js:243:9:243:31 | s().rep ... ]/g,'') | +| tst.js:244:9:244:33 | s().rep ... /g, '') | +| tst.js:244:9:244:33 | s().rep ... /g, '') | +| tst.js:244:9:244:33 | s().rep ... /g, '') | +| tst.js:249:9:249:33 | s().rep ... ]/g,'') | +| tst.js:249:9:249:33 | s().rep ... ]/g,'') | +| tst.js:249:9:249:33 | s().rep ... ]/g,'') | +| tst.js:253:21:253:45 | s().rep ... /g, '') | +| tst.js:253:21:253:45 | s().rep ... /g, '') | +| tst.js:253:21:253:45 | s().rep ... /g, '') | +| tst.js:254:32:254:56 | s().rep ... /g, '') | +| tst.js:254:32:254:56 | s().rep ... /g, '') | +| tst.js:254:32:254:56 | s().rep ... /g, '') | +| tst.js:270:61:270:85 | s().rep ... /g, '') | +| tst.js:270:61:270:85 | s().rep ... /g, '') | +| tst.js:270:61:270:85 | s().rep ... /g, '') | +| tst.js:274:6:274:94 | arr | +| tst.js:274:12:274:94 | s().val ... g , '') | +| tst.js:274:12:274:94 | s().val ... g , '') | +| tst.js:275:9:275:11 | arr | +| tst.js:275:9:275:21 | arr.join(" ") | +| tst.js:275:9:275:21 | arr.join(" ") | +edges +| tst.js:243:9:243:31 | s().rep ... ]/g,'') | tst.js:243:9:243:31 | s().rep ... ]/g,'') | +| tst.js:244:9:244:33 | s().rep ... /g, '') | tst.js:244:9:244:33 | s().rep ... /g, '') | +| tst.js:249:9:249:33 | s().rep ... ]/g,'') | tst.js:249:9:249:33 | s().rep ... ]/g,'') | +| tst.js:253:21:253:45 | s().rep ... /g, '') | tst.js:253:21:253:45 | s().rep ... /g, '') | +| tst.js:254:32:254:56 | s().rep ... /g, '') | tst.js:254:32:254:56 | s().rep ... /g, '') | +| tst.js:270:61:270:85 | s().rep ... /g, '') | tst.js:270:61:270:85 | s().rep ... /g, '') | +| tst.js:274:6:274:94 | arr | tst.js:275:9:275:11 | arr | +| tst.js:274:12:274:94 | s().val ... g , '') | tst.js:274:6:274:94 | arr | +| tst.js:274:12:274:94 | s().val ... g , '') | tst.js:274:6:274:94 | arr | +| tst.js:275:9:275:11 | arr | tst.js:275:9:275:21 | arr.join(" ") | +| tst.js:275:9:275:11 | arr | tst.js:275:9:275:21 | arr.join(" ") | +#select +| tst.js:243:9:243:31 | s().rep ... ]/g,'') | tst.js:243:9:243:31 | s().rep ... ]/g,'') | tst.js:243:9:243:31 | s().rep ... ]/g,'') | Cross-site scripting vulnerability as the output of $@ may contain quotes when it reaches this attribute definition. | tst.js:243:9:243:31 | s().rep ... ]/g,'') | this final HTML sanitizer step | +| tst.js:244:9:244:33 | s().rep ... /g, '') | tst.js:244:9:244:33 | s().rep ... /g, '') | tst.js:244:9:244:33 | s().rep ... /g, '') | Cross-site scripting vulnerability as the output of $@ may contain quotes when it reaches this attribute definition. | tst.js:244:9:244:33 | s().rep ... /g, '') | this final HTML sanitizer step | +| tst.js:249:9:249:33 | s().rep ... ]/g,'') | tst.js:249:9:249:33 | s().rep ... ]/g,'') | tst.js:249:9:249:33 | s().rep ... ]/g,'') | Cross-site scripting vulnerability as the output of $@ may contain quotes when it reaches this attribute definition. | tst.js:249:9:249:33 | s().rep ... ]/g,'') | this final HTML sanitizer step | +| tst.js:253:21:253:45 | s().rep ... /g, '') | tst.js:253:21:253:45 | s().rep ... /g, '') | tst.js:253:21:253:45 | s().rep ... /g, '') | Cross-site scripting vulnerability as the output of $@ may contain ampersands or quotes when it reaches this attribute definition. | tst.js:253:21:253:45 | s().rep ... /g, '') | this final HTML sanitizer step | +| tst.js:254:32:254:56 | s().rep ... /g, '') | tst.js:254:32:254:56 | s().rep ... /g, '') | tst.js:254:32:254:56 | s().rep ... /g, '') | Cross-site scripting vulnerability as the output of $@ may contain ampersands or quotes when it reaches this attribute definition. | tst.js:254:32:254:56 | s().rep ... /g, '') | this final HTML sanitizer step | +| tst.js:270:61:270:85 | s().rep ... /g, '') | tst.js:270:61:270:85 | s().rep ... /g, '') | tst.js:270:61:270:85 | s().rep ... /g, '') | Cross-site scripting vulnerability as the output of $@ may contain ampersands or quotes when it reaches this attribute definition. | tst.js:270:61:270:85 | s().rep ... /g, '') | this final HTML sanitizer step | +| tst.js:275:9:275:21 | arr.join(" ") | tst.js:274:12:274:94 | s().val ... g , '') | tst.js:275:9:275:21 | arr.join(" ") | Cross-site scripting vulnerability as the output of $@ may contain quotes when it reaches this attribute definition. | tst.js:274:12:274:94 | s().val ... g , '') | this final HTML sanitizer step | diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteHtmlAttributeSanitization.qlref b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteHtmlAttributeSanitization.qlref new file mode 100644 index 00000000000..d6963163eef --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/IncompleteHtmlAttributeSanitization.qlref @@ -0,0 +1 @@ +Security/CWE-116/IncompleteHtmlAttributeSanitization.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js index 067e80cc593..e5e784c7a87 100644 --- a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/tst.js @@ -201,3 +201,97 @@ app.get('/some/path', function(req, res) { function bad18(p) { return p.replace("/../", ""); // NOT OK } + +function typicalBadHtmlSanitizers(s) { + s().replace(/[<>]/g,''); // NOT OK + s().replace(/[<>&]/g, ''); // NOT OK + s().replace(/[<>"]/g, ''); // NOT OK + s().replace(//g, ''); // NOT OK + s().replace(//g, '').replace(/&/g, ''); // NOT OK + s().replace(//g, ''); // NOT OK + s().replace(/&/g, '').replace(/>/g, '').replace(//g, ''); // NOT OK + s().replace(//g, '>').replace(/&/g, '&').replace(/"/g, '"'); // OK + s().replace(//g, '>').replace(/&/g, '&').replace(/'/g, '''); // NOT OK + + s().replace(//g, '>').replace(RE, function(match) {/* ... */ }); // OK (probably) + + s().replace(/[<>'"&]/g,''); // OK + + s().replace(//g, '>').replace(/"/g, '"').replace(/&(?![\w\#]+;)/g, '&'); // OK + + s().replace(!encode ? /&(?!#?\w+;)/g : /&/g, '&').replace(//g, '>').replace(/"/g, '"').replace(/'/g, '''); // OK (and not recognized due to the ternary) + + s().replace(/[\\/:\*\?"<>\|]/g, ''); // OK + + s().replace(/[<>]/g,'_'); // OK (but probably unsafe) + + s().replace(/&/g, ">"); // OK + + s().replace(/[\#\%4\/\-\~\>8\_\@0\!\&3\[651d\=e7fA2D\(aFBb]/g, ""); // OK + + s().replace(/[<>]/g,'').replace(/[^a-z]/g, ''); // OK + + s().replace(/[<>]/g,'').replace(/[^abc]/g, ''); // OK + + s().replace(/[<>]/g,'').replace(/[ -~]/g, ''); // OK +} + +function incompleteHtmlAttributeSanitization() { + '="' + s().replace(/[<>]/g,'') + '"'; // NOT OK + '="' + s().replace(/[<>&]/g, '') + '"'; // NOT OK + '="' + s().replace(/[<>"]/g, '') + '"'; // OK (maybe, since the attribute name is unknown) + '="' + s().replace(/[<>&"]/g,'') + '"'; // OK + '="' + s().replace(/[&"]/g,'') + '"'; // OK + + '="' + s().replace(/[<>&']/g,'') + '"'; // NOT OK + "='" + s().replace(/[<>&"]/g,'') + "'"; // OK (but given the context, it is probably not fine) + "='" + s().replace(/[<>&']/g,'') + "'"; // NOT OK (but given the context, it is probably fine) + + 'onFunkyEvent="' + s().replace(/[<>"]/g, '') + '"'; // NOT OK + '
/g, '>'); + } + function attr_str(a) { + return ' ' + a.name + '="' + escapeHTML(a.value).replace(/"/g, '"') + '"'; // OK + } + result += '<' + tag(node) + [].map.call(x, attr_str).join('') + '>'; +} + +function moreIncompleteHtmlAttributeSanitization() { + ''; // NOT OK + '="' + s().replace(/[<>]/g,'').replace(/[^\w ]+/g, '') + '"'; // OK + '="' + encodeURIComponent(s().replace(/[<>]/g,'')) + '"'; // OK + + var arr = s().val().trim().replace(/^,|,$/g , '').replace(/^;|;$/g , '').replace(/<|>/g , ''); + '="' + arr.join(" ") + '"'; // NOT OK + var arr2 = s().val().trim().replace(/^,|,$/g , '').replace(/^;|;$/g , '').replace(/<|>/g , '') + arr2 = arr2.replace(/"/g,""); + '="' + arr2.join(" ") + '"'; // OK + + var x; + x = x.replace(/&/g, '&'); + x = x.replace(//g, '>'); + 'onclick="' + x + '"'; // NOT OK - but not flagged since the `x` replace chain is extended below + x = x.replace(/"/g, '"'); + 'onclick="' + x + '"'; // OK + + var y; + if (escapeAmpersand) { + y = y.replace(/&/g, '&'); + } + y = y.replace(//g, '>'); + 'onclick="' + y + '"'; // NOT OK - but not flagged since the `x` replace chain is extended below + if (escapeQuotes) { + y = y.replace(/"/g, '"'); + } + 'onclick="' + y + '"'; // OK +}