Merge pull request github#3735 from esbena/js/insecure-http-options

JS: polish js/disabling-certificate-validation

Author: asgerf

change-notes/1.25/analysis-javascript.md

@@ -41,6 +41,7 @@
 | Creating biased random numbers from a cryptographically secure source (`js/biased-cryptographic-random`) | security, external/cwe/cwe-327 | Highlights mathematical operations on cryptographically secure numbers that can create biased results. Results are shown on LGTM by default. |
 | Storage of sensitive information in build artifact (`js/build-artifact-leak`) | security, external/cwe/cwe-312 | Highlights storage of sensitive information in build artifacts. 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. | 
 | Incomplete multi-character sanitization (`js/incomplete-multi-character-sanitization`) | correctness, security, external/cwe/cwe-20, external/cwe/cwe-116 | Highlights sanitizers that fail to remove dangerous substrings completely. Results are shown on LGTM by default. |
 
 ## Changes to existing queries

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, making man-in-the-middle attacks more likely to occur
+
+		</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 explicitly set 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>Wikipedia: <a href="https://en.wikipedia.org/wiki/Man-in-the-middle_attack">Man-in-the-middle attack</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
+  }
+);