JS: documentation for js/unclosed-stream

Author: esbena

Diff for: 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. |
+| Unclosed stream (`js/unclosed-stream`) | security, external/cwe/cwe-404, external/cwe/cwe-730 | Highlights stream processors that do not close their stream when they should. Results are shown on LGTM by default. | 
 
 ## Changes to existing queries
 

Diff for: javascript/config/suites/javascript/security

@@ -42,6 +42,7 @@
 + semmlecode-javascript-queries/Security/CWE-640/HostHeaderPoisoningInEmailGeneration.ql: /Security/CWE/CWE-640
 + semmlecode-javascript-queries/Security/CWE-643/XpathInjection.ql: /Security/CWE/CWE-643
 + semmlecode-javascript-queries/Security/CWE-730/RegExpInjection.ql: /Security/CWE/CWE-730
++ semmlecode-javascript-queries/Security/CWE-730/UnclosedStream.ql: /Security/CWE/CWE-730
 + semmlecode-javascript-queries/Security/CWE-754/UnvalidatedDynamicMethodCall.ql: /Security/CWE/CWE-754
 + semmlecode-javascript-queries/Security/CWE-770/MissingRateLimiting.ql: /Security/CWE/CWE-770
 + semmlecode-javascript-queries/Security/CWE-776/XmlBomb.ql: /Security/CWE/CWE-776

Diff for: javascript/ql/src/Security/CWE-730/UnclosedStream.qhelp

@@ -1,22 +1,69 @@
 <!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
 <qhelp>
 
-	<overview>
+<overview>
 
-	</overview>
+	<p>
 
-	<recommendation>
+		Callback-based input streams generate calls until they are empty or
+		closed explicitly, and they will take up system resources until
+		that point in time.
 
-	</recommendation>
+	</p>
 
-	<example>
+	<p>
 
-	</example>
+		Forgetting to close a stream that no longer is useful can cause
+		unnecessary exhaustion of system resources, which may cause the
+		application to be unresponsive or even crash.
 
-	<references>
+	</p>
 
-	</references>
+</overview>
+
+<recommendation>
+
+	<p>
+
+		Close input streams explicitly when there is no interest in their
+		remaining content.
+
+	</p>
+
+</recommendation>
+
+<example>
+
+	<p>
+
+In the following example, an array stores the content of a stream
+until the array becomes too big. When that happens, an error message
+is generated, and the stream processing is supposed to end.
+
+	</p>
+
+	<sample src="examples/UnclosedStream.js"/>
+
+	<p>
+
+The stream will however keep invoking the callback until it becomes
+empty, regardless of the programmers intention to not process
+additional content. This means that the array will grow indefinitely, if a malicious actor keep sending data.
+
+Close the stream to remedy this:
+
+	</p>
+
+	<sample src="examples/UnclosedStream_fixed.js"/>
+
+</example>
+
+<references>
+
+	<li>Node.js: <a href="https://nodejs.org/api/stream.html">Stream</a></li>
+
+</references>
 
 </qhelp>

Diff for: javascript/ql/src/Security/CWE-730/examples/UnclosedStream.js

@@ -0,0 +1,16 @@
+var https = require("https");
+
+new Promise(function dispatchHttpRequest(resolve, reject) {
+  https.request(options, function(stream) {
+    var responseBuffer = [];
+    stream.on("data", function(chunk) {
+      responseBuffer.push(chunk);
+
+      if (responseBuffer.length > 1024) {
+        reject("Input size of 1024 exceeded."); // BAD
+      }
+    });
+
+    stream.on("end", () => resolve(responseBuffer));
+  });
+});

Diff for: javascript/ql/src/Security/CWE-730/examples/UnclosedStream_fixed.js

@@ -0,0 +1,17 @@
+var https = require("https");
+
+new Promise(function dispatchHttpRequest(resolve, reject) {
+  https.request(options, function(stream) {
+    var responseBuffer = [];
+    stream.on("data", function(chunk) {
+      responseBuffer.push(chunk);
+
+      if (responseBuffer.length > 1024) {
+        stream.destroy();
+        reject("Input size of 1024 exceeded."); // GOOD
+      }
+    });
+
+    stream.on("end", () => resolve(responseBuffer));
+  });
+});