introduce query to detect biased random number generators

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

@@ -0,0 +1,36 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+	<overview>
+		<p>
+			Placeholder
+		</p>
+
+	</overview>
+	<recommendation>
+
+		<p>
+			Placeholder.
+		</p>
+
+	</recommendation>
+	<example>
+
+		<p>
+			Placeholder
+		</p>
+
+	</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>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

@@ -0,0 +1,128 @@
+/**
+ * @name Creating biased random numbers from cryptographically secure source.
+ * @description Some mathematical operations on random numbers can cause bias in
+ *              the results and compromise security.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id js/biased-cryptographic-random
+ * @tags security
+ *       external/cwe/cwe-327
+ */
+
+import javascript
+private import semmle.javascript.dataflow.InferredTypes
+private import semmle.javascript.dataflow.internal.StepSummary
+
+/**
+ * Gets a Buffer/TypedArray containing cryptographically secure random numbers.
+ */
+private DataFlow::SourceNode randomBufferSource() {
+  result = DataFlow::moduleMember("crypto", ["randomBytes", "randomFillSync"]).getACall()
+  or
+  exists(DataFlow::CallNode call |
+    call = DataFlow::moduleMember("crypto", ["randomFill", "randomFillSync"]) and
+    result = call.getArgument(0).getALocalSource()
+  )
+  or
+  result = DataFlow::globalVarRef("crypto").getAMethodCall("getRandomValues")
+}
+
+/**
+ * Gets the pseudo-property used to track elements inside a Buffer.
+ * The API for `Set` is close enough to the API for `Buffer` that we can reuse the type-tracking steps.
+ */
+private string prop() { result = DataFlow::PseudoProperties::setElement() }
+
+/**
+ * Gets a reference to a cryptographically secure random number, type tracked using `t`.
+ */
+private DataFlow::SourceNode goodRandom(DataFlow::TypeTracker t) {
+  t.startInProp(prop()) and
+  result = randomBufferSource()
+  or
+  // Loading a number from a `Buffer`.
+  exists(DataFlow::TypeTracker t2 | t = t2.append(LoadStep(prop())) |
+    // the random generators return arrays/Buffers of random numbers, we therefore track through an indexed read.
+    exists(DataFlow::PropRead read |
+      read.getBase() = goodRandom(t2) and
+      exists(read.getPropertyNameExpr())
+    )
+    or
+    // reading a number from a Buffer.
+    exists(DataFlow::MethodCallNode call |
+      call.getReceiver().getALocalSource() = goodRandom(t.continue()) and
+      call
+          .getMethodName()
+          .regexpMatch("read(BigInt|BigUInt|Double|Float|Int|UInt)(8|16|32|64)?(BE|LE)?")
+    )
+  )
+  or
+  exists(DataFlow::TypeTracker t2 | result = goodRandom(t2).track(t2, t))
+  or
+  // re-using the collection steps for `Set`. 
+  exists(DataFlow::TypeTracker t2 |
+    result = CollectionsTypeTracking::collectionStep(goodRandom(t2), t, t2)
+  )
+}
+
+/**
+ * Gets a reference to a cryptographically random number.
+ */
+DataFlow::SourceNode goodRandom() { result = goodRandom(DataFlow::TypeTracker::end()) }
+
+/**
+ * Gets a node that that produces a biased result from otherwise cryptographically secure random numbers.
+ */
+DataFlow::Node badCrypto(string description) {
+  // addition and multiplication - always bad when both the lhs and rhs are random.
+  exists(BinaryExpr binop | result.asExpr() = binop |
+    goodRandom().flowsToExpr(binop.getLeftOperand()) and
+    goodRandom().flowsToExpr(binop.getRightOperand()) and
+    (
+      binop.getOperator() = "+" and description = "addition"
+      or
+      binop.getOperator() = "*" and description = "multiplication"
+    )
+  )
+  or
+  // division - always bad
+  exists(DivExpr div | result.asExpr() = div |
+    goodRandom().flowsToExpr(div.getLeftOperand()) and
+    description = "division"
+  )
+  or
+  // modulo - only bad if not by a power of 2 - and the result is not checked for bias
+  exists(ModExpr mod, DataFlow::SourceNode random |
+    result.asExpr() = mod and mod.getOperator() = "%"
+  |
+    description = "modulo" and
+    goodRandom() = random and
+    random.flowsToExpr(mod.getLeftOperand()) and
+    not mod.getRightOperand().getIntValue() = [2, 4, 8, 16, 32, 64, 128] and
+    // not exists a comparison that checks if the result is potentially biased.
+    not exists(BinaryExpr comparison | comparison.getOperator() = [">", "<", "<=", ">="] |
+      AccessPath::getAnAliasedSourceNode(random).flowsToExpr(comparison.getAnOperand())
+      or
+      exists(DataFlow::PropRead otherRead |
+        otherRead = random.(DataFlow::PropRead).getBase().getALocalSource().getAPropertyRead() and
+        not exists(otherRead.getPropertyName()) and
+        otherRead.flowsToExpr(comparison.getAnOperand())
+      )
+    )
+  )
+  or
+  // create a number from a string - always a bad idea.
+  exists(DataFlow::CallNode number, StringOps::ConcatenationRoot root | result = number |
+    number = DataFlow::globalVarRef(["Number", "parseInt", "parseFloat"]).getACall() and
+    root = number.getArgument(0) and
+    goodRandom().flowsTo(root.getALeaf()) and
+    exists(root.getALeaf().getStringValue()) and
+    description = "string concatenation"
+  )
+}
+
+from DataFlow::Node node, string description
+where node = badCrypto(description)
+select node,
+  "Using " + description + " on cryptographically random numbers produces biased results."

