add qhelp to js/biased-cryptographic-random

change-notes/1.25/analysis-javascript.md

@@ -35,7 +35,7 @@
 | Incomplete HTML attribute sanitization (`js/incomplete-html-attribute-sanitization`) | security, external/cwe/cwe-20, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities due to incomplete sanitization of HTML meta-characters. Results are shown on LGTM by default. |
 | 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. |
-| Creating biased random numbers from 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. |
+| 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. |
 
 ## Changes to existing queries
 

javascript/ql/src/Security/CWE-327/BadRandomness.qhelp

@@ -4,33 +4,57 @@
 <qhelp>
 	<overview>
 		<p>
-			Placeholder
+			Generating secure random numbers can be an important part of creating
+			a secure software system, and for that purpose there exists secure APIs
+			for creating 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 below example 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, where the random byte is discarded if 
+			the value was 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-fixed.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

javascript/ql/src/Security/CWE-327/examples/bad-random-fixed.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

@@ -15,3 +15,4 @@
 | 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

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