Merge branch 'js-team-sprint' into https-fix

erik-krogh · web-flow · commit 0f5ef2c02af8 · 2020-06-19T14:57:44.000+02:00
diff --git a/change-notes/1.25/analysis-javascript.md b/change-notes/1.25/analysis-javascript.md
@@ -37,6 +37,8 @@
 | 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. |
 | Download of sensitive file through insecure connection (`js/insecure-download`) | security, external/cwe/cwe-829 | Highlights downloads of sensitive files through an unencrypted protocol. Results are shown on LGTM by default. |
+| Exposure of private files (`js/exposure-of-private-files`) | security, external/cwe/cwe-200 | Highlights servers that serve private files. Results are shown on LGTM by default. |
+| 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. |
 
diff --git a/javascript/ql/src/Security/CWE-200/PrivateFileExposure.qhelp b/javascript/ql/src/Security/CWE-200/PrivateFileExposure.qhelp
@@ -5,20 +5,33 @@
 
 <overview>
   <p>
-    Placeholder
+    Libraries like <code>express</code> provide easy methods for serving entire 
+    directories of static files from a web server.
+    However, using these can sometimes lead to accidental information exposure. 
+    If for example the <code>node_modules</code> folder is served, then an attacker
+    can access the <code>_where</code> field from a <code>package.json</code> file, 
+    which gives access to the absolute path of the file. 
   </p>
 </overview>
 
 <recommendation>
   <p>
-    Placeholder
+    Limit which folders of static files are served from a web server.
   </p>
 </recommendation>
 
 <example>
   <p>
-    Placeholder
+    In the example below, all the files from the <code>node_modules</code> are served. 
+    This allows clients to easily access all the files inside that folder,
+    which includes potentially private information inside <code>package.json</code> files.
   </p>
+  <sample src="examples/PrivateFileExposure.js"/>
+  <p>
+    The issue has been fixed below by only serving specific folders within the
+    <code>node_modules</code> folder.
+  </p>
+  <sample src="examples/PrivateFileExposureFixed.js"/>
 </example>
 
 <references>
diff --git a/javascript/ql/src/Security/CWE-200/PrivateFileExposure.ql b/javascript/ql/src/Security/CWE-200/PrivateFileExposure.ql
@@ -69,19 +69,33 @@ pragma[noinline]
 Folder getAPackageJSONFolder() { result = any(PackageJSON json).getFile().getParentContainer() }
 
 /**
- * Gets a reference to `dirname` that might cause information to be leaked.
- * That can happen if there is a `package.json` file in the same folder.
- * (It is assumed that the presence of a `package.json` file means that a `node_modules` folder can also exist.
+ * Gets a reference to `dirname`, the home folder, the current working folder, or the root folder.
+ * All of these might cause information to be leaked.
+ *
+ * For `dirname` that can happen if there is a `package.json` file in the same folder.
+ * It is assumed that the presence of a `package.json` file means that a `node_modules` folder can also exist.
+ *
+ * For the root/home/working folder, they contain so much information that they must leak information somehow (e.g. ssh keys in the `~/.ssh` folder).
  */
