Skip to content

JS: More precise type-test sanitizer guards in unsafe-html-construction #12177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 27, 2023
Merged

JS: More precise type-test sanitizer guards in unsafe-html-construction #12177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
09794fa
delete PrefixStringSanitizer
erik-krogh Feb 15, 2023
51ddb55
use tainted-object to precisely model that plain object are fine, but…
erik-krogh Feb 15, 2023
0e60fc5
Merge branch 'main' into alias-html
erik-krogh Feb 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions ...cript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}
}
36 changes: 22 additions & 14 deletions javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,23 @@ 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.
*/
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)
Expand All @@ -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
Expand All @@ -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 }
}
Expand Down
94 changes: 87 additions & 7 deletions .../test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -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> ... /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> ... /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> ... /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> ... /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 |
Expand Down
23 changes: 20 additions & 3 deletions javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/lib2/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,21 @@
export function trivialXss(s: string) {
const html = "<span>" + s + "</span>"; // NOT OK - this file is recognized as a main file.
document.querySelector("#html").innerHTML = html;
}
const html = "<span>" + s + "</span>"; // 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 = "<span>" + settings + "</span>"; // 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 = "<span>" + name + "</span>"; // NOT OK
}
}
}