Merge branch 'js-team-sprint' into priv-file-polish

Author: erik-krogh

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

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

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 |

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