-DataFlow::Node dirname() {
+DataFlow::Node getALeakingFolder(string description) {
   exists(ModuleScope ms | result.asExpr() = ms.getVariable("__dirname").getAnAccess()) and
-  result.getFile().getParentContainer() = getAPackageJSONFolder()
+  result.getFile().getParentContainer() = getAPackageJSONFolder() and
+  description = "the folder " + result.getFile().getParentContainer().getRelativePath()
+  or
+  result = DataFlow::moduleImport("os").getAMemberCall("homedir") and
+  description = "the home folder "
+  or
+  result.mayHaveStringValue("/") and
+  description = "the root folder"
+  or
+  result.getStringValue() = [".", "./"] and
+  description = "the current working folder"
   or
-  result.getAPredecessor() = dirname()
+  result.getAPredecessor() = getALeakingFolder(description)
   or
   exists(StringOps::ConcatenationRoot root | root = result |
     root.getNumOperand() = 2 and
-    root.getOperand(0) = dirname() and
+    root.getOperand(0) = getALeakingFolder(description) and
     root.getOperand(1).getStringValue() = "/"
   )
 }
@@ -94,18 +108,17 @@ DataFlow::Node getAPrivateFolderPath(string description) {
     result = getANodeModulePath(path) and description = "the folder \"" + path + "\""
   )
   or
-  result = dirname() and
-  description = "the folder " + result.getFile().getParentContainer().getRelativePath()
-  or
-  result.getStringValue() = [".", "./"] and
-  description = "the current working folder"
+  result = getALeakingFolder(description)
 }
 
 /**
  * Gest a call that serves the folder `path` to the public.
  */
 DataFlow::CallNode servesAPrivateFolder(string description) {
-  result = DataFlow::moduleMember("express", "static").getACall() and
+  result = DataFlow::moduleMember(["express", "connect"], "static").getACall() and
+  result.getArgument(0) = getAPrivateFolderPath(description)
+  or
+  result = DataFlow::moduleImport("serve-static").getACall() and
   result.getArgument(0) = getAPrivateFolderPath(description)
 }
 
diff --git a/javascript/ql/src/Security/CWE-200/examples/PrivateFileExposure.js b/javascript/ql/src/Security/CWE-200/examples/PrivateFileExposure.js
@@ -0,0 +1,6 @@
+
+var express = require('express');
+
+var app = express();
+
+app.use('/node_modules', express.static(path.resolve(__dirname, '../node_modules')));
diff --git a/javascript/ql/src/Security/CWE-200/examples/PrivateFileExposureFixed.js b/javascript/ql/src/Security/CWE-200/examples/PrivateFileExposureFixed.js
@@ -0,0 +1,7 @@
+
+var express = require('express');
+
+var app = express();
+
+app.use("jquery", express.static('./node_modules/jquery/dist'));
+app.use("bootstrap", express.static('./node_modules/bootstrap/dist'));
diff --git a/javascript/ql/src/Security/CWE-327/BadRandomness.qhelp b/javascript/ql/src/Security/CWE-327/BadRandomness.qhelp
@@ -4,33 +4,62 @@
 <qhelp>
 	<overview>
 		<p>
-			Placeholder
+			Generating secure random numbers can be an important part of creating a
+			secure software system. This can be done using APIs that create 
+			cryptographically secure random numbers.
+		</p>
+		<p>
+			However, using some mathematical operations on these cryptographically 
+			secure random numbers can create biased results, where some outcomes 
+			are more likely than others. 
+			Such biased results can make it easier for an attacker to guess the random
+			numbers, and thereby break the security of the software system. 
 		</p>
-
 	</overview>
 	<recommendation>
-
 		<p>
-			Placeholder.
+			Be very careful not to introduce bias when performing mathematical operations
+			on cryptographically secure random numbers.  
+		</p>
+		<p>
+			If possible, avoid performing mathematical operations on cryptographically secure 
+			random numbers at all, and use a preexisting library instead. 
 		</p>
-
 	</recommendation>
 	<example>
-
 		<p>
-			Placeholder
+			The example below uses the modulo operator to create an array of 10 random digits
+			using random bytes as the source for randomness. 
 		</p>
+		<sample src="examples/bad-random.js" />
+		<p>
+			The random byte is a uniformly random value between 0 and 255, and thus the result 
+			from using the modulo operator is slightly more likely to be between 0 and 5 than 
+			between 6 and 9.
+		</p>
+		<p>
+			The issue has been fixed in the code below by using a library that correctly generates
+			cryptographically secure random values.
+		</p>
+		<sample src="examples/bad-random-fixed.js" />
+		<p>
+			Alternatively, the issue can be fixed by fixing the math in the original code. 
+			In the code below the random byte is discarded if the value is greater than or equal to 250. 
+			Thus the modulo operator is used on a uniformly random number between 0 and 249, which
+			results in a uniformly random digit between 0 and 9.
+		</p>
+		<sample src="examples/bad-random-fixed2.js" />
 
 	</example>
 
+
 	<references>
