mirror of
https://github.com/github/codeql.git
synced 2026-05-04 13:15:21 +02:00
Merge pull request #3663 from erik-krogh/bad-crypto
JS: Introduce query to detect biased random number generators
This commit is contained in:
Erik Krogh Kristensen
36
javascript/ql/src/Security/CWE-327/BadRandomness.qhelp
Normal file
36
javascript/ql/src/Security/CWE-327/BadRandomness.qhelp
Normal file
@@ -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>
|
||||
195
javascript/ql/src/Security/CWE-327/BadRandomness.ql
Normal file
195
javascript/ql/src/Security/CWE-327/BadRandomness.ql
Normal file
@@ -0,0 +1,195 @@
|
||||
/**
|
||||
* @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.internal.StepSummary
|
||||
private import semmle.javascript.security.dataflow.InsecureRandomnessCustomizations
|
||||
private import semmle.javascript.dataflow.InferredTypes
|
||||
|
||||
/**
|
||||
* Gets a number that is a power of 2.
|
||||
*/
|
||||
private int powerOfTwo() {
|
||||
result = 1
|
||||
or
|
||||
result = 2 * powerOfTwo() and
|
||||
not result < 0
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets a node that has value 2^n for some n.
|
||||
*/
|
||||
private DataFlow::Node isPowerOfTwo() {
|
||||
exists(DataFlow::Node prev |
|
||||
prev.getIntValue() = powerOfTwo()
|
||||
or
|
||||
// Getting around the 32 bit ints in QL. These are some hex values of the form 0x10000000
|
||||
prev.asExpr().(NumberLiteral).getValue() =
|
||||
["281474976710656", "17592186044416", "1099511627776", "68719476736", "4294967296"]
|
||||
|
|
||||
result = prev.getASuccessor*()
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets a node that has value (2^n)-1 for some n.
|
||||
*/
|
||||
private DataFlow::Node isPowerOfTwoMinusOne() {
|
||||
exists(DataFlow::Node prev |
|
||||
prev.getIntValue() = powerOfTwo() - 1
|
||||
or
|
||||
// Getting around the 32 bit ints in QL. These are some hex values of the form 0xfffffff
|
||||
prev.asExpr().(NumberLiteral).getValue() =
|
||||
["281474976710655", "17592186044415", "1099511627775", "68719476735", "4294967295"]
|
||||
|
|
||||
result = prev.getASuccessor*()
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* 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")
|
||||
or
|
||||
result = DataFlow::moduleImport("secure-random").getACall()
|
||||
or
|
||||
result =
|
||||
DataFlow::moduleImport("secure-random")
|
||||
.getAMethodCall(["randomArray", "randomUint8Array", "randomBuffer"])
|
||||
}
|
||||
|
||||
/**
|
||||
* 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 produced by `source` and type tracked using `t`.
|
||||
*/
|
||||
private DataFlow::Node goodRandom(DataFlow::TypeTracker t, DataFlow::SourceNode source) {
|
||||
t.startInProp(prop()) and
|
||||
result = randomBufferSource() and
|
||||
result = source
|
||||
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 | result = read |
|
||||
read.getBase() = goodRandom(t2, source) and
|
||||
not read.getPropertyNameExpr() instanceof Label
|
||||
)
|
||||
or
|
||||
// reading a number from a Buffer.
|
||||
exists(DataFlow::MethodCallNode call | result = call |
|
||||
call.getReceiver() = goodRandom(t2, source) and
|
||||
call
|
||||
.getMethodName()
|
||||
.regexpMatch("read(BigInt|BigUInt|Double|Float|Int|UInt)(8|16|32|64)?(BE|LE)?")
|
||||
)
|
||||
)
|
||||
or
|
||||
exists(DataFlow::TypeTracker t2 | t = t2.smallstep(goodRandom(t2, source), result))
|
||||
or
|
||||
// re-using the collection steps for `Set`.
|
||||
exists(DataFlow::TypeTracker t2 |
|
||||
result = CollectionsTypeTracking::collectionStep(goodRandom(t2, source), t, t2)
|
||||
)
|
||||
or
|
||||
InsecureRandomness::isAdditionalTaintStep(goodRandom(t.continue(), source), result) and
|
||||
// bit shifts and multiplication by powers of two are generally used for constructing larger numbers from smaller numbers.
|
||||
not exists(BinaryExpr binop | binop = result.asExpr() |
|
||||
binop.getOperator().regexpMatch(".*(<|>).*")
|
||||
or
|
||||
binop.getOperator() = "*" and isPowerOfTwo().asExpr() = binop.getAnOperand()
|
||||
or
|
||||
// string concat does not produce a number
|
||||
unique(InferredType type | type = binop.flow().analyze().getAType()) = TTString()
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets a reference to a cryptographically secure random number produced by `source`.
|
||||
*/
|
||||
DataFlow::Node goodRandom(DataFlow::SourceNode source) {
|
||||
result = goodRandom(DataFlow::TypeTracker::end(), source)
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets a node that that produces a biased result from otherwise cryptographically secure random numbers produced by `source`.
|
||||
*/
|
||||
DataFlow::Node badCrypto(string description, DataFlow::SourceNode source) {
|
||||
// addition and multiplication - always bad when both the lhs and rhs are random.
|
||||
exists(BinaryExpr binop | result.asExpr() = binop |
|
||||
goodRandom(_).asExpr() = binop.getLeftOperand() and
|
||||
goodRandom(_).asExpr() = binop.getRightOperand() and
|
||||
goodRandom(source).asExpr() = binop.getAnOperand() and
|
||||
(
|
||||
binop.getOperator() = "+" and description = "addition"
|
||||
or
|
||||
binop.getOperator() = "*" and description = "multiplication"
|
||||
)
|
||||
)
|
||||
or
|
||||
// division - bad if result is rounded.
|
||||
exists(DivExpr div | result.asExpr() = div |
|
||||
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
|
||||
)
|
||||
or
|
||||
// modulo - only bad if not by a power of 2 - and the result is not checked for bias
|
||||
exists(ModExpr mod, DataFlow::Node random | result.asExpr() = mod and mod.getOperator() = "%" |
|
||||
description = "modulo" and
|
||||
goodRandom(source) = random and
|
||||
random.asExpr() = mod.getLeftOperand() and
|
||||
// division by a power of 2 is OK. E.g. if `x` is uniformly random is in the range [0..255] then `x % 32` is uniformly random in the range [0..31].
|
||||
not mod.getRightOperand() = isPowerOfTwo().asExpr() and
|
||||
// not exists a comparison that checks if the result is potentially biased.
|
||||
not exists(BinaryExpr comparison | comparison.getOperator() = [">", "<", "<=", ">="] |
|
||||
AccessPath::getAnAliasedSourceNode(random.getALocalSource())
|
||||
.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(source) = root.getALeaf() and
|
||||
exists(root.getALeaf().getStringValue()) and
|
||||
description = "string concatenation"
|
||||
)
|
||||
}
|
||||
|
||||
from DataFlow::Node node, string description, DataFlow::SourceNode source
|
||||
where node = badCrypto(description, source)
|
||||
select node, "Using " + description + " on a $@ produces biased results.", source,
|
||||
"cryptographically secure random number"
|
||||
@@ -36,16 +36,7 @@ module InsecureRandomness {
|
||||
}
|
||||
|
||||
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
|
||||
// Assume that all operations on tainted values preserve taint: crypto is hard
|
||||
succ.asExpr().(BinaryExpr).getAnOperand() = pred.asExpr()
|
||||
or
|
||||
succ.asExpr().(UnaryExpr).getOperand() = pred.asExpr()
|
||||
or
|
||||
exists(DataFlow::MethodCallNode mc |
|
||||
mc = DataFlow::globalVarRef("Math").getAMemberCall(_) and
|
||||
pred = mc.getAnArgument() and
|
||||
succ = mc
|
||||
)
|
||||
InsecureRandomness::isAdditionalTaintStep(pred, succ)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -78,4 +78,20 @@ module InsecureRandomness {
|
||||
class CryptoKeySink extends Sink {
|
||||
CryptoKeySink() { this instanceof CryptographicKey }
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if the step `pred` -> `succ` is an additional taint-step for random values that are not cryptographically secure.
|
||||
*/
|
||||
predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
|
||||
// Assume that all operations on tainted values preserve taint: crypto is hard
|
||||
succ.asExpr().(BinaryExpr).getAnOperand() = pred.asExpr()
|
||||
or
|
||||
succ.asExpr().(UnaryExpr).getOperand() = pred.asExpr()
|
||||
or
|
||||
exists(DataFlow::MethodCallNode mc |
|
||||
mc = DataFlow::globalVarRef("Math").getAMemberCall(_) and
|
||||
pred = mc.getAnArgument() and
|
||||
succ = mc
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,15 @@
|
||||
| bad-random.js:3:11:3:61 | crypto. ... s(1)[0] | Using addition on a $@ produces biased results. | bad-random.js:3:11:3:31 | crypto. ... ytes(1) | cryptographically secure random number |
|
||||
| bad-random.js:3:11:3:61 | crypto. ... s(1)[0] | Using addition on a $@ produces biased results. | bad-random.js:3:38:3:58 | crypto. ... ytes(1) | cryptographically secure random number |
|
||||
| bad-random.js:4:11:4:61 | crypto. ... s(1)[0] | Using multiplication on a $@ produces biased results. | bad-random.js:4:11:4:31 | crypto. ... ytes(1) | cryptographically secure random number |
|
||||
| bad-random.js:4:11:4:61 | crypto. ... s(1)[0] | Using multiplication on a $@ produces biased results. | bad-random.js:4:38:4:58 | crypto. ... ytes(1) | cryptographically secure random number |
|
||||
| bad-random.js:9:28:9:43 | buffer[i] / 25.6 | Using division and rounding the result on a $@ produces biased results. | bad-random.js:6:16:6:40 | crypto. ... (bytes) | cryptographically secure random number |
|
||||
| bad-random.js:11:17:11:31 | buffer[i] % 100 | Using modulo on a $@ produces biased results. | bad-random.js:6:16:6:40 | crypto. ... (bytes) | cryptographically secure random number |
|
||||
| bad-random.js:14:11:14:63 | Number( ... (0, 3)) | Using string concatenation on a $@ produces biased results. | bad-random.js:14:25:14:45 | crypto. ... ytes(3) | cryptographically secure random number |
|
||||
| bad-random.js:73:32:73:42 | byte / 25.6 | Using division and rounding the result on a $@ produces biased results. | bad-random.js:70:20:70:44 | crypto. ... (bytes) | cryptographically secure random number |
|
||||
| bad-random.js:75:21:75:30 | byte % 100 | Using modulo on a $@ produces biased results. | bad-random.js:70:20:70:44 | crypto. ... (bytes) | cryptographically secure random number |
|
||||
| bad-random.js:81:11:81:51 | secureR ... (10)[0] | Using addition on a $@ produces biased results. | bad-random.js:81:11:81:26 | secureRandom(10) | cryptographically secure random number |
|
||||
| bad-random.js:81:11:81:51 | secureR ... (10)[0] | Using addition on a $@ produces biased results. | bad-random.js:81:33:81:48 | secureRandom(10) | cryptographically secure random number |
|
||||
| bad-random.js:85:11:85:35 | goodRan ... Random2 | Using addition on a $@ produces biased results. | bad-random.js:83:23:83:38 | secureRandom(10) | cryptographically secure random number |
|
||||
| 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 |
|
||||
@@ -0,0 +1 @@
|
||||
Security/CWE-327/BadRandomness.ql
|
||||
113
javascript/ql/test/query-tests/Security/CWE-327/bad-random.js
Normal file
113
javascript/ql/test/query-tests/Security/CWE-327/bad-random.js
Normal file
@@ -0,0 +1,113 @@
|
||||
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 - input is a random byte, so the output is a uniformly random number between 0 and 7.
|
||||
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
|
||||
}
|
||||
}
|
||||
|
||||
const secureRandom = require("secure-random");
|
||||
|
||||
var bad = secureRandom(10)[0] + secureRandom(10)[0]; // NOT OK
|
||||
|
||||
var goodRandom1 = 5 + secureRandom(10)[0];
|
||||
var goodRandom2 = 5 + secureRandom(10)[0];
|
||||
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 = (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 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.
|
||||
|
||||
var good = (secureRandom(10)[0] + "foo") + (secureRandom(10)[0] + "bar"); // OK - string concat
|
||||
|
||||
var eight = 8;
|
||||
var good = crypto.randomBytes(4)[0] % eight; // OK - modulo by power of 2.
|
||||
|
||||
var twoHundredAndFiftyFive = 0xff;
|
||||
var good = Math.floor(max * (crypto.randomBytes(1)[0] / twoHundredAndFiftyFive)); // OK - division by 0xff (255) gives a uniformly random number between 0 and 1.
|
||||
|
||||
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
|
||||
Reference in New Issue
Block a user