JS: polish js/disabling-certificate-validation

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
 

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

javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.qhelp

@@ -5,18 +5,67 @@
 
 	<overview>
 
+		<p>
+
+			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.
+
+		</p>
+
+		<p>
+
+			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.
+
+		</p>
+
 	</overview>
 
 	<recommendation>
 
+		<p>
+
+			Do not disable certificate validation for TLS connections.
+
+		</p>
+
 	</recommendation>
 
 	<example>
 
+		<p>
+
+			The following example shows a HTTPS connection that
+			transfers confidential information to a remote server. But the
+			connection is not secure since the <code>rejectUnauthorized</code>
+			option of the connection is set to <code>false</code>. As a
+			consequence, anyone can impersonate the remote server, and receive the
+			confidential information.
+
+		</p>
+
+		<sample src="examples/DisablingCertificateValidation.js"/>
+
+		<p>
+
+			To make the connection secure, the
+			<code>rejectUnauthorized</code> option should have its default value,
+			or be set explicitly to <code>true</code>.
+
+		</p>
+
 	</example>
 
 	<references>
 
+		<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Transport_Layer_Security">Transport Layer Security
+		(TLS)</a></li>
+
+		<li>Node.js: <a href="https://nodejs.org/api/tls.html">TLS (SSL)</a></li>
+
 	</references>
 
 </qhelp>

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."

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
+  }
+);