-		<li>NIST, FIPS 140 Annex a: <a href="http://csrc.nist.gov/publications/fips/fips140-2/fips1402annexa.pdf"> Approved Security Functions</a>.</li>
-		<li>NIST, SP 800-131A: <a href="http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar1.pdf"> Transitions: Recommendation for Transitioning the Use of Cryptographic Algorithms and Key Lengths</a>.</li>
+		<li>Stack Overflow: <a href="https://stackoverflow.com/questions/3956478/understanding-randomness">Understanding “randomness”</a>.</li>
+		<li>OWASP: <a href="https://owasp.org/www-community/vulnerabilities/Insecure_Randomness">Insecure Randomness</a>.</li>
 		<li>OWASP: <a
 		href="https://cheatsheetseries.owasp.org/cheatsheets/Cryptographic_Storage_Cheat_Sheet.html#rule---use-strong-approved-authenticated-encryption">Rule
 		- Use strong approved cryptographic algorithms</a>.
 		</li>
-		<li>Stack Overflow: <a href="https://stackoverflow.com/questions/3956478/understanding-randomness">Understanding “randomness”</a>.</li>
 	</references>
 
 </qhelp>
diff --git a/javascript/ql/src/Security/CWE-327/BadRandomness.ql b/javascript/ql/src/Security/CWE-327/BadRandomness.ql
@@ -1,5 +1,5 @@
 /**
- * @name Creating biased random numbers from cryptographically secure source.
+ * @name Creating biased random numbers from a cryptographically secure source.
  * @description Some mathematical operations on random numbers can cause bias in
  *              the results and compromise security.
  * @kind problem
@@ -132,6 +132,18 @@ DataFlow::Node goodRandom(DataFlow::SourceNode source) {
   result = goodRandom(DataFlow::TypeTracker::end(), source)
 }
 
+/**
+ * Gets a node that is passed to a rounding function from `Math`, using type-backtracker `t`.
+ */
+DataFlow::Node isRounded(DataFlow::TypeBackTracker t) {
+  t.start() and
+  result = DataFlow::globalVarRef("Math").getAMemberCall(["round", "floor", "ceil"]).getArgument(0)
+  or
+  exists(DataFlow::TypeBackTracker t2 | t2 = t.smallstep(result, isRounded(t2)))
+  or
+  InsecureRandomness::isAdditionalTaintStep(result, isRounded(t.continue()))
+}
+
 /**
  * Gets a node that that produces a biased result from otherwise cryptographically secure random numbers produced by `source`.
  */
@@ -153,10 +165,7 @@ DataFlow::Node badCrypto(string description, DataFlow::SourceNode source) {
     goodRandom(source).asExpr() = div.getLeftOperand() and
     description = "division and rounding the result" and
     not div.getRightOperand() = isPowerOfTwoMinusOne().asExpr() and // division by (2^n)-1 most of the time produces a uniformly random number between 0 and 1.
-    DataFlow::globalVarRef("Math")
-        .getAMemberCall(["round", "floor", "ceil"])
-        .getArgument(0)
-        .asExpr() = div
+    div = isRounded(DataFlow::TypeBackTracker::end()).asExpr()
   )
   or
   // modulo - only bad if not by a power of 2 - and the result is not checked for bias
