From f3c0bf78268013b2d54b9d8869e64dba13aa1db3 Mon Sep 17 00:00:00 2001 From: valeria-meli Date: Tue, 27 Jul 2021 18:09:11 -0300 Subject: [PATCH 01/12] copy-paste from our repo --- .../Security/CWE-918/SSRF/SSRF.qhelp | 49 +++++++++++++++ .../Security/CWE-918/SSRF/SSRF.ql | 18 ++++++ .../Security/CWE-918/SSRF/SSRF.qll | 60 +++++++++++++++++++ 3 files changed, 127 insertions(+) create mode 100644 javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.qhelp create mode 100644 javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.ql create mode 100644 javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.qll diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.qhelp b/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.qhelp new file mode 100644 index 000000000000..05ac30b2e954 --- /dev/null +++ b/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.qhelp @@ -0,0 +1,49 @@ + + + + +

+ Directly incorporating user input into an HTTP request without validating the input can facilitate + server side request forgery attacks, where the attacker essentially controls the request. +

+ + + +

+ To guard against server side request forgery, it is advisable to avoid putting user input directly into a + network request. If using user input is necessary, then is mandatory to validate them. Only allow numeric and alphanumeric values. + URL encoding is not a solution in certain scenarios, such as, an architecture build over NGINX proxies. +

+ + + +

+ The following example shows an HTTP request parameter being used directly in a URL request without + validating the input, which facilitates an SSRF attack. The request axios.get("https://example.com/current_api/"+target) is + vulnerable since attackers can choose the value of target to be anything they want. For + instance, the attacker can choose "../super_secret_api" as the target, causing the + URL to become "https://example.com/super_secret_api". +

+ +

+ A request to https://example.com/super_secret_api may be problematic if that api is not + meant to be directly accessible from the attacker's machine. +

+ + + +

+ One way to remedy the problem is to validate the user input to only allow alphanumeric values: +

