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

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>

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.getAPredecessor() = dirname()
+  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() = 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)
 }
 

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')));

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'));

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>

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

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

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

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