diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll index 6ffbf88f9621..5c309b07727d 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll @@ -182,16 +182,17 @@ module UnsafeHtmlConstruction { } /** A test for the value of `typeof x`, restricting the potential types of `x`. */ - class TypeTestGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode { + class TypeTestGuard extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::ValueNode { override EqualityTest astNode; Expr operand; boolean polarity; TypeTestGuard() { TaintTracking::isStringTypeGuard(astNode, operand, polarity) } - override predicate sanitizes(boolean outcome, Expr e) { + override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel lbl) { polarity = outcome and - e = operand + e = operand and + lbl.isTaint() } } } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll index 8e5060036af7..1b50dab5bc60 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll @@ -7,6 +7,7 @@ import javascript private import semmle.javascript.security.dataflow.DomBasedXssCustomizations::DomBasedXss as DomBasedXss private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin as UnsafeJQueryPlugin import UnsafeHtmlConstructionCustomizations::UnsafeHtmlConstruction +import semmle.javascript.security.TaintedObject /** * A taint-tracking configuration for reasoning about unsafe HTML constructed from library input vulnerabilities. @@ -14,9 +15,15 @@ import UnsafeHtmlConstructionCustomizations::UnsafeHtmlConstruction class Configration extends TaintTracking::Configuration { Configration() { this = "UnsafeHtmlConstruction" } - override predicate isSource(DataFlow::Node source) { source instanceof Source } + override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { + source instanceof Source and + label = [TaintedObject::label(), DataFlow::FlowLabel::taint(), DataFlow::FlowLabel::data()] + } - override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) { + sink instanceof Sink and + label = DataFlow::FlowLabel::taint() + } override predicate isSanitizer(DataFlow::Node node) { super.isSanitizer(node) @@ -36,12 +43,22 @@ class Configration extends TaintTracking::Configuration { DataFlow::hasPathWithoutUnmatchedReturn(source, sink) } - override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { - DataFlow::localFieldStep(pred, succ) + override predicate isAdditionalFlowStep( + DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl + ) { + DataFlow::localFieldStep(pred, succ) and + inlbl.isTaint() and + outlbl.isTaint() + or + TaintedObject::step(pred, succ, inlbl, outlbl) + or + // property read from a tainted object is considered tainted + succ.(DataFlow::PropRead).getBase() = pred and + inlbl = TaintedObject::label() and + outlbl = DataFlow::FlowLabel::taint() } override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) { - guard instanceof PrefixStringSanitizer or guard instanceof QuoteGuard or guard instanceof ContainsHtmlGuard or guard instanceof TypeTestGuard @@ -50,15 +67,6 @@ class Configration extends TaintTracking::Configuration { private import semmle.javascript.security.dataflow.Xss::Shared as Shared -private class PrefixStringSanitizer extends TaintTracking::SanitizerGuardNode, - DomBasedXss::PrefixStringSanitizer { - PrefixStringSanitizer() { this = this } -} - -private class PrefixString extends DataFlow::FlowLabel, DomBasedXss::PrefixString { - PrefixString() { this = this } -} - private class QuoteGuard extends TaintTracking::SanitizerGuardNode, Shared::QuoteGuard { QuoteGuard() { this = this } } diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected index 726fb8964351..b05425e65da6 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected @@ -3,15 +3,33 @@ nodes | jquery-plugin.js:11:27:11:31 | stuff | | jquery-plugin.js:11:34:11:40 | options | | jquery-plugin.js:11:34:11:40 | options | +| jquery-plugin.js:11:34:11:40 | options | +| jquery-plugin.js:11:34:11:40 | options | +| jquery-plugin.js:12:31:12:37 | options | +| jquery-plugin.js:12:31:12:37 | options | | jquery-plugin.js:12:31:12:37 | options | | jquery-plugin.js:12:31:12:41 | options.foo | | jquery-plugin.js:12:31:12:41 | options.foo | +| jquery-plugin.js:12:31:12:41 | options.foo | +| jquery-plugin.js:12:31:12:41 | options.foo | | jquery-plugin.js:14:31:14:35 | stuff | | jquery-plugin.js:14:31:14:35 | stuff | | lib2/index.ts:1:28:1:28 | s | | lib2/index.ts:1:28:1:28 | s | -| lib2/index.ts:2:29:2:29 | s | -| lib2/index.ts:2:29:2:29 | s | +| lib2/index.ts:2:27:2:27 | s | +| lib2/index.ts:2:27:2:27 | s | +| lib2/index.ts:6:29:6:36 | settings | +| lib2/index.ts:6:29:6:36 | settings | +| lib2/index.ts:6:29:6:36 | settings | +| lib2/index.ts:7:58:7:65 | settings | +| lib2/index.ts:7:58:7:65 | settings | +| lib2/index.ts:13:9:13:41 | name | +| lib2/index.ts:13:16:13:23 | settings | +| lib2/index.ts:13:16:13:33 | settings.mySetting | +| lib2/index.ts:13:16:13:36 | setting ... ting[i] | +| lib2/index.ts:13:16:13:41 | setting ... i].name | +| lib2/index.ts:18:62:18:65 | name | +| lib2/index.ts:18:62:18:65 | name | | lib2/src/MyNode.ts:1:28:1:28 | s | | lib2/src/MyNode.ts:1:28:1:28 | s | | lib2/src/MyNode.ts:2:29:2:29 | s | @@ -45,13 +63,31 @@ nodes | main.js:53:20:53:20 | s | | main.js:56:28:56:34 | options | | main.js:56:28:56:34 | options | +| main.js:56:28:56:34 | options | +| main.js:56:28:56:34 | options | +| main.js:57:11:59:5 | defaults | | main.js:57:11:59:5 | defaults | +| main.js:57:11:59:5 | defaults | +| main.js:57:22:59:5 | {\\n ... "\\n } | | main.js:57:22:59:5 | {\\n ... "\\n } | +| main.js:57:22:59:5 | {\\n ... "\\n } | +| main.js:60:11:60:48 | settings | +| main.js:60:11:60:48 | settings | | main.js:60:11:60:48 | settings | | main.js:60:22:60:48 | $.exten ... ptions) | +| main.js:60:22:60:48 | $.exten ... ptions) | +| main.js:60:22:60:48 | $.exten ... ptions) | +| main.js:60:31:60:38 | defaults | +| main.js:60:31:60:38 | defaults | | main.js:60:31:60:38 | defaults | | main.js:60:41:60:47 | options | +| main.js:60:41:60:47 | options | +| main.js:60:41:60:47 | options | +| main.js:62:19:62:26 | settings | | main.js:62:19:62:26 | settings | +| main.js:62:19:62:26 | settings | +| main.js:62:19:62:31 | settings.name | +| main.js:62:19:62:31 | settings.name | | main.js:62:19:62:31 | settings.name | | main.js:62:19:62:31 | settings.name | | main.js:66:35:66:41 | attrVal | @@ -106,12 +142,32 @@ edges | jquery-plugin.js:11:27:11:31 | stuff | jquery-plugin.js:14:31:14:35 | stuff | | jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options | | jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options | +| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options | +| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options | +| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options | +| jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:37 | options | +| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo | +| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo | +| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo | | jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo | | jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo | -| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s | -| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s | -| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s | -| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s | +| jquery-plugin.js:12:31:12:37 | options | jquery-plugin.js:12:31:12:41 | options.foo | +| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s | +| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s | +| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s | +| lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s | +| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings | +| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings | +| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings | +| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings | +| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:13:16:13:23 | settings | +| lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:13:16:13:23 | settings | +| lib2/index.ts:13:9:13:41 | name | lib2/index.ts:18:62:18:65 | name | +| lib2/index.ts:13:9:13:41 | name | lib2/index.ts:18:62:18:65 | name | +| lib2/index.ts:13:16:13:23 | settings | lib2/index.ts:13:16:13:33 | settings.mySetting | +| lib2/index.ts:13:16:13:33 | settings.mySetting | lib2/index.ts:13:16:13:36 | setting ... ting[i] | +| lib2/index.ts:13:16:13:36 | setting ... ting[i] | lib2/index.ts:13:16:13:41 | setting ... i].name | +| lib2/index.ts:13:16:13:41 | setting ... i].name | lib2/index.ts:13:9:13:41 | name | | lib2/src/MyNode.ts:1:28:1:28 | s | lib2/src/MyNode.ts:2:29:2:29 | s | | lib2/src/MyNode.ts:1:28:1:28 | s | lib2/src/MyNode.ts:2:29:2:29 | s | | lib2/src/MyNode.ts:1:28:1:28 | s | lib2/src/MyNode.ts:2:29:2:29 | s | @@ -144,13 +200,35 @@ edges | main.js:53:20:53:20 | s | main.js:41:17:41:17 | s | | main.js:56:28:56:34 | options | main.js:60:41:60:47 | options | | main.js:56:28:56:34 | options | main.js:60:41:60:47 | options | +| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options | +| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options | +| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options | +| main.js:56:28:56:34 | options | main.js:60:41:60:47 | options | +| main.js:57:11:59:5 | defaults | main.js:60:31:60:38 | defaults | +| main.js:57:11:59:5 | defaults | main.js:60:31:60:38 | defaults | | main.js:57:11:59:5 | defaults | main.js:60:31:60:38 | defaults | | main.js:57:22:59:5 | {\\n ... "\\n } | main.js:57:11:59:5 | defaults | +| main.js:57:22:59:5 | {\\n ... "\\n } | main.js:57:11:59:5 | defaults | +| main.js:57:22:59:5 | {\\n ... "\\n } | main.js:57:11:59:5 | defaults | +| main.js:60:11:60:48 | settings | main.js:62:19:62:26 | settings | | main.js:60:11:60:48 | settings | main.js:62:19:62:26 | settings | +| main.js:60:11:60:48 | settings | main.js:62:19:62:26 | settings | +| main.js:60:22:60:48 | $.exten ... ptions) | main.js:60:11:60:48 | settings | | main.js:60:22:60:48 | $.exten ... ptions) | main.js:60:11:60:48 | settings | +| main.js:60:22:60:48 | $.exten ... ptions) | main.js:60:11:60:48 | settings | +| main.js:60:31:60:38 | defaults | main.js:60:22:60:48 | $.exten ... ptions) | | main.js:60:31:60:38 | defaults | main.js:60:22:60:48 | $.exten ... ptions) | +| main.js:60:31:60:38 | defaults | main.js:60:22:60:48 | $.exten ... ptions) | +| main.js:60:41:60:47 | options | main.js:57:22:59:5 | {\\n ... "\\n } | +| main.js:60:41:60:47 | options | main.js:57:22:59:5 | {\\n ... "\\n } | | main.js:60:41:60:47 | options | main.js:57:22:59:5 | {\\n ... "\\n } | | main.js:60:41:60:47 | options | main.js:60:22:60:48 | $.exten ... ptions) | +| main.js:60:41:60:47 | options | main.js:60:22:60:48 | $.exten ... ptions) | +| main.js:60:41:60:47 | options | main.js:60:22:60:48 | $.exten ... ptions) | +| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name | +| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name | +| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name | +| main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name | | main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name | | main.js:62:19:62:26 | settings | main.js:62:19:62:31 | settings.name | | main.js:66:35:66:41 | attrVal | main.js:67:63:67:69 | attrVal | @@ -207,7 +285,9 @@ edges #select | jquery-plugin.js:12:31:12:41 | options.foo | jquery-plugin.js:11:34:11:40 | options | jquery-plugin.js:12:31:12:41 | options.foo | This HTML construction which depends on $@ might later allow $@. | jquery-plugin.js:11:34:11:40 | options | library input | jquery-plugin.js:12:20:12:53 | " ... /span>" | cross-site scripting | | jquery-plugin.js:14:31:14:35 | stuff | jquery-plugin.js:11:27:11:31 | stuff | jquery-plugin.js:14:31:14:35 | stuff | This HTML construction which depends on $@ might later allow $@. | jquery-plugin.js:11:27:11:31 | stuff | library input | jquery-plugin.js:14:20:14:47 | " ... /span>" | cross-site scripting | -| lib2/index.ts:2:29:2:29 | s | lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:29:2:29 | s | This HTML construction which depends on $@ might later allow $@. | lib2/index.ts:1:28:1:28 | s | library input | lib2/index.ts:3:49:3:52 | html | cross-site scripting | +| lib2/index.ts:2:27:2:27 | s | lib2/index.ts:1:28:1:28 | s | lib2/index.ts:2:27:2:27 | s | This HTML construction which depends on $@ might later allow $@. | lib2/index.ts:1:28:1:28 | s | library input | lib2/index.ts:3:47:3:50 | html | cross-site scripting | +| lib2/index.ts:7:58:7:65 | settings | lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:7:58:7:65 | settings | This HTML construction which depends on $@ might later allow $@. | lib2/index.ts:6:29:6:36 | settings | library input | lib2/index.ts:7:47:7:77 | " ... /span>" | cross-site scripting | +| lib2/index.ts:18:62:18:65 | name | lib2/index.ts:6:29:6:36 | settings | lib2/index.ts:18:62:18:65 | name | This HTML construction which depends on $@ might later allow $@. | lib2/index.ts:6:29:6:36 | settings | library input | lib2/index.ts:18:51:18:77 | " ... /span>" | cross-site scripting | | lib2/src/MyNode.ts:2:29:2:29 | s | lib2/src/MyNode.ts:1:28:1:28 | s | lib2/src/MyNode.ts:2:29:2:29 | s | This HTML construction which depends on $@ might later allow $@. | lib2/src/MyNode.ts:1:28:1:28 | s | library input | lib2/src/MyNode.ts:3:49:3:52 | html | cross-site scripting | | lib/src/MyNode.ts:2:29:2:29 | s | lib/src/MyNode.ts:1:28:1:28 | s | lib/src/MyNode.ts:2:29:2:29 | s | This HTML construction which depends on $@ might later allow $@. | lib/src/MyNode.ts:1:28:1:28 | s | library input | lib/src/MyNode.ts:3:49:3:52 | html | cross-site scripting | | main.js:2:29:2:29 | s | main.js:1:55:1:55 | s | main.js:2:29:2:29 | s | This HTML construction which depends on $@ might later allow $@. | main.js:1:55:1:55 | s | library input | main.js:3:49:3:52 | html | cross-site scripting | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/lib2/index.ts b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/lib2/index.ts index 21bb420b71f4..4e5e4730547e 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/lib2/index.ts +++ b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/lib2/index.ts @@ -1,4 +1,21 @@ export function trivialXss(s: string) { - const html = "" + s + ""; // NOT OK - this file is recognized as a main file. - document.querySelector("#html").innerHTML = html; -} \ No newline at end of file + const html = "" + s + ""; // NOT OK - this file is recognized as a main file. + document.querySelector("#html").innerHTML = html; +} + +export function objectStuff(settings: any, i: number) { + document.querySelector("#html").innerHTML = "" + settings + ""; // NOT OK + var name; + + if (settings.mySetting && settings.mySetting.length !== 0) { + for (i = 0; i < settings.mySetting.length; ++i) { + if (typeof settings.mySetting[i] === "object") { + name = settings.mySetting[i].name; // `settings.mySetting[i]` is correctly sanitized, as it is an object. However, the `name` property is stil tainted. + } else { + name = ""; + } + + document.querySelector("#html").innerHTML = "" + name + ""; // NOT OK + } + } +}