+ + + + + + +
  • OWASP: SSRF
    • + +     +     \ No newline at end of file diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.ql b/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.ql new file mode 100644 index 000000000000..1afbefe87e0b --- /dev/null +++ b/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.ql @@ -0,0 +1,18 @@ +/** + * @id javascript/ssrf-path + * @kind path-problem + * @name Uncontrolled data used in network request + * @description Sending network requests with user-controlled data as part of the URL allows for request forgery attacks. + * @problem.severity error + * @precision medium + * @tags security + * external/cwe/cwe-918 + */ + +import javascript +import SSRF +import DataFlow::PathGraph + +from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node request +where cfg.hasFlowPath(source, sink) and request = sink.getNode().(RequestForgery::Sink).getARequest() +select sink, source, sink, "The URL of this request depends on a user-provided value" diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.qll b/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.qll new file mode 100644 index 000000000000..8b868d0c2154 --- /dev/null +++ b/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.qll @@ -0,0 +1,60 @@ +import javascript + +import semmle.javascript.security.dataflow.RequestForgeryCustomizations +import semmle.javascript.security.dataflow.UrlConcatenation + +class Configuration extends TaintTracking::Configuration { + Configuration() { this = "SSRF" } + + override predicate isSource(DataFlow::Node source) { source instanceof RequestForgery::Source } + + override predicate isSink(DataFlow::Node sink) { sink instanceof RequestForgery::Sink } + + override predicate isSanitizer(DataFlow::Node node) { + super.isSanitizer(node) or + node instanceof RequestForgery::Sanitizer + } + + override predicate isSanitizerEdge(DataFlow::Node source, DataFlow::Node sink) { + sanitizingPrefixEdge(source, sink) + } + override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) { + nd instanceof IntegerCheck or + nd instanceof ValidatorCheck + } +} + +/** + * Number.isInteger is a sanitizer guard because a number can't be used to exploit a SSRF. + */ +class IntegerCheck extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode{ + IntegerCheck() { + this = DataFlow::globalVarRef("Number").getAMemberCall("isInteger") + } + + override predicate sanitizes(boolean outcome, Expr e){ + outcome = true and + e = getArgument(0).asExpr() + } +} + +/** + * ValidatorCheck identifies if exists a call to validator's library methods. + * validator is a library which has a variety of input-validation functions. We are interesed in + * checking that source is a number (any type of number) or an alphanumeric value. + */ +class ValidatorCheck extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode { + ValidatorCheck() { + exists( + DataFlow::SourceNode mod, string method | + mod = DataFlow::moduleImport("validator") and + this = mod.getAChainedMethodCall(method) + and method in ["isAlphanumeric", "isAlpha", "isDecimal", "isFloat", + "isHexadecimal", "isHexColor", "isInt", "isNumeric", "isOctal", "isUUID"] + ) + } + override predicate sanitizes(boolean outcome, Expr e){ + outcome = true and + e = getArgument(0).asExpr() + } +} From 92c874c2e2fb179ca3f77d53399ecf57dbb2513c Mon Sep 17 00:00:00 2001 From: valeria-meli Date: Tue, 3 Aug 2021 17:32:36 -0300 Subject: [PATCH 02/12] rename query --- javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.ql b/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.ql index 1afbefe87e0b..b4124e38cc3a 100644 --- a/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.ql +++ b/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.ql @@ -1,5 +1,5 @@ /** - * @id javascript/ssrf-path + * @id javascript/ssrf * @kind path-problem * @name Uncontrolled data used in network request * @description Sending network requests with user-controlled data as part of the URL allows for request forgery attacks. From 57ac944319ee22ecd24e526d5f02345c1e90214f Mon Sep 17 00:00:00 2001 From: valeria-meli Date: Tue, 3 Aug 2021 17:39:48 -0300 Subject: [PATCH 03/12] rename folders --- .../ql/src/experimental/Security/CWE-918/{SSRF => }/SSRF.qhelp | 0 .../ql/src/experimental/Security/CWE-918/{SSRF => }/SSRF.ql | 0 .../ql/src/experimental/Security/CWE-918/{SSRF => }/SSRF.qll | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename javascript/ql/src/experimental/Security/CWE-918/{SSRF => }/SSRF.qhelp (100%) rename javascript/ql/src/experimental/Security/CWE-918/{SSRF => }/SSRF.ql (100%) rename javascript/ql/src/experimental/Security/CWE-918/{SSRF => }/SSRF.qll (100%) diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.qhelp b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qhelp similarity index 100% rename from javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.qhelp rename to javascript/ql/src/experimental/Security/CWE-918/SSRF.qhelp diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.ql b/javascript/ql/src/experimental/Security/CWE-918/SSRF.ql similarity index 100% rename from javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.ql rename to javascript/ql/src/experimental/Security/CWE-918/SSRF.ql diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.qll b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll similarity index 100% rename from javascript/ql/src/experimental/Security/CWE-918/SSRF/SSRF.qll rename to javascript/ql/src/experimental/Security/CWE-918/SSRF.qll From 595ea6c383d3e01ae72b8d8679bb5eedf4fee9af Mon Sep 17 00:00:00 2001 From: valeria-meli Date: Tue, 3 Aug 2021 18:00:29 -0300 Subject: [PATCH 04/12] files for qhelp --- .../src/experimental/Security/CWE-918/SSRF.js | 15 ++++++++++++++ .../experimental/Security/CWE-918/SSRFGood.js | 20 +++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 javascript/ql/src/experimental/Security/CWE-918/SSRF.js create mode 100644 javascript/ql/src/experimental/Security/CWE-918/SSRFGood.js diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF.js b/javascript/ql/src/experimental/Security/CWE-918/SSRF.js new file mode 100644 index 000000000000..e39e97604417 --- /dev/null +++ b/javascript/ql/src/experimental/Security/CWE-918/SSRF.js @@ -0,0 +1,15 @@ +const axios = require('axios'); + +export const handler = async (req, res, next) => { + const { target } = req.body; + + try { + // BAD: `target` is controlled by the attacker + const response = await axios.get('https://example.com/current_api/' + target); + + // process request response + use(response); + } catch (err) { + // process error + } +}; diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRFGood.js b/javascript/ql/src/experimental/Security/CWE-918/SSRFGood.js new file mode 100644 index 000000000000..10ab26e607b7 --- /dev/null +++ b/javascript/ql/src/experimental/Security/CWE-918/SSRFGood.js @@ -0,0 +1,20 @@ +const axios = require('axios'); +const validator = require('validator'); + +export const handler = async (req, res, next) => { + const { target } = req.body; + + if (!validator.isAlphanumeric(target)) { + return next(new Error('Bad request')); + } + + try { + // `target` is validated + const response = await axios.get('https://example.com/current_api/' + target); + + // process request response + use(response); + } catch (err) { + // process error + } +}; From 0b5c8909dd383494e5e387f2e2f8ffd7e49ab50e Mon Sep 17 00:00:00 2001 From: valeria-meli Date: Tue, 3 Aug 2021 18:00:49 -0300 Subject: [PATCH 05/12] tests --- .../Security/CWE-918/SSRF.expected | 176 ++++++++++++++++++ .../experimental/Security/CWE-918/SSRF.qlref | 1 + .../Security/CWE-918/check-domain.js | 34 ++++ .../Security/CWE-918/check-middleware.js | 19 ++ .../Security/CWE-918/check-path.js | 54 ++++++ .../Security/CWE-918/check-regex.js | 46 +++++ .../Security/CWE-918/check-validator.js | 98 ++++++++++ 7 files changed, 428 insertions(+) create mode 100644 javascript/ql/test/experimental/Security/CWE-918/SSRF.expected create mode 100644 javascript/ql/test/experimental/Security/CWE-918/SSRF.qlref create mode 100644 javascript/ql/test/experimental/Security/CWE-918/check-domain.js create mode 100644 javascript/ql/test/experimental/Security/CWE-918/check-middleware.js create mode 100644 javascript/ql/test/experimental/Security/CWE-918/check-path.js create mode 100644 javascript/ql/test/experimental/Security/CWE-918/check-regex.js create mode 100644 javascript/ql/test/experimental/Security/CWE-918/check-validator.js diff --git a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected new file mode 100644 index 000000000000..5e1ffe1e982a --- /dev/null +++ b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected @@ -0,0 +1,176 @@ +nodes +| check-domain.js:16:9:16:27 | url | +| check-domain.js:16:15:16:27 | req.query.url | +| check-domain.js:16:15:16:27 | req.query.url | +| check-domain.js:17:13:17:15 | url | +| check-domain.js:17:13:17:15 | url | +| check-domain.js:26:15:26:27 | req.query.url | +| check-domain.js:26:15:26:27 | req.query.url | +| check-domain.js:26:15:26:27 | req.query.url | +| check-middleware.js:9:13:9:43 | "test.c ... tainted | +| check-middleware.js:9:13:9:43 | "test.c ... tainted | +| check-middleware.js:9:27:9:43 | req.query.tainted | +| check-middleware.js:9:27:9:43 | req.query.tainted | +| check-path.js:19:13:19:43 | 'test.c ... tainted | +| check-path.js:19:13:19:43 | 'test.c ... tainted | +| check-path.js:19:27:19:43 | req.query.tainted | +| check-path.js:19:27:19:43 | req.query.tainted | +| check-path.js:22:13:22:63 | 'test.c ... ainted) | +| check-path.js:22:13:22:63 | 'test.c ... ainted) | +| check-path.js:22:27:22:63 | encodeU ... ainted) | +| check-path.js:22:46:22:62 | req.query.tainted | +| check-path.js:22:46:22:62 | req.query.tainted | +| check-path.js:23:13:23:45 | `/addre ... inted}` | +| check-path.js:23:13:23:45 | `/addre ... inted}` | +| check-path.js:23:27:23:43 | req.query.tainted | +| check-path.js:23:27:23:43 | req.query.tainted | +| check-path.js:24:13:24:65 | `/addre ... nted)}` | +| check-path.js:24:13:24:65 | `/addre ... nted)}` | +| check-path.js:24:27:24:63 | encodeU ... ainted) | +| check-path.js:24:46:24:62 | req.query.tainted | +| check-path.js:24:46:24:62 | req.query.tainted | +| check-path.js:33:15:33:45 | 'test.c ... tainted | +| check-path.js:33:15:33:45 | 'test.c ... tainted | +| check-path.js:33:29:33:45 | req.query.tainted | +| check-path.js:33:29:33:45 | req.query.tainted | +| check-path.js:37:15:37:45 | 'test.c ... tainted | +| check-path.js:37:15:37:45 | 'test.c ... tainted | +| check-path.js:37:29:37:45 | req.query.tainted | +| check-path.js:37:29:37:45 | req.query.tainted | +| check-regex.js:24:15:24:42 | baseURL ... tainted | +| check-regex.js:24:15:24:42 | baseURL ... tainted | +| check-regex.js:24:25:24:42 | req.params.tainted | +| check-regex.js:24:25:24:42 | req.params.tainted | +| check-regex.js:31:15:31:45 | "test.c ... tainted | +| check-regex.js:31:15:31:45 | "test.c ... tainted | +| check-regex.js:31:29:31:45 | req.query.tainted | +| check-regex.js:31:29:31:45 | req.query.tainted | +| check-regex.js:34:15:34:42 | baseURL ... tainted | +| check-regex.js:34:15:34:42 | baseURL ... tainted | +| check-regex.js:34:25:34:42 | req.params.tainted | +| check-regex.js:34:25:34:42 | req.params.tainted | +| check-regex.js:41:13:41:43 | "test.c ... tainted | +| check-regex.js:41:13:41:43 | "test.c ... tainted | +| check-regex.js:41:27:41:43 | req.query.tainted | +| check-regex.js:41:27:41:43 | req.query.tainted | +| check-validator.js:15:15:15:45 | "test.c ... tainted | +| check-validator.js:15:15:15:45 | "test.c ... tainted | +| check-validator.js:15:29:15:45 | req.query.tainted | +| check-validator.js:15:29:15:45 | req.query.tainted | +| check-validator.js:27:15:27:45 | "test.c ... tainted | +| check-validator.js:27:15:27:45 | "test.c ... tainted | +| check-validator.js:27:29:27:45 | req.query.tainted | +| check-validator.js:27:29:27:45 | req.query.tainted | +| check-validator.js:50:15:50:45 | "test.c ... tainted | +| check-validator.js:50:15:50:45 | "test.c ... tainted | +| check-validator.js:50:29:50:45 | req.query.tainted | +| check-validator.js:50:29:50:45 | req.query.tainted | +| check-validator.js:54:9:54:37 | numberURL | +| check-validator.js:54:21:54:37 | req.query.tainted | +| check-validator.js:54:21:54:37 | req.query.tainted | +| check-validator.js:59:15:59:45 | "test.c ... tainted | +| check-validator.js:59:15:59:45 | "test.c ... tainted | +| check-validator.js:59:29:59:45 | req.query.tainted | +| check-validator.js:59:29:59:45 | req.query.tainted | +| check-validator.js:62:15:62:37 | "test.c ... mberURL | +| check-validator.js:62:15:62:37 | "test.c ... mberURL | +| check-validator.js:62:29:62:37 | numberURL | +| check-validator.js:68:15:68:45 | "test.c ... tainted | +| check-validator.js:68:15:68:45 | "test.c ... tainted | +| check-validator.js:68:29:68:45 | req.query.tainted | +| check-validator.js:68:29:68:45 | req.query.tainted | +edges +| check-domain.js:16:9:16:27 | url | check-domain.js:17:13:17:15 | url | +| check-domain.js:16:9:16:27 | url | check-domain.js:17:13:17:15 | url | +| check-domain.js:16:15:16:27 | req.query.url | check-domain.js:16:9:16:27 | url | +| check-domain.js:16:15:16:27 | req.query.url | check-domain.js:16:9:16:27 | url | +| check-domain.js:26:15:26:27 | req.query.url | check-domain.js:26:15:26:27 | req.query.url | +| check-middleware.js:9:27:9:43 | req.query.tainted | check-middleware.js:9:13:9:43 | "test.c ... tainted | +| check-middleware.js:9:27:9:43 | req.query.tainted | check-middleware.js:9:13:9:43 | "test.c ... tainted | +| check-middleware.js:9:27:9:43 | req.query.tainted | check-middleware.js:9:13:9:43 | "test.c ... tainted | +| check-middleware.js:9:27:9:43 | req.query.tainted | check-middleware.js:9:13:9:43 | "test.c ... tainted | +| check-path.js:19:27:19:43 | req.query.tainted | check-path.js:19:13:19:43 | 'test.c ... tainted | +| check-path.js:19:27:19:43 | req.query.tainted | check-path.js:19:13:19:43 | 'test.c ... tainted | +| check-path.js:19:27:19:43 | req.query.tainted | check-path.js:19:13:19:43 | 'test.c ... tainted | +| check-path.js:19:27:19:43 | req.query.tainted | check-path.js:19:13:19:43 | 'test.c ... tainted | +| check-path.js:22:27:22:63 | encodeU ... ainted) | check-path.js:22:13:22:63 | 'test.c ... ainted) | +| check-path.js:22:27:22:63 | encodeU ... ainted) | check-path.js:22:13:22:63 | 'test.c ... ainted) | +| check-path.js:22:46:22:62 | req.query.tainted | check-path.js:22:27:22:63 | encodeU ... ainted) | +| check-path.js:22:46:22:62 | req.query.tainted | check-path.js:22:27:22:63 | encodeU ... ainted) | +| check-path.js:23:27:23:43 | req.query.tainted | check-path.js:23:13:23:45 | `/addre ... inted}` | +| check-path.js:23:27:23:43 | req.query.tainted | check-path.js:23:13:23:45 | `/addre ... inted}` | +| check-path.js:23:27:23:43 | req.query.tainted | check-path.js:23:13:23:45 | `/addre ... inted}` | +| check-path.js:23:27:23:43 | req.query.tainted | check-path.js:23:13:23:45 | `/addre ... inted}` | +| check-path.js:24:27:24:63 | encodeU ... ainted) | check-path.js:24:13:24:65 | `/addre ... nted)}` | +| check-path.js:24:27:24:63 | encodeU ... ainted) | check-path.js:24:13:24:65 | `/addre ... nted)}` | +| check-path.js:24:46:24:62 | req.query.tainted | check-path.js:24:27:24:63 | encodeU ... ainted) | +| check-path.js:24:46:24:62 | req.query.tainted | check-path.js:24:27:24:63 | encodeU ... ainted) | +| check-path.js:33:29:33:45 | req.query.tainted | check-path.js:33:15:33:45 | 'test.c ... tainted | +| check-path.js:33:29:33:45 | req.query.tainted | check-path.js:33:15:33:45 | 'test.c ... tainted | +| check-path.js:33:29:33:45 | req.query.tainted | check-path.js:33:15:33:45 | 'test.c ... tainted | +| check-path.js:33:29:33:45 | req.query.tainted | check-path.js:33:15:33:45 | 'test.c ... tainted | +| check-path.js:37:29:37:45 | req.query.tainted | check-path.js:37:15:37:45 | 'test.c ... tainted | +| check-path.js:37:29:37:45 | req.query.tainted | check-path.js:37:15:37:45 | 'test.c ... tainted | +| check-path.js:37:29:37:45 | req.query.tainted | check-path.js:37:15:37:45 | 'test.c ... tainted | +| check-path.js:37:29:37:45 | req.query.tainted | check-path.js:37:15:37:45 | 'test.c ... tainted | +| check-regex.js:24:25:24:42 | req.params.tainted | check-regex.js:24:15:24:42 | baseURL ... tainted | +| check-regex.js:24:25:24:42 | req.params.tainted | check-regex.js:24:15:24:42 | baseURL ... tainted | +| check-regex.js:24:25:24:42 | req.params.tainted | check-regex.js:24:15:24:42 | baseURL ... tainted | +| check-regex.js:24:25:24:42 | req.params.tainted | check-regex.js:24:15:24:42 | baseURL ... tainted | +| check-regex.js:31:29:31:45 | req.query.tainted | check-regex.js:31:15:31:45 | "test.c ... tainted | +| check-regex.js:31:29:31:45 | req.query.tainted | check-regex.js:31:15:31:45 | "test.c ... tainted | +| check-regex.js:31:29:31:45 | req.query.tainted | check-regex.js:31:15:31:45 | "test.c ... tainted | +| check-regex.js:31:29:31:45 | req.query.tainted | check-regex.js:31:15:31:45 | "test.c ... tainted | +| check-regex.js:34:25:34:42 | req.params.tainted | check-regex.js:34:15:34:42 | baseURL ... tainted | +| check-regex.js:34:25:34:42 | req.params.tainted | check-regex.js:34:15:34:42 | baseURL ... tainted | +| check-regex.js:34:25:34:42 | req.params.tainted | check-regex.js:34:15:34:42 | baseURL ... tainted | +| check-regex.js:34:25:34:42 | req.params.tainted | check-regex.js:34:15:34:42 | baseURL ... tainted | +| check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | +| check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | +| check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | +| check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | +| check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | +| check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | +| check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | +| check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | +| check-validator.js:27:29:27:45 | req.query.tainted | check-validator.js:27:15:27:45 | "test.c ... tainted | +| check-validator.js:27:29:27:45 | req.query.tainted | check-validator.js:27:15:27:45 | "test.c ... tainted | +| check-validator.js:27:29:27:45 | req.query.tainted | check-validator.js:27:15:27:45 | "test.c ... tainted | +| check-validator.js:27:29:27:45 | req.query.tainted | check-validator.js:27:15:27:45 | "test.c ... tainted | +| check-validator.js:50:29:50:45 | req.query.tainted | check-validator.js:50:15:50:45 | "test.c ... tainted | +| check-validator.js:50:29:50:45 | req.query.tainted | check-validator.js:50:15:50:45 | "test.c ... tainted | +| check-validator.js:50:29:50:45 | req.query.tainted | check-validator.js:50:15:50:45 | "test.c ... tainted | +| check-validator.js:50:29:50:45 | req.query.tainted | check-validator.js:50:15:50:45 | "test.c ... tainted | +| check-validator.js:54:9:54:37 | numberURL | check-validator.js:62:29:62:37 | numberURL | +| check-validator.js:54:21:54:37 | req.query.tainted | check-validator.js:54:9:54:37 | numberURL | +| check-validator.js:54:21:54:37 | req.query.tainted | check-validator.js:54:9:54:37 | numberURL | +| check-validator.js:59:29:59:45 | req.query.tainted | check-validator.js:59:15:59:45 | "test.c ... tainted | +| check-validator.js:59:29:59:45 | req.query.tainted | check-validator.js:59:15:59:45 | "test.c ... tainted | +| check-validator.js:59:29:59:45 | req.query.tainted | check-validator.js:59:15:59:45 | "test.c ... tainted | +| check-validator.js:59:29:59:45 | req.query.tainted | check-validator.js:59:15:59:45 | "test.c ... tainted | +| check-validator.js:62:29:62:37 | numberURL | check-validator.js:62:15:62:37 | "test.c ... mberURL | +| check-validator.js:62:29:62:37 | numberURL | check-validator.js:62:15:62:37 | "test.c ... mberURL | +| check-validator.js:68:29:68:45 | req.query.tainted | check-validator.js:68:15:68:45 | "test.c ... tainted | +| check-validator.js:68:29:68:45 | req.query.tainted | check-validator.js:68:15:68:45 | "test.c ... tainted | +| check-validator.js:68:29:68:45 | req.query.tainted | check-validator.js:68:15:68:45 | "test.c ... tainted | +| check-validator.js:68:29:68:45 | req.query.tainted | check-validator.js:68:15:68:45 | "test.c ... tainted | +#select +| check-domain.js:17:13:17:15 | url | check-domain.js:16:15:16:27 | req.query.url | check-domain.js:17:13:17:15 | url | The URL of this request depends on a user-provided value | +| check-domain.js:26:15:26:27 | req.query.url | check-domain.js:26:15:26:27 | req.query.url | check-domain.js:26:15:26:27 | req.query.url | The URL of this request depends on a user-provided value | +| check-middleware.js:9:13:9:43 | "test.c ... tainted | check-middleware.js:9:27:9:43 | req.query.tainted | check-middleware.js:9:13:9:43 | "test.c ... tainted | The URL of this request depends on a user-provided value | +| check-path.js:19:13:19:43 | 'test.c ... tainted | check-path.js:19:27:19:43 | req.query.tainted | check-path.js:19:13:19:43 | 'test.c ... tainted | The URL of this request depends on a user-provided value | +| check-path.js:22:13:22:63 | 'test.c ... ainted) | check-path.js:22:46:22:62 | req.query.tainted | check-path.js:22:13:22:63 | 'test.c ... ainted) | The URL of this request depends on a user-provided value | +| check-path.js:23:13:23:45 | `/addre ... inted}` | check-path.js:23:27:23:43 | req.query.tainted | check-path.js:23:13:23:45 | `/addre ... inted}` | The URL of this request depends on a user-provided value | +| check-path.js:24:13:24:65 | `/addre ... nted)}` | check-path.js:24:46:24:62 | req.query.tainted | check-path.js:24:13:24:65 | `/addre ... nted)}` | The URL of this request depends on a user-provided value | +| check-path.js:33:15:33:45 | 'test.c ... tainted | check-path.js:33:29:33:45 | req.query.tainted | check-path.js:33:15:33:45 | 'test.c ... tainted | The URL of this request depends on a user-provided value | +| check-path.js:37:15:37:45 | 'test.c ... tainted | check-path.js:37:29:37:45 | req.query.tainted | check-path.js:37:15:37:45 | 'test.c ... tainted | The URL of this request depends on a user-provided value | +| check-regex.js:24:15:24:42 | baseURL ... tainted | check-regex.js:24:25:24:42 | req.params.tainted | check-regex.js:24:15:24:42 | baseURL ... tainted | The URL of this request depends on a user-provided value | +| check-regex.js:31:15:31:45 | "test.c ... tainted | check-regex.js:31:29:31:45 | req.query.tainted | check-regex.js:31:15:31:45 | "test.c ... tainted | The URL of this request depends on a user-provided value | +| check-regex.js:34:15:34:42 | baseURL ... tainted | check-regex.js:34:25:34:42 | req.params.tainted | check-regex.js:34:15:34:42 | baseURL ... tainted | The URL of this request depends on a user-provided value | +| check-regex.js:41:13:41:43 | "test.c ... tainted | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | The URL of this request depends on a user-provided value | +| check-validator.js:15:15:15:45 | "test.c ... tainted | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | The URL of this request depends on a user-provided value | +| check-validator.js:27:15:27:45 | "test.c ... tainted | check-validator.js:27:29:27:45 | req.query.tainted | check-validator.js:27:15:27:45 | "test.c ... tainted | The URL of this request depends on a user-provided value | +| check-validator.js:50:15:50:45 | "test.c ... tainted | check-validator.js:50:29:50:45 | req.query.tainted | check-validator.js:50:15:50:45 | "test.c ... tainted | The URL of this request depends on a user-provided value | +| check-validator.js:59:15:59:45 | "test.c ... tainted | check-validator.js:59:29:59:45 | req.query.tainted | check-validator.js:59:15:59:45 | "test.c ... tainted | The URL of this request depends on a user-provided value | +| check-validator.js:62:15:62:37 | "test.c ... mberURL | check-validator.js:54:21:54:37 | req.query.tainted | check-validator.js:62:15:62:37 | "test.c ... mberURL | The URL of this request depends on a user-provided value | +| check-validator.js:68:15:68:45 | "test.c ... tainted | check-validator.js:68:29:68:45 | req.query.tainted | check-validator.js:68:15:68:45 | "test.c ... tainted | The URL of this request depends on a user-provided value | diff --git a/javascript/ql/test/experimental/Security/CWE-918/SSRF.qlref b/javascript/ql/test/experimental/Security/CWE-918/SSRF.qlref new file mode 100644 index 000000000000..05a9c8145e60 --- /dev/null +++ b/javascript/ql/test/experimental/Security/CWE-918/SSRF.qlref @@ -0,0 +1 @@ +./experimental/Security/CWE-918/SSRF.ql \ No newline at end of file diff --git a/javascript/ql/test/experimental/Security/CWE-918/check-domain.js b/javascript/ql/test/experimental/Security/CWE-918/check-domain.js new file mode 100644 index 000000000000..0821140ab5fe --- /dev/null +++ b/javascript/ql/test/experimental/Security/CWE-918/check-domain.js @@ -0,0 +1,34 @@ +// native modules +const url = require('url'); + +// dependencies +const axios = require('axios'); +const express = require('express'); + +// constants +const VALID_DOMAINS = ['example.com', 'example-2.com']; + +// start +const app = express(); + +app.get('/check-with-axios', req => { + // without validation + const url = req.query.url; + axios.get(url); //SSRF + + // validating domain only + const decodedURI = decodeURIComponent(req.query.url); + const { hostname } = url.parse(decodedURI); + + const { hostname } = url.parse(decodedURI); + + if (isValidDomain(hostname, validDomains)) { + axios.get(req.query.url); //SSRF + } +}); + +const isValidDomain = (hostname, validDomains) => ( + validDomains.some(domain => ( + hostname === domain || hostname.endsWith(`.${domain}`)) + ) +); \ No newline at end of file diff --git a/javascript/ql/test/experimental/Security/CWE-918/check-middleware.js b/javascript/ql/test/experimental/Security/CWE-918/check-middleware.js new file mode 100644 index 000000000000..2a1e6d541665 --- /dev/null +++ b/javascript/ql/test/experimental/Security/CWE-918/check-middleware.js @@ -0,0 +1,19 @@ +// dependencies +const axios = require('axios'); +const express = require('express'); + +// start +const app = express(); + +app.get('/check-with-axios', validationMiddleware, req => { + axios.get("test.com/" + req.query.tainted); // OK is sanitized by the middleware - False Positive +}); + + +const validationMiddleware = (req, res, next) => { + if (!Number.isInteger(req.query.tainted)) { + return res.sendStatus(400); + } + + next(); +} diff --git a/javascript/ql/test/experimental/Security/CWE-918/check-path.js b/javascript/ql/test/experimental/Security/CWE-918/check-path.js new file mode 100644 index 000000000000..35681d58491b --- /dev/null +++ b/javascript/ql/test/experimental/Security/CWE-918/check-path.js @@ -0,0 +1,54 @@ +// native modules +const path = require('path'); +const url = require('url'); + +// dependencies +const axios = require('axios'); +const express = require('express'); + +// constants +const VALID_PATHS = ['/api/users/me', '/help', '/system/health']; + +// start +const app = express(); + +app.get('/check-with-axios', req => { + const hardcoded = 'hardcodeado'; + + axios.get('test.com/' + hardcoded); // OK + axios.get('test.com/' + req.query.tainted); // SSRF + axios.get('test.com/' + Number(req.query.tainted)); // OK + axios.get('test.com/' + req.user.id); // OK + axios.get('test.com/' + encodeURIComponent(req.query.tainted)); // SSRF + axios.get(`/addresses/${req.query.tainted}`); // SSRF + axios.get(`/addresses/${encodeURIComponent(req.query.tainted)}`); // SSRF + + if (Number.isInteger(req.query.tainted)) { + axios.get('test.com/' + req.query.tainted); // OK + } + + if (isValidInput(req.query.tainted)){ + axios.get('test.com/' + req.query.tainted); // OK + } else { + axios.get('test.com/' + req.query.tainted); // SSRF + } + + if (doesntCheckAnything(req.query.tainted)) { + axios.get('test.com/' + req.query.tainted); // SSRF + } + + if (isValidPath(req.query.tainted, VALID_PATHS)) { + axios.get('test.com/' + req.query.tainted) // OK + } + + if(!isValidInput(req.query.tainted)) { + return; + } + axios.get("test.com/" + req.query.tainted); // OK +}); + +const isValidPath = (path, validPaths) => validPaths.includes(path); + +const isValidInput = (path) => Number.isInteger(path); + +const doesntCheckAnything = (path) => true; diff --git a/javascript/ql/test/experimental/Security/CWE-918/check-regex.js b/javascript/ql/test/experimental/Security/CWE-918/check-regex.js new file mode 100644 index 000000000000..a05c1ae7ddfa --- /dev/null +++ b/javascript/ql/test/experimental/Security/CWE-918/check-regex.js @@ -0,0 +1,46 @@ +// dependencies +const axios = require('axios'); +const express = require('express'); + +// start +const app = express(); + +app.get('/check-with-axios', req => { + if (req.query.tainted.match(/^[0-9a-z]+$/)) { // letters and numbers + axios.get("test.com/" + req.query.tainted); // OK + } + if (req.query.tainted.match(/^[0-9a-z\-_]+$/)) { // letters, numbers, - and _ + axios.get("test.com/" + req.query.tainted); // OK + } + if (req.query.tainted.match(/^.*$/)) { // anything + axios.get("test.com/" + req.query.tainted); // SSRF - False Negative + } + + const baseURL = "test.com/" + if (isValidPath(req.params.tainted) ) { + axios.get(baseURL + req.params.tainted); // OK + } + if (!isValidPath(req.params.tainted) ) { + axios.get(baseURL + req.params.tainted); // SSRF + } else { + axios.get(baseURL + req.params.tainted); // OK + } + + // Blacklists are not safe + if (!req.query.tainted.match(/^[/\.%]+$/)) { + axios.get("test.com/" + req.query.tainted); // SSRF + } + if (!isInBlacklist(req.params.tainted) ) { + axios.get(baseURL + req.params.tainted); // SSRF + } + + if (!isValidPath(req.params.tainted)) { + return; + } + + axios.get("test.com/" + req.query.tainted); // OK - False Positive +}); + +const isValidPath = path => path.match(/^[0-9a-z]+$/); + +const isInBlackList = path => path.match(/^[/\.%]+$/); diff --git a/javascript/ql/test/experimental/Security/CWE-918/check-validator.js b/javascript/ql/test/experimental/Security/CWE-918/check-validator.js new file mode 100644 index 000000000000..dfe3314b07b3 --- /dev/null +++ b/javascript/ql/test/experimental/Security/CWE-918/check-validator.js @@ -0,0 +1,98 @@ +// dependencies +const axios = require('axios'); +const express = require('express'); +const validator = require('validator'); + +// start +const app = express(); + +app.get("/check-with-axios", req => { + // alphanumeric + if (validator.isAlphanumeric(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // OK + } + if (isAlphanumeric(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // SSRF + } + if (validAlphanumeric(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // OK + } + if (validAlpha(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // OK + } + if (validNumber(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // OK + } + if (wrongValidation(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // SSRF + } + + // numbers + if (validHexadecimal(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // OK + } + if (validHexaColor(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // OK + } + if (validDecimal(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // OK + } + if (validFloat(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // OK + } + if (validInt(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // OK + } + if (validOctal(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // OK + } + if (validHexa(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // OK. False Positive + } + + // with simple assignation + const numberURL = req.query.tainted; + if (validNumber(numberURL)) { + axios.get("test.com/" + numberURL); // OK + } + if (validNumber(numberURL)) { + axios.get("test.com/" + req.query.tainted); // OK. False Positive + } + if (validNumber(req.query.tainted)) { + axios.get("test.com/" + numberURL); // OK. False Positive + } + + if (validHexadecimal(req.query.tainted) || validHexaColor(req.query.tainted) || + validDecimal(req.query.tainted) || validFloat(req.query.tainted) || validInt(req.query.tainted) || + validNumber(req.query.tainted) || validOctal(req.query.tainted)) { + axios.get("test.com/" + req.query.tainted); // OK. False Positive + } +}); + +// safe validators +const validAlphanumeric = url => validator.isAlphanumeric(url); + +const validAlpha = url => validator.isAlpha(url); + +const validDecimal = url => validator.isDecimal(url); + +const validFloat = url => validator.isFloat(url); + +const validInt = url => validator.isInt(url); + +const validNumber = url => validator.isNumeric(url); + +const validOctal = url => validator.isOctal(url); + +const validHexa = url => validator.isHexadecimal(url) || validator.isHexColor(url); + +const validHexadecimal = url => validator.isHexadecimal(url); + +const validHexaColor = url => validator.isHexColor(url); + +const validUUID = url => validator.isUUID(url); + +// unsafe validators +const wrongValidation = url => validator.isByteLength(url, {min:4,max:8}); + +const isAlphanumeric = url => true; From 629efb85fb50188f6e2a1b9153059f698b2a4a1f Mon Sep 17 00:00:00 2001 From: Nati Pesaresi Date: Thu, 2 Sep 2021 17:55:09 -0300 Subject: [PATCH 06/12] ternary operator --- .../experimental/Security/CWE-918/SSRF.qll | 61 +++++- .../Security/CWE-918/ternary-operator.js | 182 ++++++++++++++++++ 2 files changed, 242 insertions(+), 1 deletion(-) create mode 100644 javascript/ql/test/experimental/Security/CWE-918/ternary-operator.js diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll index 8b868d0c2154..3cfde23453fb 100644 --- a/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll +++ b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll @@ -20,7 +20,66 @@ class Configuration extends TaintTracking::Configuration { } override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) { nd instanceof IntegerCheck or - nd instanceof ValidatorCheck + nd instanceof ValidatorCheck or + nd instanceof TernaryOperatorSanitizerGuard + } +} + +/** TODO add comment */ +class TernaryOperatorSanitizerGuard extends TaintTracking::SanitizerGuardNode { + TaintTracking::SanitizerGuardNode originalGuard; + + TernaryOperatorSanitizerGuard() { + exists(DataFlow::Node falseNode | + this.getAPredecessor+() = falseNode and + falseNode.asExpr().(BooleanLiteral).mayHaveBooleanValue(false) + ) and + this.getAPredecessor+() = originalGuard and + not this.asExpr() instanceof LogicalBinaryExpr + } + + override predicate sanitizes(boolean outcome, Expr e) { + not this.asExpr() instanceof LogNotExpr and + originalGuard.sanitizes(outcome, e) + or + exists(boolean originalOutcome | + this.asExpr() instanceof LogNotExpr and + originalGuard.sanitizes(originalOutcome, e) and + ( + originalOutcome = true and outcome = false + or + originalOutcome = false and outcome = true + ) + ) + } +} + +/** TODO add comment */ +class TernaryOperatorSanitizer extends RequestForgery::Sanitizer { + TernaryOperatorSanitizer() { + exists( + TaintTracking::SanitizerGuardNode guard, IfStmt ifStmt, DataFlow::Node taintedInput, boolean outcome, + Stmt r, DataFlow::Node falseNode + | + ifStmt.getCondition().flow().getAPredecessor+() = guard and + ifStmt.getCondition().flow().getAPredecessor+() = falseNode and + falseNode.asExpr().(BooleanLiteral).mayHaveBooleanValue(false) and + not ifStmt.getCondition() instanceof LogicalBinaryExpr and + guard.sanitizes(outcome, taintedInput.asExpr()) and + ( + outcome = true and r = ifStmt.getThen() and not ifStmt.getCondition() instanceof LogNotExpr + or + outcome = false and r = ifStmt.getElse() and not ifStmt.getCondition() instanceof LogNotExpr + or + outcome = false and r = ifStmt.getThen() and ifStmt.getCondition() instanceof LogNotExpr + or + outcome = true and r = ifStmt.getElse() and ifStmt.getCondition() instanceof LogNotExpr + ) and + r.getFirstControlFlowNode() + .getBasicBlock() + .(ReachableBasicBlock) + .dominates(this.getBasicBlock()) + ) } } diff --git a/javascript/ql/test/experimental/Security/CWE-918/ternary-operator.js b/javascript/ql/test/experimental/Security/CWE-918/ternary-operator.js new file mode 100644 index 000000000000..26e2607a80f2 --- /dev/null +++ b/javascript/ql/test/experimental/Security/CWE-918/ternary-operator.js @@ -0,0 +1,182 @@ +const express = require('express'); +const app = express(); + +app.use(express.json()); + +app.get('/direct-ternary-operator', function (req, res) { + let taintedURL = req.params.url + + let v = req.params.url ? req.params.url == "someURL" : false + if (v) { + req_frontend_restclient.get(req.params.url) // OK + } + + let v1 = taintedURL ? taintedURL == "someURL" : false + if (v1) { + req_frontend_restclient.get(taintedURL) // OK + } + + let v2 = taintedURL ? valid(taintedURL) : false + if (v2) { + req_frontend_restclient.get(taintedURL) // OK + } + + let v3 = req.params.url ? valid(req.params.url) : false + if (v3) { + req_frontend_restclient.get(req.params.url) // OK + } + + let v4 = req.params.url == undefined ? false : valid(req.params.url) + if (v4) { + req_frontend_restclient.get(req.params.url) // OK + } + + let v5 = req.params.url == undefined ? true : valid(req.params.url) + if (v5) { + req_frontend_restclient.get(req.params.url) // SSRF + } + + let v6 = req.params.url ? valid(req.params.url) : true + if (v6) { + req_frontend_restclient.get(req.params.url) // SSRF + } + + let f = false + let v7 = req.params.url ? valid(req.params.url) : true + if (v7) { + req_frontend_restclient.get(req.params.url) // SSRF + } + + let v8 = req.params.url == undefined ? false : valid(req.params.url) + if (!v8) { + return + } + req_frontend_restclient.get(req.params.url) // OK +}) + +app.get('/functions', function (req, res) { + let taintedURL = req.params.url + + if (valid2(taintedURL)) { + req_frontend_restclient.get(taintedURL) // OK + } + + if (!invalid(taintedURL)) { + req_frontend_restclient.get(taintedURL) // False positive + } + + if (valid2(req.params.url)){ + req_frontend_restclient.get(req.params.url) // OK + } + + if (!assertAlphanumeric(req.params.url)) { + return + } + req_frontend_restclient.get(req.params.url); // OK +}) + +app.get('/normal-use-of-ternary-operator', function (req, res) { + let taintedURL = req.params.url + + let url = valid(req.params.url) ? req.params.url : undefined + req_frontend_restclient.get(url) // OK + + let url = valid(taintedURL) ? taintedURL : undefined + req_frontend_restclient.get(url) // OK + + let url4 = req.params.url.match(/^[\w.-]+$/) ? req.params.url : undefined + req_frontend_restclient.get(url4) // OK +}) + +app.get('/throw-errors', function (req, res) { + req_frontend_restclient.get(valid3(req.params.url)) // False positive + + req_frontend_restclient.get(assertOther(req.params.url)); // False positive + + req_frontend_restclient.get(assertOther2(req.params.url)); // False positive +}); + +app.get('/bad-endpoint', function (req, res) { + req_frontend_restclient.get(req.params.url); // SSRF + + const valid = req.params.url ? req.params.url == "someURL" : false + if (!valid) { + throw new Error(`Invalid parameter: "${req.params.url}", must be alphanumeric`); + } + req_frontend_restclient.get(req.params.url); // OK +}) + + +app.get('/bad-endpoint-variable', function (req, res) { + let taintedURL = req.params.url + req_frontend_restclient.get(taintedURL); // SSRF + + const valid = taintedURL ? taintedURL == "someURL" : false + if (!valid) { + return + } + req_frontend_restclient.get(taintedURL); // False positive +}) + +app.get('/not-invalid', function (req, res) { + const invalidParam = req.params.url ? !Number.isInteger(req.params.url) : false + if (invalidParam) { + return + } + req_frontend_restclient.get(req.params.url); // False positive +}) + + +app.get('/bad-endpoint-2', function (req, res) { + other(req.params.url) +}) + +function other(taintedURL) { + req_frontend_restclient.get(taintedURL); // SSRF + + const valid = taintedURL ? taintedURL == "someURL" : false + if (!valid) { + return + } + req_frontend_restclient.get(taintedURL); // False positive +} + +function assertAlphanumeric(value) { + return value ? value.match(/^[\w.-]+$/) : false; +} + +function assertOther(value) { + const valid = value ? !!value.match(/^[\w.-]+$/) : false; + if (!valid) { + throw new Error(`Invalid parameter: "${value}", must be alphanumeric`); + } + return value; +} + +function assertOther2(value) { + const valid = value ? value.match(/^[\w.-]+$/) : false; + if (!valid) { + throw new Error(`Invalid parameter: "${value}", must be alphanumeric`); + } + return value; +} + +function invalid(value) { + return value ? !Number.isInteger(value) : true +} + +function valid(value) { + return value.match(/^[\w.-]+$/) +} + +function valid2(value) { + return value ? value == "someURL" : false +} + +function valid3(value) { + const valid = value ? value == "someURL" : false + if (!valid) { + throw new Error(`Invalid parameter: "${value}", must be alphanumeric`); + } + return value; +} \ No newline at end of file From 0b0ac8317cbebe9ed78e90d83e42cc77d00ef1e8 Mon Sep 17 00:00:00 2001 From: luciaromeroML Date: Fri, 17 Sep 2021 18:05:52 -0300 Subject: [PATCH 07/12] format ql code --- .../src/experimental/Security/CWE-918/SSRF.ql | 3 +- .../experimental/Security/CWE-918/SSRF.qll | 134 +++++++++--------- 2 files changed, 69 insertions(+), 68 deletions(-) diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF.ql b/javascript/ql/src/experimental/Security/CWE-918/SSRF.ql index b4124e38cc3a..f13f5fae14e2 100644 --- a/javascript/ql/src/experimental/Security/CWE-918/SSRF.ql +++ b/javascript/ql/src/experimental/Security/CWE-918/SSRF.ql @@ -14,5 +14,6 @@ import SSRF import DataFlow::PathGraph from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node request -where cfg.hasFlowPath(source, sink) and request = sink.getNode().(RequestForgery::Sink).getARequest() +where + cfg.hasFlowPath(source, sink) and request = sink.getNode().(RequestForgery::Sink).getARequest() select sink, source, sink, "The URL of this request depends on a user-provided value" diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll index 3cfde23453fb..698e1e8f379d 100644 --- a/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll +++ b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll @@ -1,72 +1,72 @@ import javascript - import semmle.javascript.security.dataflow.RequestForgeryCustomizations import semmle.javascript.security.dataflow.UrlConcatenation class Configuration extends TaintTracking::Configuration { - Configuration() { this = "SSRF" } + Configuration() { this = "SSRF" } + + override predicate isSource(DataFlow::Node source) { source instanceof RequestForgery::Source } - override predicate isSource(DataFlow::Node source) { source instanceof RequestForgery::Source } + override predicate isSink(DataFlow::Node sink) { sink instanceof RequestForgery::Sink } - override predicate isSink(DataFlow::Node sink) { sink instanceof RequestForgery::Sink } + override predicate isSanitizer(DataFlow::Node node) { + super.isSanitizer(node) or + node instanceof RequestForgery::Sanitizer + } - override predicate isSanitizer(DataFlow::Node node) { - super.isSanitizer(node) or - node instanceof RequestForgery::Sanitizer - } + override predicate isSanitizerEdge(DataFlow::Node source, DataFlow::Node sink) { + sanitizingPrefixEdge(source, sink) + } - override predicate isSanitizerEdge(DataFlow::Node source, DataFlow::Node sink) { - sanitizingPrefixEdge(source, sink) - } - override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) { - nd instanceof IntegerCheck or - nd instanceof ValidatorCheck or - nd instanceof TernaryOperatorSanitizerGuard - } + override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) { + nd instanceof IntegerCheck or + nd instanceof ValidatorCheck or + nd instanceof TernaryOperatorSanitizerGuard + } } /** TODO add comment */ class TernaryOperatorSanitizerGuard extends TaintTracking::SanitizerGuardNode { - TaintTracking::SanitizerGuardNode originalGuard; + TaintTracking::SanitizerGuardNode originalGuard; - TernaryOperatorSanitizerGuard() { + TernaryOperatorSanitizerGuard() { exists(DataFlow::Node falseNode | - this.getAPredecessor+() = falseNode and - falseNode.asExpr().(BooleanLiteral).mayHaveBooleanValue(false) + this.getAPredecessor+() = falseNode and + falseNode.asExpr().(BooleanLiteral).mayHaveBooleanValue(false) ) and - this.getAPredecessor+() = originalGuard and + this.getAPredecessor+() = originalGuard and not this.asExpr() instanceof LogicalBinaryExpr - } + } - override predicate sanitizes(boolean outcome, Expr e) { - not this.asExpr() instanceof LogNotExpr and - originalGuard.sanitizes(outcome, e) + override predicate sanitizes(boolean outcome, Expr e) { + not this.asExpr() instanceof LogNotExpr and + originalGuard.sanitizes(outcome, e) or exists(boolean originalOutcome | - this.asExpr() instanceof LogNotExpr and - originalGuard.sanitizes(originalOutcome, e) and - ( - originalOutcome = true and outcome = false + this.asExpr() instanceof LogNotExpr and + originalGuard.sanitizes(originalOutcome, e) and + ( + originalOutcome = true and outcome = false or originalOutcome = false and outcome = true - ) + ) ) - } + } } /** TODO add comment */ class TernaryOperatorSanitizer extends RequestForgery::Sanitizer { - TernaryOperatorSanitizer() { + TernaryOperatorSanitizer() { exists( - TaintTracking::SanitizerGuardNode guard, IfStmt ifStmt, DataFlow::Node taintedInput, boolean outcome, - Stmt r, DataFlow::Node falseNode + TaintTracking::SanitizerGuardNode guard, IfStmt ifStmt, DataFlow::Node taintedInput, + boolean outcome, Stmt r, DataFlow::Node falseNode | - ifStmt.getCondition().flow().getAPredecessor+() = guard and - ifStmt.getCondition().flow().getAPredecessor+() = falseNode and - falseNode.asExpr().(BooleanLiteral).mayHaveBooleanValue(false) and - not ifStmt.getCondition() instanceof LogicalBinaryExpr and - guard.sanitizes(outcome, taintedInput.asExpr()) and - ( + ifStmt.getCondition().flow().getAPredecessor+() = guard and + ifStmt.getCondition().flow().getAPredecessor+() = falseNode and + falseNode.asExpr().(BooleanLiteral).mayHaveBooleanValue(false) and + not ifStmt.getCondition() instanceof LogicalBinaryExpr and + guard.sanitizes(outcome, taintedInput.asExpr()) and + ( outcome = true and r = ifStmt.getThen() and not ifStmt.getCondition() instanceof LogNotExpr or outcome = false and r = ifStmt.getElse() and not ifStmt.getCondition() instanceof LogNotExpr @@ -74,27 +74,25 @@ class TernaryOperatorSanitizer extends RequestForgery::Sanitizer { outcome = false and r = ifStmt.getThen() and ifStmt.getCondition() instanceof LogNotExpr or outcome = true and r = ifStmt.getElse() and ifStmt.getCondition() instanceof LogNotExpr - ) and - r.getFirstControlFlowNode() - .getBasicBlock() - .(ReachableBasicBlock) - .dominates(this.getBasicBlock()) + ) and + r.getFirstControlFlowNode() + .getBasicBlock() + .(ReachableBasicBlock) + .dominates(this.getBasicBlock()) ) - } + } } /** * Number.isInteger is a sanitizer guard because a number can't be used to exploit a SSRF. */ -class IntegerCheck extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode{ - IntegerCheck() { - this = DataFlow::globalVarRef("Number").getAMemberCall("isInteger") - } +class IntegerCheck extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode { + IntegerCheck() { this = DataFlow::globalVarRef("Number").getAMemberCall("isInteger") } - override predicate sanitizes(boolean outcome, Expr e){ - outcome = true and - e = getArgument(0).asExpr() - } + override predicate sanitizes(boolean outcome, Expr e) { + outcome = true and + e = getArgument(0).asExpr() + } } /** @@ -103,17 +101,19 @@ class IntegerCheck extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode * checking that source is a number (any type of number) or an alphanumeric value. */ class ValidatorCheck extends TaintTracking::SanitizerGuardNode, DataFlow::CallNode { - ValidatorCheck() { - exists( - DataFlow::SourceNode mod, string method | - mod = DataFlow::moduleImport("validator") and - this = mod.getAChainedMethodCall(method) - and method in ["isAlphanumeric", "isAlpha", "isDecimal", "isFloat", - "isHexadecimal", "isHexColor", "isInt", "isNumeric", "isOctal", "isUUID"] - ) - } - override predicate sanitizes(boolean outcome, Expr e){ - outcome = true and - e = getArgument(0).asExpr() - } + ValidatorCheck() { + exists(DataFlow::SourceNode mod, string method | + mod = DataFlow::moduleImport("validator") and + this = mod.getAChainedMethodCall(method) and + method in [ + "isAlphanumeric", "isAlpha", "isDecimal", "isFloat", "isHexadecimal", "isHexColor", + "isInt", "isNumeric", "isOctal", "isUUID" + ] + ) + } + + override predicate sanitizes(boolean outcome, Expr e) { + outcome = true and + e = getArgument(0).asExpr() + } } From 25065bc986c3f7a4ffc8c98029053809b33833a6 Mon Sep 17 00:00:00 2001 From: luciaromeroML Date: Fri, 17 Sep 2021 18:07:04 -0300 Subject: [PATCH 08/12] simplifying sentence --- javascript/ql/src/experimental/Security/CWE-918/SSRF.qll | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll index 698e1e8f379d..916438c31fc6 100644 --- a/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll +++ b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll @@ -30,10 +30,7 @@ class TernaryOperatorSanitizerGuard extends TaintTracking::SanitizerGuardNode { TaintTracking::SanitizerGuardNode originalGuard; TernaryOperatorSanitizerGuard() { - exists(DataFlow::Node falseNode | - this.getAPredecessor+() = falseNode and - falseNode.asExpr().(BooleanLiteral).mayHaveBooleanValue(false) - ) and + this.getAPredecessor+().asExpr().(BooleanLiteral).mayHaveBooleanValue(false) and this.getAPredecessor+() = originalGuard and not this.asExpr() instanceof LogicalBinaryExpr } From f348a5ce47be97ba77eb77423147b867042c1910 Mon Sep 17 00:00:00 2001 From: luciaromeroML Date: Fri, 17 Sep 2021 18:25:14 -0300 Subject: [PATCH 09/12] adding comments to some functions --- .../experimental/Security/CWE-918/SSRF.qll | 77 ++++++++++++------- 1 file changed, 50 insertions(+), 27 deletions(-) diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll index 916438c31fc6..5488d60719af 100644 --- a/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll +++ b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll @@ -25,33 +25,17 @@ class Configuration extends TaintTracking::Configuration { } } -/** TODO add comment */ -class TernaryOperatorSanitizerGuard extends TaintTracking::SanitizerGuardNode { - TaintTracking::SanitizerGuardNode originalGuard; - - TernaryOperatorSanitizerGuard() { - this.getAPredecessor+().asExpr().(BooleanLiteral).mayHaveBooleanValue(false) and - this.getAPredecessor+() = originalGuard and - not this.asExpr() instanceof LogicalBinaryExpr - } - - override predicate sanitizes(boolean outcome, Expr e) { - not this.asExpr() instanceof LogNotExpr and - originalGuard.sanitizes(outcome, e) - or - exists(boolean originalOutcome | - this.asExpr() instanceof LogNotExpr and - originalGuard.sanitizes(originalOutcome, e) and - ( - originalOutcome = true and outcome = false - or - originalOutcome = false and outcome = true - ) - ) - } -} - -/** TODO add comment */ +/** + * This sanitizers models the next example: + * let valid = req.params.id ? Number.isInteger(req.params.id) : false + * if (valid) { sink(req.params.id) } + * + * This sanitizer models this way of using ternary operators, + * when the sanitizer guard is used as any of the branches + * instead of being used as the condition. + * + * This sanitizer sanitize the corresponding if statement branch. + */ class TernaryOperatorSanitizer extends RequestForgery::Sanitizer { TernaryOperatorSanitizer() { exists( @@ -80,6 +64,45 @@ class TernaryOperatorSanitizer extends RequestForgery::Sanitizer { } } +/** + * This sanitizer guard is another way of modeling the example from above + * In this case: + * let valid = req.params.id ? Number.isInteger(req.params.id) : false + * if (!valid) { return } + * sink(req.params.id) + * + * The previous sanitizer is not enough, + * because we are sanitizing the entire if statement branch + * but we need to sanitize the use of this variable from now on. + * + * Thats why we model this sanitizer guard which says that + * the result of the ternary operator execution is a sanitizer guard. + */ +class TernaryOperatorSanitizerGuard extends TaintTracking::SanitizerGuardNode { + TaintTracking::SanitizerGuardNode originalGuard; + + TernaryOperatorSanitizerGuard() { + this.getAPredecessor+().asExpr().(BooleanLiteral).mayHaveBooleanValue(false) and + this.getAPredecessor+() = originalGuard and + not this.asExpr() instanceof LogicalBinaryExpr + } + + override predicate sanitizes(boolean outcome, Expr e) { + not this.asExpr() instanceof LogNotExpr and + originalGuard.sanitizes(outcome, e) + or + exists(boolean originalOutcome | + this.asExpr() instanceof LogNotExpr and + originalGuard.sanitizes(originalOutcome, e) and + ( + originalOutcome = true and outcome = false + or + originalOutcome = false and outcome = true + ) + ) + } +} + /** * Number.isInteger is a sanitizer guard because a number can't be used to exploit a SSRF. */ From 1f2618b893e67feca17dc31742aa60e37a7c808c Mon Sep 17 00:00:00 2001 From: luciaromeroML Date: Mon, 27 Sep 2021 17:37:11 -0300 Subject: [PATCH 10/12] new test case for unknown base url --- .../ql/test/experimental/Security/CWE-918/SSRF.expected | 9 +++++++++ .../ql/test/experimental/Security/CWE-918/check-path.js | 3 +++ 2 files changed, 12 insertions(+) diff --git a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected index 5e1ffe1e982a..a7bfb5d1f6c0 100644 --- a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected +++ b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected @@ -37,6 +37,10 @@ nodes | check-path.js:37:15:37:45 | 'test.c ... tainted | | check-path.js:37:29:37:45 | req.query.tainted | | check-path.js:37:29:37:45 | req.query.tainted | +| check-path.js:45:13:45:44 | `${base ... inted}` | +| check-path.js:45:13:45:44 | `${base ... inted}` | +| check-path.js:45:26:45:42 | req.query.tainted | +| check-path.js:45:26:45:42 | req.query.tainted | | check-regex.js:24:15:24:42 | baseURL ... tainted | | check-regex.js:24:15:24:42 | baseURL ... tainted | | check-regex.js:24:25:24:42 | req.params.tainted | @@ -113,6 +117,10 @@ edges | check-path.js:37:29:37:45 | req.query.tainted | check-path.js:37:15:37:45 | 'test.c ... tainted | | check-path.js:37:29:37:45 | req.query.tainted | check-path.js:37:15:37:45 | 'test.c ... tainted | | check-path.js:37:29:37:45 | req.query.tainted | check-path.js:37:15:37:45 | 'test.c ... tainted | +| check-path.js:45:26:45:42 | req.query.tainted | check-path.js:45:13:45:44 | `${base ... inted}` | +| check-path.js:45:26:45:42 | req.query.tainted | check-path.js:45:13:45:44 | `${base ... inted}` | +| check-path.js:45:26:45:42 | req.query.tainted | check-path.js:45:13:45:44 | `${base ... inted}` | +| check-path.js:45:26:45:42 | req.query.tainted | check-path.js:45:13:45:44 | `${base ... inted}` | | check-regex.js:24:25:24:42 | req.params.tainted | check-regex.js:24:15:24:42 | baseURL ... tainted | | check-regex.js:24:25:24:42 | req.params.tainted | check-regex.js:24:15:24:42 | baseURL ... tainted | | check-regex.js:24:25:24:42 | req.params.tainted | check-regex.js:24:15:24:42 | baseURL ... tainted | @@ -164,6 +172,7 @@ edges | check-path.js:24:13:24:65 | `/addre ... nted)}` | check-path.js:24:46:24:62 | req.query.tainted | check-path.js:24:13:24:65 | `/addre ... nted)}` | The URL of this request depends on a user-provided value | | check-path.js:33:15:33:45 | 'test.c ... tainted | check-path.js:33:29:33:45 | req.query.tainted | check-path.js:33:15:33:45 | 'test.c ... tainted | The URL of this request depends on a user-provided value | | check-path.js:37:15:37:45 | 'test.c ... tainted | check-path.js:37:29:37:45 | req.query.tainted | check-path.js:37:15:37:45 | 'test.c ... tainted | The URL of this request depends on a user-provided value | +| check-path.js:45:13:45:44 | `${base ... inted}` | check-path.js:45:26:45:42 | req.query.tainted | check-path.js:45:13:45:44 | `${base ... inted}` | The URL of this request depends on a user-provided value | | check-regex.js:24:15:24:42 | baseURL ... tainted | check-regex.js:24:25:24:42 | req.params.tainted | check-regex.js:24:15:24:42 | baseURL ... tainted | The URL of this request depends on a user-provided value | | check-regex.js:31:15:31:45 | "test.c ... tainted | check-regex.js:31:29:31:45 | req.query.tainted | check-regex.js:31:15:31:45 | "test.c ... tainted | The URL of this request depends on a user-provided value | | check-regex.js:34:15:34:42 | baseURL ... tainted | check-regex.js:34:25:34:42 | req.params.tainted | check-regex.js:34:15:34:42 | baseURL ... tainted | The URL of this request depends on a user-provided value | diff --git a/javascript/ql/test/experimental/Security/CWE-918/check-path.js b/javascript/ql/test/experimental/Security/CWE-918/check-path.js index 35681d58491b..6ed1ab1fb7b7 100644 --- a/javascript/ql/test/experimental/Security/CWE-918/check-path.js +++ b/javascript/ql/test/experimental/Security/CWE-918/check-path.js @@ -41,6 +41,9 @@ app.get('/check-with-axios', req => { axios.get('test.com/' + req.query.tainted) // OK } + let baseURL = require('config').base + axios.get(`${baseURL}${req.query.tainted}`); // SSRF + if(!isValidInput(req.query.tainted)) { return; } From 1fc58e51a3838496a44fae3706530a5f7bbb62f1 Mon Sep 17 00:00:00 2001 From: luciaromeroML Date: Mon, 27 Sep 2021 17:37:36 -0300 Subject: [PATCH 11/12] adding suggestion that removes sanitizer for unknown base urls --- .../src/experimental/Security/CWE-918/SSRF.qll | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll index 5488d60719af..3bdca588c98b 100644 --- a/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll +++ b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll @@ -14,8 +14,22 @@ class Configuration extends TaintTracking::Configuration { node instanceof RequestForgery::Sanitizer } + private predicate hasSanitizingSubstring(DataFlow::Node nd) { + nd.getStringValue().regexpMatch(".*[?#].*") + or + hasSanitizingSubstring(StringConcatenation::getAnOperand(nd)) + or + hasSanitizingSubstring(nd.getAPredecessor()) + } + + private predicate strictSanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) { + exists(DataFlow::Node operator, int n | + StringConcatenation::taintStep(source, sink, operator, n) and + hasSanitizingSubstring(StringConcatenation::getOperand(operator, [0 .. n - 1])) + ) + } override predicate isSanitizerEdge(DataFlow::Node source, DataFlow::Node sink) { - sanitizingPrefixEdge(source, sink) + strictSanitizingPrefixEdge(source, sink) } override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) { From e50938588e35c43138d71b98d9b4b765215f2840 Mon Sep 17 00:00:00 2001 From: luciaromeroML Date: Wed, 3 Nov 2021 10:30:35 -0300 Subject: [PATCH 12/12] formatting qll file --- javascript/ql/src/experimental/Security/CWE-918/SSRF.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll index 3bdca588c98b..cba9b1f51543 100644 --- a/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll +++ b/javascript/ql/src/experimental/Security/CWE-918/SSRF.qll @@ -21,13 +21,14 @@ class Configuration extends TaintTracking::Configuration { or hasSanitizingSubstring(nd.getAPredecessor()) } - + private predicate strictSanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) { exists(DataFlow::Node operator, int n | StringConcatenation::taintStep(source, sink, operator, n) and hasSanitizingSubstring(StringConcatenation::getOperand(operator, [0 .. n - 1])) ) } + override predicate isSanitizerEdge(DataFlow::Node source, DataFlow::Node sink) { strictSanitizingPrefixEdge(source, sink) }