diff --git a/javascript/ql/src/Security/CWE-327/examples/bad-random-fixed.js b/javascript/ql/src/Security/CWE-327/examples/bad-random-fixed.js
@@ -0,0 +1,3 @@
+const cryptoRandomString = require('crypto-random-string');
+
+const digits = cryptoRandomString({length: 10, type: 'numeric'});
diff --git a/javascript/ql/src/Security/CWE-327/examples/bad-random-fixed2.js b/javascript/ql/src/Security/CWE-327/examples/bad-random-fixed2.js
@@ -0,0 +1,10 @@
+const crypto = require('crypto');
+
+const digits = [];
+while (digits.length < 10) {
+    const byte = crypto.randomBytes(1)[0];
+    if (byte >= 250) {
+        continue;
+    }
+    digits.push(byte % 10); // OK
+}
diff --git a/javascript/ql/src/Security/CWE-327/examples/bad-random.js b/javascript/ql/src/Security/CWE-327/examples/bad-random.js
@@ -0,0 +1,6 @@
+const crypto = require('crypto');
+
+const digits = [];
+for (let i = 0; i < 10; i++) {
+    digits.push(crypto.randomBytes(1)[0] % 10); // NOT OK
+}
diff --git a/javascript/ql/test/query-tests/Security/CWE-200/PrivateFileExposure.expected b/javascript/ql/test/query-tests/Security/CWE-200/PrivateFileExposure.expected
@@ -14,3 +14,8 @@
 | private-file-exposure.js:18:1:18:74 | app.use ... les"))) | Serves the folder "/node_modules", which can contain private information. |
 | private-file-exposure.js:19:1:19:88 | app.use ... lar/')) | Serves the folder "/node_modules/angular/", which can contain private information. |
 | private-file-exposure.js:22:1:22:58 | app.use ... lar/')) | Serves the folder "/node_modules/angular/", which can contain private information. |
+| private-file-exposure.js:40:1:40:88 | app.use ... lar/')) | Serves the folder "/node_modules/angular/", which can contain private information. |
+| private-file-exposure.js:41:1:41:97 | app.use ... lar/')) | Serves the folder "/node_modules/angular/", which can contain private information. |
+| private-file-exposure.js:42:1:42:66 | app.use ... dir())) | Serves the home folder , which can contain private information. |
+| private-file-exposure.js:43:1:43:46 | app.use ... )("/")) | Serves the root folder, which can contain private information. |
+| private-file-exposure.js:51:5:51:88 | app.use ... les'))) | Serves the folder "../node_modules", which can contain private information. |
diff --git a/javascript/ql/test/query-tests/Security/CWE-200/private-file-exposure.js b/javascript/ql/test/query-tests/Security/CWE-200/private-file-exposure.js
@@ -35,3 +35,28 @@ app.use('/js/', express.static('node_modules/bootstrap/dist/js'))
 app.use('/css/', express.static('node_modules/font-awesome/css'));
 app.use('basedir', express.static(__dirname)); // GOOD, because there is no package.json in the same folder.
 app.use('/monthly', express.static(__dirname + '/')); // GOOD, because there is no package.json in the same folder.
+
+const connect = require("connect");
+app.use('/angular', connect.static(path.join(__dirname, "/node_modules") + '/angular/')); // NOT OK
+app.use('/angular', require('serve-static')(path.join(__dirname, "/node_modules") + '/angular/')); // NOT OK
+app.use('/home', require('serve-static')(require("os").homedir())); // NOT OK
+app.use('/root', require('serve-static')("/")); // NOT OK
+
+// Bad documentation example
+function bad() {
+    var express = require('express');
+
+    var app = express();
+
+    app.use('/node_modules', express.static(path.resolve(__dirname, '../node_modules'))); // NOT OK
+}
+
+// Good documentation example
+function good() {
+    var express = require('express');
+
+    var app = express();
+
+    app.use("jquery", express.static('./node_modules/jquery/dist')); // OK
+    app.use("bootstrap", express.static('./node_modules/bootstrap/dist')); // OK
+}
diff --git a/javascript/ql/test/query-tests/Security/CWE-327/BadRandomness.expected b/javascript/ql/test/query-tests/Security/CWE-327/BadRandomness.expected
@@ -13,3 +13,6 @@
 | bad-random.js:85:11:85:35 | goodRan ... Random2 | Using addition on a $@ produces biased results. | bad-random.js:84:23:84:38 | secureRandom(10) | cryptographically secure random number |
 | bad-random.js:87:16:87:24 | bad + bad | Using addition on a $@ produces biased results. | bad-random.js:83:23:83:38 | secureRandom(10) | cryptographically secure random number |
 | bad-random.js:87:16:87:24 | bad + bad | Using addition on a $@ produces biased results. | bad-random.js:84:23:84:38 | secureRandom(10) | cryptographically secure random number |
