From 5e31f3a34e9eabc31faaa90475bb745f1cfedae7 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Thu, 18 Jun 2020 09:03:29 +0200 Subject: [PATCH 1/5] JS: polish js/disabling-certificate-validation --- change-notes/1.25/analysis-javascript.md | 1 + javascript/config/suites/javascript/security | 1 + .../DisablingCertificateValidation.qhelp | 49 +++++++++++++++++++ .../CWE-295/DisablingCertificateValidation.ql | 30 +++++++----- .../DisablingCertificateValidation.js | 14 ++++++ 5 files changed, 82 insertions(+), 13 deletions(-) create mode 100644 javascript/ql/src/Security/CWE-295/examples/DisablingCertificateValidation.js diff --git a/change-notes/1.25/analysis-javascript.md b/change-notes/1.25/analysis-javascript.md index 24a1d1274d35..5e900920ecf5 100644 --- a/change-notes/1.25/analysis-javascript.md +++ b/change-notes/1.25/analysis-javascript.md @@ -37,6 +37,7 @@ | Unsafe expansion of self-closing HTML tag (`js/unsafe-html-expansion`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities caused by unsafe expansion of self-closing HTML tags. | | Unsafe shell command constructed from library input (`js/shell-command-constructed-from-input`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights potential command injections due to a shell command being constructed from library inputs. Results are shown on LGTM by default. | | Improper code sanitization (`js/bad-code-sanitization`) | security, external/cwe/cwe-094, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights string concatenation where code is constructed without proper sanitization. Results are shown on LGTM by default. | +| Disabling certificate validation (`js/disabling-certificate-validation`) | security, external/cwe-295 | Highligts locations where SSL certificate validation is disabled . Results are shown on LGTM by default. | ## Changes to existing queries diff --git a/javascript/config/suites/javascript/security b/javascript/config/suites/javascript/security index 5eb02bc148b6..2eb58549373c 100644 --- a/javascript/config/suites/javascript/security +++ b/javascript/config/suites/javascript/security @@ -24,6 +24,7 @@ + semmlecode-javascript-queries/Security/CWE-134/TaintedFormatString.ql: /Security/CWE/CWE-134 + semmlecode-javascript-queries/Security/CWE-201/PostMessageStar.ql: /Security/CWE/CWE-201 + semmlecode-javascript-queries/Security/CWE-209/StackTraceExposure.ql: /Security/CWE/CWE-209 ++ semmlecode-javascript-queries/Security/CWE-295/DisablingCertificateValidation.ql: /Security/CWE/CWE-295 + semmlecode-javascript-queries/Security/CWE-312/CleartextStorage.ql: /Security/CWE/CWE-312 + semmlecode-javascript-queries/Security/CWE-312/CleartextLogging.ql: /Security/CWE/CWE-312 + semmlecode-javascript-queries/Security/CWE-313/PasswordInConfigurationFile.ql: /Security/CWE/CWE-313 diff --git a/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp b/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp index c3258c4e5f1e..b5be132c8e3e 100644 --- a/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp +++ b/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp @@ -5,18 +5,67 @@ +

+ + Certificate validation is the standard authentication + method of a secure TLS connection. Without it, there is no guarantee + about who the other party of a TLS connection is. + +

+ +

+ + When testing software that uses TLS connections, it may be useful to + disable the certificate validation temporarily. But disabling it in + production environments is strongly discouraged, unless an alternative + method of authentication is used. + +

+ +

+ + Do not disable certificate validation for TLS connections. + +

+ +

+ + The following example shows a HTTPS connection that + transfers confidential information to a remote server. But the + connection is not secure since the rejectUnauthorized + option of the connection is set to false. As a + consequence, anyone can impersonate the remote server, and receive the + confidential information. + +

+ + + +

+ + To make the connection secure, the + rejectUnauthorized option should have its default value, + or be set explicitly to true. + +

+ +
  • Wikipedia: Transport Layer Security + (TLS)
    • + +
  • Node.js: TLS (SSL)
    • +     diff --git a/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql b/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql index 707dfc872a6b..6855ec2519db 100644 --- a/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql +++ b/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql @@ -11,19 +11,11 @@ import javascript -from DataFlow::PropWrite disable -where - exists(DataFlow::SourceNode env | - env = NodeJSLib::process().getAPropertyRead("env") and - disable = env.getAPropertyWrite("NODE_TLS_REJECT_UNAUTHORIZED") and - disable.getRhs().mayHaveStringValue("0") - ) - or - exists(DataFlow::ObjectLiteralNode options, DataFlow::InvokeNode invk | - options.flowsTo(invk.getAnArgument()) and - disable = options.getAPropertyWrite("rejectUnauthorized") and - disable.getRhs().(AnalyzedNode).getTheBooleanValue() = false - | +/** + * Gets an options object for a TLS connection. + */ +DataFlow::ObjectLiteralNode tlsOptions() { + exists(DataFlow::InvokeNode invk | result.flowsTo(invk.getAnArgument()) | invk instanceof NodeJSLib::NodeJSClientRequest or invk = DataFlow::moduleMember("https", "Agent").getAnInstantiation() @@ -37,4 +29,16 @@ where or invk = DataFlow::moduleMember("tls", ["connect", "createServer"]).getACall() ) +} + +from DataFlow::PropWrite disable +where + exists(DataFlow::SourceNode env | + env = NodeJSLib::process().getAPropertyRead("env") and + disable = env.getAPropertyWrite("NODE_TLS_REJECT_UNAUTHORIZED") and + disable.getRhs().mayHaveStringValue("0") + ) + or + disable = tlsOptions().getAPropertyWrite("rejectUnauthorized") and + disable.getRhs().(AnalyzedNode).getTheBooleanValue() = false select disable, "Disabling certificate validation is strongly discouraged." diff --git a/javascript/ql/src/Security/CWE-295/examples/DisablingCertificateValidation.js b/javascript/ql/src/Security/CWE-295/examples/DisablingCertificateValidation.js new file mode 100644 index 000000000000..800b5c43a44a --- /dev/null +++ b/javascript/ql/src/Security/CWE-295/examples/DisablingCertificateValidation.js @@ -0,0 +1,14 @@ +let https = require("https"); + +https.request( + { + hostname: "secure.my-online-bank.com", + port: 443, + method: "POST", + path: "send-confidential-information", + rejectUnauthorized: false // BAD + }, + response => { + // ... communicate with secure.my-online-bank.com + } +); From 44aa182d0d76436582c1d3025bf183761ff8fce4 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Thu, 18 Jun 2020 10:14:16 +0200 Subject: [PATCH 2/5] Update change-notes/1.25/analysis-javascript.md Co-authored-by: Asger F --- change-notes/1.25/analysis-javascript.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change-notes/1.25/analysis-javascript.md b/change-notes/1.25/analysis-javascript.md index 5e900920ecf5..5e3368484e4a 100644 --- a/change-notes/1.25/analysis-javascript.md +++ b/change-notes/1.25/analysis-javascript.md @@ -37,7 +37,7 @@ | Unsafe expansion of self-closing HTML tag (`js/unsafe-html-expansion`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities caused by unsafe expansion of self-closing HTML tags. | | Unsafe shell command constructed from library input (`js/shell-command-constructed-from-input`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights potential command injections due to a shell command being constructed from library inputs. Results are shown on LGTM by default. | | Improper code sanitization (`js/bad-code-sanitization`) | security, external/cwe/cwe-094, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights string concatenation where code is constructed without proper sanitization. Results are shown on LGTM by default. | -| Disabling certificate validation (`js/disabling-certificate-validation`) | security, external/cwe-295 | Highligts locations where SSL certificate validation is disabled . Results are shown on LGTM by default. | +| Disabling certificate validation (`js/disabling-certificate-validation`) | security, external/cwe-295 | Highlights locations where SSL certificate validation is disabled . Results are shown on LGTM by default. | ## Changes to existing queries From 457588e89376edba37a33cfaa4bbee28316651f5 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Fri, 19 Jun 2020 11:56:46 +0200 Subject: [PATCH 3/5] JS: mention MITM --- .../CWE-295/DisablingCertificateValidation.qhelp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp b/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp index b5be132c8e3e..8e7e1e0b6e45 100644 --- a/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp +++ b/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp @@ -7,9 +7,10 @@

    - Certificate validation is the standard authentication - method of a secure TLS connection. Without it, there is no guarantee - about who the other party of a TLS connection is. + Certificate validation is the standard authentication method of a + secure TLS connection. Without it, there is no guarantee about who the + other party of a TLS connection is, enabling man-in-the-middle + attacks.

    @@ -61,8 +62,9 @@ -
  • Wikipedia: Transport Layer Security - (TLS)
    • +
  • Wikipedia: Transport Layer Security (TLS)
    • + +
  • Wikipedia: Man-in-the-middle attack
  • Node.js: TLS (SSL)
    • From 3e898487e8d13e1444e3e4e58a03ed910dd54dca Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Mon, 22 Jun 2020 11:23:40 +0200 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com> --- change-notes/1.25/analysis-javascript.md | 2 +- .../src/Security/CWE-295/DisablingCertificateValidation.qhelp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/change-notes/1.25/analysis-javascript.md b/change-notes/1.25/analysis-javascript.md index 5e3368484e4a..559d5c676458 100644 --- a/change-notes/1.25/analysis-javascript.md +++ b/change-notes/1.25/analysis-javascript.md @@ -37,7 +37,7 @@ | Unsafe expansion of self-closing HTML tag (`js/unsafe-html-expansion`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities caused by unsafe expansion of self-closing HTML tags. | | Unsafe shell command constructed from library input (`js/shell-command-constructed-from-input`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights potential command injections due to a shell command being constructed from library inputs. Results are shown on LGTM by default. | | Improper code sanitization (`js/bad-code-sanitization`) | security, external/cwe/cwe-094, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights string concatenation where code is constructed without proper sanitization. Results are shown on LGTM by default. | -| Disabling certificate validation (`js/disabling-certificate-validation`) | security, external/cwe-295 | Highlights locations where SSL certificate validation is disabled . Results are shown on LGTM by default. | +| Disabling certificate validation (`js/disabling-certificate-validation`) | security, external/cwe-295 | Highlights locations where SSL certificate validation is disabled. Results are shown on LGTM by default. | ## Changes to existing queries diff --git a/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp b/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp index 8e7e1e0b6e45..fc6ac2520b44 100644 --- a/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp +++ b/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp @@ -54,7 +54,7 @@ To make the connection secure, the rejectUnauthorized option should have its default value, - or be set explicitly to true. + or be explicitly set to true.

    From f1dad0d6e0aa1e1e9bb7ed51ca7d73384e397a37 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Mon, 22 Jun 2020 11:24:33 +0200 Subject: [PATCH 5/5] Update DisablingCertificateValidation.qhelp --- .../src/Security/CWE-295/DisablingCertificateValidation.qhelp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp b/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp index fc6ac2520b44..103a51ddc45f 100644 --- a/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp +++ b/javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp @@ -8,9 +8,7 @@

    Certificate validation is the standard authentication method of a - secure TLS connection. Without it, there is no guarantee about who the - other party of a TLS connection is, enabling man-in-the-middle - attacks. + secure TLS connection. Without it, there is no guarantee about who the other party of a TLS connection is, making man-in-the-middle attacks more likely to occur