diff --git a/javascript/frameworks/xsjs/ext/xsjs.model.yml b/javascript/frameworks/xsjs/ext/xsjs.model.yml index 8fc74518..cd7175b3 100644 --- a/javascript/frameworks/xsjs/ext/xsjs.model.yml +++ b/javascript/frameworks/xsjs/ext/xsjs.model.yml @@ -60,6 +60,7 @@ extensions: extensible: sinkModel data: - [WebResponse, "Member[setBody].Argument[0]", html-injection] + - [XsjsDollar, "Member[import].Argument[0..1]", path-injection] # - [Mail, "Member[send].Argument[this]", "???"] # - [SMTPConnection, "Member[send].Argument[0]", "???"] # - ["HTTPClient", "Member[request].Argument[0]", "???"] @@ -68,4 +69,5 @@ extensions: pack: codeql/javascript-all extensible: summaryModel data: - - [global, "Member[JSON].Member[parse]", "Argument[0]", "ReturnValue", taint] \ No newline at end of file + - [global, "Member[JSON].Member[parse]", "Argument[0]", "ReturnValue", taint] + - ["@sap/xss-secure", "Member[encodeCSS,encodeHTML,encodeJS,encodeURL,encodeXML]", "Argument[0]", "ReturnValue", "taint"] diff --git a/javascript/frameworks/xsjs/lib/advanced_security/javascript/frameworks/xsjs/AsyncXSJS.qll b/javascript/frameworks/xsjs/lib/advanced_security/javascript/frameworks/xsjs/AsyncXSJS.qll index 4d62fb40..7ec9b084 100644 --- a/javascript/frameworks/xsjs/lib/advanced_security/javascript/frameworks/xsjs/AsyncXSJS.qll +++ b/javascript/frameworks/xsjs/lib/advanced_security/javascript/frameworks/xsjs/AsyncXSJS.qll @@ -1,23 +1,24 @@ import javascript import DataFlow +import XSJSLibModules /** * The root XSJS namespace, accessed as a dollar sign (`$`) symbol. */ -class XSJSDollarNamespace extends GlobalVarRefNode { - XSJSDollarNamespace() { - this = globalVarRef("$") and - this.getFile().getExtension() = "xsjs" +class XSJSDollarNode extends DataFlow::SourceNode { + XSJSDollarNode() { + this.accessesGlobal("$") and + this.getFile().getExtension() = ["xsjs", "xsjslib"] } } /** - * `TypeModel` for `XSJSDollarNamespace`. + * `TypeModel` for `XSJSDollarNode`. */ class XSJSDollarTypeModel extends ModelInput::TypeModel { override DataFlow::Node getASource(string type) { type = "XsjsDollar" and - result = any(XSJSDollarNamespace dollar) + result = any(XSJSDollarNode dollar) } /** @@ -37,9 +38,9 @@ class XSJSRequestOrResponse extends SourceNode instanceof PropRef { XSJSRequestOrResponse() { fieldName = ["request", "response"] and ( - exists(XSJSDollarNamespace dollar | this = dollar.getAPropertyReference(fieldName)) + exists(XSJSDollarNode dollar | this = dollar.getAPropertyReference(fieldName)) or - exists(XSJSDollarNamespace dollar | + exists(XSJSDollarNode dollar | this = dollar .getAPropertyReference(fieldName) @@ -171,7 +172,7 @@ class XSJSDatabaseConnectionReference extends MethodCallNode { string subNamespace; XSJSDatabaseConnectionReference() { - exists(XSJSDollarNamespace dollar | + exists(XSJSDollarNode dollar | this.getMethodName() = "getConnection" and this.getReceiver().getALocalSource() = dollar.getAPropertyReference(subNamespace) ) @@ -211,7 +212,7 @@ class XSJSHDBConnectionReference extends XSJSDatabaseConnectionReference { class XSJSUtilNamespace extends SourceNode instanceof PropRef { XSJSUtilNamespace() { - exists(XSJSDollarNamespace dollar | this = dollar.getAPropertyReference("util")) + exists(XSJSDollarNode dollar | this = dollar.getAPropertyReference("util")) } } diff --git a/javascript/frameworks/xsjs/lib/advanced_security/javascript/frameworks/xsjs/XSJSLibModules.qll b/javascript/frameworks/xsjs/lib/advanced_security/javascript/frameworks/xsjs/XSJSLibModules.qll new file mode 100644 index 00000000..8ead29fd --- /dev/null +++ b/javascript/frameworks/xsjs/lib/advanced_security/javascript/frameworks/xsjs/XSJSLibModules.qll @@ -0,0 +1,61 @@ +/** Provides classes for working with XSJSLib modules. */ + +import javascript + +/** + * An XSJSLib module. + */ +class XSJSModule extends Module { + XSJSModule() { this.getFile().getExtension() = ["xsjs", "xsjslib"] } + + /** + * Get a value that is explicitly exported from this module with under `name`. + */ + override DataFlow::ValueNode getAnExportedValue(string name) { + exists(FunctionDeclStmt fds | + fds.getParent() = this.getTopLevel() and + fds.getName() = name and + result.getAstNode() = fds + ) + } +} + +/** + * An XSJSLib module import declaration. + * ``` + * $.import("module.xsjslib"); + * ``` + */ +class XSJSImportExpr extends CallExpr, Import { + XSJSImportExpr() { + this.getReceiver().(GlobalVarAccess).getName() = "$" and + this.getFile().getExtension() = ["xsjs", "xsjslib"] and + this.getCalleeName() = "import" + } + + override XSJSModule getEnclosingModule() { result = this.getTopLevel() } + + override PathExpr getImportedPath() { result = this.getLastArgument() } + + override DataFlow::Node getImportedModuleNode() { result = DataFlow::valueNode(this) } +} + +private class XSJSModuleImportPath extends PathExpr, ConstantString { + XSJSModuleImportPath() { this = any(XSJSImportExpr e).getLastArgument() } + + override Folder getSearchRoot(int priority) { + priority = 0 and + result = this.getFile().getParentContainer() + } + + override string getValue() { + exists(XSJSImportExpr e | + this = e.getArgument(0) and result = this.getStringValue() + or + this = e.getArgument(1) and + result = + e.getArgument(0).getStringValue().replaceAll(".", "/") + "/" + this.getStringValue() + + ".xsjslib" + ) + } +} \ No newline at end of file diff --git a/javascript/frameworks/xsjs/lib/advanced_security/javascript/frameworks/xsjs/XSJSReflectedXssQuery.qll b/javascript/frameworks/xsjs/lib/advanced_security/javascript/frameworks/xsjs/XSJSReflectedXssQuery.qll index 8ffdd364..7e584972 100644 --- a/javascript/frameworks/xsjs/lib/advanced_security/javascript/frameworks/xsjs/XSJSReflectedXssQuery.qll +++ b/javascript/frameworks/xsjs/lib/advanced_security/javascript/frameworks/xsjs/XSJSReflectedXssQuery.qll @@ -13,6 +13,15 @@ class XSJSResponseSetBodyCall extends MethodCallNode { XSJSResponse getParentXSJSResponse() { result = response } } +SourceNode xssSecure(TypeTracker t) { + t.start() and + result = moduleImport("@sap/xss-secure") + or + exists(TypeTracker t2 | result = xssSecure(t2).track(t2, t)) +} + +SourceNode xssSecure() { result = xssSecure(TypeTracker::end()) } + class Configuration extends TaintTracking::Configuration { Configuration() { this = "XSJS Reflected XSS Query" } @@ -35,4 +44,12 @@ class Configuration extends TaintTracking::Configuration { ) ) } + + override predicate isSanitizer(DataFlow::Node node) { + super.isSanitizer(node) or + node instanceof DomBasedXss::Sanitizer or + node = + xssSecure() + .getAMemberInvocation(["encodeCSS", "encodeHTML", "encodeJS", "encodeURL", "encodeXML"]) + } } diff --git a/javascript/frameworks/xsjs/test/codeql-pack.lock.yml b/javascript/frameworks/xsjs/test/codeql-pack.lock.yml index 68a286eb..2c0c0a33 100644 --- a/javascript/frameworks/xsjs/test/codeql-pack.lock.yml +++ b/javascript/frameworks/xsjs/test/codeql-pack.lock.yml @@ -5,16 +5,22 @@ dependencies: version: 1.1.2 codeql/javascript-all: version: 2.0.0 + codeql/javascript-queries: + version: 1.2.0 codeql/mad: version: 1.0.8 codeql/regex: version: 1.0.8 codeql/ssa: version: 1.0.8 + codeql/suite-helpers: + version: 1.0.8 codeql/tutorial: version: 1.0.8 codeql/typetracking: version: 1.0.8 + codeql/typos: + version: 1.0.8 codeql/util: version: 1.0.8 codeql/xml: diff --git a/javascript/frameworks/xsjs/test/models/modules/npm/npm.expected b/javascript/frameworks/xsjs/test/models/modules/npm/npm.expected new file mode 100644 index 00000000..d775ab92 --- /dev/null +++ b/javascript/frameworks/xsjs/test/models/modules/npm/npm.expected @@ -0,0 +1,2 @@ +| npm.xsjs:3:14:3:71 | crypto. ... 1024 }) | +| npm.xsjs:5:15:5:72 | crypto. ... 4096 }) | \ No newline at end of file diff --git a/javascript/frameworks/xsjs/test/models/modules/npm/npm.ql b/javascript/frameworks/xsjs/test/models/modules/npm/npm.ql new file mode 100644 index 00000000..0d2e6a88 --- /dev/null +++ b/javascript/frameworks/xsjs/test/models/modules/npm/npm.ql @@ -0,0 +1,4 @@ +import javascript + +from CryptographicKeyCreation crypto +select crypto diff --git a/javascript/frameworks/xsjs/test/models/modules/npm/npm.xsjs b/javascript/frameworks/xsjs/test/models/modules/npm/npm.xsjs new file mode 100644 index 00000000..7ecc391f --- /dev/null +++ b/javascript/frameworks/xsjs/test/models/modules/npm/npm.xsjs @@ -0,0 +1,5 @@ +const crypto = $.require("crypto"); + +const bad1 = crypto.generateKeyPairSync("rsa", { modulusLength: 1024 }); // NOT OK + +const good1 = crypto.generateKeyPairSync("rsa", { modulusLength: 4096 }); // OK \ No newline at end of file diff --git a/javascript/frameworks/xsjs/test/qlpack.yml b/javascript/frameworks/xsjs/test/qlpack.yml index 012962f0..04074b39 100644 --- a/javascript/frameworks/xsjs/test/qlpack.yml +++ b/javascript/frameworks/xsjs/test/qlpack.yml @@ -4,5 +4,6 @@ version: 0.1.0 extractor: javascript dependencies: codeql/javascript-all: "^2.0.0" + codeql/javascript-queries: "^1.2.0" advanced-security/javascript-sap-async-xsjs-queries: "^0.1.0" advanced-security/javascript-sap-async-xsjs-lib: "^0.1.0" diff --git a/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/XSJSLibSqlInjection.expected b/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/XSJSLibSqlInjection.expected new file mode 100644 index 00000000..ac992895 --- /dev/null +++ b/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/XSJSLibSqlInjection.expected @@ -0,0 +1,3 @@ +nodes +edges +#select diff --git a/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/XSJSLibSqlInjection.qlref b/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/XSJSLibSqlInjection.qlref new file mode 100644 index 00000000..2fefd671 --- /dev/null +++ b/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/XSJSLibSqlInjection.qlref @@ -0,0 +1 @@ +XSJSSqlInjection/XSJSSqlInjection.ql \ No newline at end of file diff --git a/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/XSJSLibSqlInjection.xsjs b/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/XSJSLibSqlInjection.xsjs new file mode 100644 index 00000000..7b70eab0 --- /dev/null +++ b/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/XSJSLibSqlInjection.xsjs @@ -0,0 +1,11 @@ +var requestParameters = $.request.parameters; +var param = JSON.parse(requestParameters.get("param")); + +let t1=$.import("lib/injection1.xsjslib"); +t1.test1(requestParameters); + +$.import("lib/injection2.xsjslib"); +$.lib.injection2.test2(param); + +let t3=$.import("lib.test3","injection3"); +t3.test3(param); diff --git a/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/lib/injection1.xsjslib b/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/lib/injection1.xsjslib new file mode 100644 index 00000000..92f183ab --- /dev/null +++ b/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/lib/injection1.xsjslib @@ -0,0 +1,8 @@ +function test1(requestParameters) { + let query = "INSERT INTO " + requestParameters + ".ENTITY (COL1) VALUES (" + requestParameters + ")"; + + let dbConnection = $.db.getConnection(); + let preparedStatement = dbConnection.prepareStatement(query); + preparedStatement.executeUpdate(); + dbConnection.commit(); +} diff --git a/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/lib/injection2.xsjslib b/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/lib/injection2.xsjslib new file mode 100644 index 00000000..f1a26bf5 --- /dev/null +++ b/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/lib/injection2.xsjslib @@ -0,0 +1,8 @@ +function test2(requestParameters) { + let query = "INSERT INTO " + requestParameters + ".ENTITY (COL1) VALUES (" + requestParameters + ")"; + + let dbConnection = $.db.getConnection(); + let preparedStatement = dbConnection.prepareStatement(query); + preparedStatement.executeUpdate(); + dbConnection.commit(); +} diff --git a/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/lib/test3/injection3.xsjslib b/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/lib/test3/injection3.xsjslib new file mode 100644 index 00000000..af7fa544 --- /dev/null +++ b/javascript/frameworks/xsjs/test/queries/XSJSLibSqlInjection/lib/test3/injection3.xsjslib @@ -0,0 +1,8 @@ +function test3(requestParameters) { + let query = "INSERT INTO " + requestParameters + ".ENTITY (COL1) VALUES (" + requestParameters + ")"; + + let dbConnection = $.db.getConnection(); + let preparedStatement = dbConnection.prepareStatement(query); + preparedStatement.executeUpdate(); + dbConnection.commit(); +} diff --git a/javascript/frameworks/xsjs/test/queries/XSJSReflectedXss/XSJSReflectedXss.expected b/javascript/frameworks/xsjs/test/queries/XSJSReflectedXss/XSJSReflectedXss.expected index 52be51fe..f442e9c7 100644 --- a/javascript/frameworks/xsjs/test/queries/XSJSReflectedXss/XSJSReflectedXss.expected +++ b/javascript/frameworks/xsjs/test/queries/XSJSReflectedXss/XSJSReflectedXss.expected @@ -23,6 +23,13 @@ nodes | XSJSReflectedXss.xsjs:32:22:32:65 | request ... Value3) | | XSJSReflectedXss.xsjs:32:22:32:65 | request ... Value3) | | XSJSReflectedXss.xsjs:32:46:32:64 | someParameterValue3 | +| XSJSReflectedXss.xsjs:45:7:45:67 | someParameterValue4 | +| XSJSReflectedXss.xsjs:45:29:45:67 | request ... eter4") | +| XSJSReflectedXss.xsjs:45:29:45:67 | request ... eter4") | +| XSJSReflectedXss.xsjs:47:22:47:87 | request ... alue4)) | +| XSJSReflectedXss.xsjs:47:22:47:87 | request ... alue4)) | +| XSJSReflectedXss.xsjs:47:46:47:86 | xssSecu ... Value4) | +| XSJSReflectedXss.xsjs:47:67:47:85 | someParameterValue4 | edges | XSJSReflectedXss.xsjs:11:7:11:67 | someParameterValue1 | XSJSReflectedXss.xsjs:13:46:13:64 | someParameterValue1 | | XSJSReflectedXss.xsjs:11:7:11:67 | someParameterValue1 | XSJSReflectedXss.xsjs:13:46:13:64 | someParameterValue1 | @@ -44,6 +51,11 @@ edges | XSJSReflectedXss.xsjs:31:29:31:67 | request ... eter3") | XSJSReflectedXss.xsjs:31:7:31:67 | someParameterValue3 | | XSJSReflectedXss.xsjs:32:46:32:64 | someParameterValue3 | XSJSReflectedXss.xsjs:32:22:32:65 | request ... Value3) | | XSJSReflectedXss.xsjs:32:46:32:64 | someParameterValue3 | XSJSReflectedXss.xsjs:32:22:32:65 | request ... Value3) | +| XSJSReflectedXss.xsjs:45:7:45:67 | someParameterValue4 | XSJSReflectedXss.xsjs:47:67:47:85 | someParameterValue4 | +| XSJSReflectedXss.xsjs:45:29:45:67 | request ... eter4") | XSJSReflectedXss.xsjs:45:7:45:67 | someParameterValue4 | +| XSJSReflectedXss.xsjs:45:29:45:67 | request ... eter4") | XSJSReflectedXss.xsjs:45:7:45:67 | someParameterValue4 | +| XSJSReflectedXss.xsjs:47:46:47:86 | xssSecu ... Value4) | XSJSReflectedXss.xsjs:47:22:47:87 | request ... alue4)) | +| XSJSReflectedXss.xsjs:47:46:47:86 | xssSecu ... Value4) | XSJSReflectedXss.xsjs:47:22:47:87 | request ... alue4)) | +| XSJSReflectedXss.xsjs:47:67:47:85 | someParameterValue4 | XSJSReflectedXss.xsjs:47:46:47:86 | xssSecu ... Value4) | #select -| XSJSReflectedXss.xsjs:13:22:13:65 | request ... Value1) | XSJSReflectedXss.xsjs:11:29:11:67 | request ... eter1") | XSJSReflectedXss.xsjs:13:22:13:65 | request ... Value1) | Reflected XSS vulnerability due to $@. | XSJSReflectedXss.xsjs:11:29:11:67 | request ... eter1") | user-provided value | - +| XSJSReflectedXss.xsjs:13:22:13:65 | request ... Value1) | XSJSReflectedXss.xsjs:11:29:11:67 | request ... eter1") | XSJSReflectedXss.xsjs:13:22:13:65 | request ... Value1) | Reflected XSS vulnerability due to $@. | XSJSReflectedXss.xsjs:11:29:11:67 | request ... eter1") | user-provided value | \ No newline at end of file diff --git a/javascript/frameworks/xsjs/test/queries/XSJSReflectedXss/XSJSReflectedXss.xsjs b/javascript/frameworks/xsjs/test/queries/XSJSReflectedXss/XSJSReflectedXss.xsjs index b2997cf7..9d63dca0 100644 --- a/javascript/frameworks/xsjs/test/queries/XSJSReflectedXss/XSJSReflectedXss.xsjs +++ b/javascript/frameworks/xsjs/test/queries/XSJSReflectedXss/XSJSReflectedXss.xsjs @@ -10,12 +10,12 @@ function requestParameterHandler(requestParameters) { function test1(requestParameters) { let someParameterValue1 = requestParameters.get("someParameter1"); $.response.contentType = "text/html"; - $.response.setBody(requestParameterHandler(someParameterValue1)); + $.response.setBody(requestParameterHandler(someParameterValue1)); // js/xsjs-reflected-xss $.response.status = $.net.http.OK; } /** - * False positive case: content type is "text/plain" + * True negative case: content type is "text/plain" */ function test2(requestParameters) { let someParameterValue2 = requestParameters.get("someParameter2"); @@ -25,7 +25,7 @@ function test2(requestParameters) { } /** - * False positive case: content type is not set + * True negative case: content type is not set */ function test3(requestParameters) { let someParameterValue3 = requestParameters.get("someParameter3"); @@ -36,3 +36,16 @@ function test3(requestParameters) { test1(requestParameters); test2(requestParameters); test3(requestParameters); + +/** + * True negative case: the value is sanitized + */ +var xssSecure = $.require('@sap/xss-secure'); +function test4(requestParameters) { + let someParameterValue4 = requestParameters.get("someParameter4"); + $.response.contentType = "text/html"; + $.response.setBody(requestParameterHandler(xssSecure.encodeHTML(someParameterValue4))); + $.response.status = $.net.http.OK; +} + +test4(requestParameters); \ No newline at end of file