javascript/ql/test/query-tests/Security/CWE-327/BadRandomness.expected

@@ -0,0 +1,7 @@
+| bad-random.js:3:11:3:61 | crypto. ... s(1)[0] | Using addition on cryptographically random numbers produces biased results. |
+| bad-random.js:4:11:4:61 | crypto. ... s(1)[0] | Using multiplication on cryptographically random numbers produces biased results. |
+| bad-random.js:9:28:9:43 | buffer[i] / 25.6 | Using division on cryptographically random numbers produces biased results. |
+| bad-random.js:11:17:11:31 | buffer[i] % 100 | Using modulo on cryptographically random numbers produces biased results. |
+| bad-random.js:14:11:14:63 | Number( ... (0, 3)) | Using string concatenation on cryptographically random numbers produces biased results. |
+| bad-random.js:73:32:73:42 | byte / 25.6 | Using division on cryptographically random numbers produces biased results. |
+| bad-random.js:75:21:75:30 | byte % 100 | Using modulo on cryptographically random numbers produces biased results. |

javascript/ql/test/query-tests/Security/CWE-327/BadRandomness.qlref

@@ -0,0 +1 @@
+Security/CWE-327/BadRandomness.ql

javascript/ql/test/query-tests/Security/CWE-327/bad-random.js

@@ -0,0 +1,77 @@
+const crypto = require('crypto');
+
+var bad = crypto.randomBytes(1)[0] + crypto.randomBytes(1)[0]; // NOT OK
+var bad = crypto.randomBytes(1)[0] * crypto.randomBytes(1)[0]; // NOT OK
+
+const buffer = crypto.randomBytes(bytes);
+const digits = [];
+for (let i = 0; i < buffer.length; ++i) {
+    digits.push(Math.floor(buffer[i] / 25.6)); // NOT OK
+    digits.push(buffer[i] % 8); // OK - 8 is a power of 2, so the result is unbiased.
+    digits.push(buffer[i] % 100); // NOT OK
+}
+
+var bad = Number('0.' + crypto.randomBytes(3).readUIntBE(0, 3)); // NOT OK
+var good = Number(10 + crypto.randomBytes(3).readUIntBE(0, 3)); // OK
+
+const internals = {};
+exports.randomDigits = function (size) {
+    const digits = [];
+
+    let buffer = internals.random(size * 2);
+    let pos = 0;
+
+    while (digits.length < size) {
+        if (pos >= buffer.length) {
+            buffer = internals.random(size * 2);
+            pos = 0;
+        }
+
+        if (buffer[pos] < 250) {
+            digits.push(buffer[pos] % 10); // GOOD - protected by a bias-checking comparison.
+        }
+        ++pos;
+    }
+
+    return digits.join('');
+};
+
+internals.random = function (bytes) {
+    try {
+        return crypto.randomBytes(bytes);
+    }
+    catch (err) {
+        throw new Error("Failed to make bits.");
+    }
+};
+
+exports.randomDigits2 = function (size) {
+    const digits = [];
+
+    let buffer = crypto.randomBytes(size * 2);
+    let pos = 0;
+
+    while (digits.length < size) {
+        if (pos >= buffer.length) {
+            buffer = internals.random(size * 2);
+            pos = 0;
+        }
+        var num = buffer[pos];
+        if (num < 250) {
+            digits.push(num % 10); // GOOD - protected by a bias-checking comparison.
+        }
+        ++pos;
+    }
+
+    return digits.join('');
+};
+
+function setSteps() {
+    const buffer = crypto.randomBytes(bytes);
+    const digits = [];
+    for (const byte of buffer.values()) {
+        digits.push(Math.floor(byte / 25.6)); // NOT OK
+        digits.push(byte % 8); // OK - 8 is a power of 2, so the result is unbiased.
+        digits.push(byte % 100); // NOT OK
+    }
+}