+| bad-random.js:90:29:90:54 | secureR ...  / 25.6 | Using division and rounding the result on a $@ produces biased results. | bad-random.js:90:29:90:44 | secureRandom(10) | cryptographically secure random number |
+| bad-random.js:96:29:96:58 | crypto. ... ] / 100 | Using division and rounding the result on a $@ produces biased results. | bad-random.js:96:29:96:49 | crypto. ... ytes(1) | cryptographically secure random number |
+| bad-random.js:118:17:118:45 | crypto. ... 0] % 10 | Using modulo on a $@ produces biased results. | bad-random.js:118:17:118:37 | crypto. ... ytes(1) | cryptographically secure random number |
diff --git a/javascript/ql/test/query-tests/Security/CWE-327/bad-random.js b/javascript/ql/test/query-tests/Security/CWE-327/bad-random.js
@@ -87,13 +87,13 @@ var bad = goodRandom1 + goodRandom2; // NOT OK
 var dontFlag = bad + bad; // OK - the operands have already been flagged - but flagged anyway due to us not detecting that [INCONSISTENCY].
 
 var good = secureRandom(10)[0] / 0xff; // OK - result is not rounded. 
-var good = Math.ceil(0.5 - (secureRandom(10)[0] / 25.6)); // NOT OK - division generally introduces bias - but not flagged due to not looking through nested arithmetic [INCONSISTENCY]. 
+var good = Math.ceil(0.5 - (secureRandom(10)[0] / 25.6)); // NOT OK - division generally introduces bias - but not flagged due to not looking through nested arithmetic. 
 
 var good = (crypto.randomBytes(1)[0] << 8) + crypto.randomBytes(3)[0]; // OK - bit shifts are usually used to construct larger/smaller numbers, 
 
 var good = Math.floor(max * (crypto.randomBytes(1)[0] / 0xff)); // OK - division by 0xff (255) gives a uniformly random number between 0 and 1. 
 
-var bad = Math.floor(max * (crypto.randomBytes(1)[0] / 100)); // NOT OK - division by 100 gives bias - but not flagged due to not looking through nested arithmetic [INCONSISTENCY]. 
+var bad = Math.floor(max * (crypto.randomBytes(1)[0] / 100)); // NOT OK - division by 100 gives bias - but not flagged due to not looking through nested arithmetic. 
 
 var crb = crypto.randomBytes(4);
 var cryptoRand = 0x01000000 * crb[0] + 0x00010000 * crb[1] + 0x00000100 * crb[2] + 0x00000001 * crb[3]; // OK - producing a larger number from smaller numbers.
@@ -110,4 +110,20 @@ var a = crypto.randomBytes(10);
 var good = ((a[i] & 31) * 0x1000000000000) + (a[i + 1] * 0x10000000000) + (a[i + 2] * 0x100000000) + (a[i + 3] * 0x1000000) + (a[i + 4] << 16) + (a[i + 5] << 8) + a[i + 6]; // OK - generating a large number from smaller bytes.
 var good = (a[i] * 0x100000000) + a[i + 6]; // OK - generating a large number from smaller bytes.
 var good = (a[i + 2] * 0x10000000) + a[i + 6]; // OK - generating a large number from smaller bytes.
-var foo = 0xffffffffffff + 0xfffffffffff + 0xffffffffff + 0xfffffffff + 0xffffffff + 0xfffffff + 0xffffff
+var foo = 0xffffffffffff + 0xfffffffffff + 0xffffffffff + 0xfffffffff + 0xffffffff + 0xfffffff + 0xffffff
+
+// Bad documentation example: 
+const digits = [];
+for (let i = 0; i < 10; i++) {
+    digits.push(crypto.randomBytes(1)[0] % 10); // NOT OK
+}
+
+// Good documentation example: 
+const digits = [];
+while (digits.length < 10) {
+    const byte = crypto.randomBytes(1)[0];
+    if (byte >= 250) {
+        continue;
+    }
+    digits.push(byte % 10); // OK
+}

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+const cryptoRandomString = require('crypto-random-string');`
	`2`	`+`
	`3`	`+const digits = cryptoRandomString({length: 10, type: 'numeric'});`