mirror of
https://github.com/github/codeql.git
synced 2026-05-01 03:35:13 +02:00
fixing FPs in js/biased-cryptographic-random
This commit is contained in:
Erik Krogh Kristensen
@@ -11,10 +11,49 @@
|
||||
*/
|
||||
|
||||
import javascript
|
||||
private import semmle.javascript.dataflow.InferredTypes
|
||||
private import semmle.javascript.dataflow.internal.StepSummary
|
||||
private import semmle.javascript.security.dataflow.InsecureRandomnessCustomizations
|
||||
|
||||
/**
|
||||
* 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.
|
||||
*/
|
||||
@@ -73,11 +112,23 @@ private DataFlow::Node goodRandom(DataFlow::TypeTracker t, DataFlow::SourceNode
|
||||
result = CollectionsTypeTracking::collectionStep(goodRandom(t2, source), t, t2)
|
||||
)
|
||||
or
|
||||
InsecureRandomness::isAdditionalTaintStep(goodRandom(t.continue(), source), result)
|
||||
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
|
||||
binop.getAnOperand().(NumberLiteral).getValue().regexpMatch("0x0*10*")
|
||||
)
|
||||
or
|
||||
binop.getOperator() = "+" and exists(binop.getAnOperand().getStringValue()) // string concat does not produce a number
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets a reference to a cryptographically random number produced by `source`.
|
||||
* Gets a reference to a cryptographically secure random number produced by `source`.
|
||||
*/
|
||||
DataFlow::Node goodRandom(DataFlow::SourceNode source) {
|
||||
result = goodRandom(DataFlow::TypeTracker::end(), source)
|
||||
@@ -99,10 +150,13 @@ DataFlow::Node badCrypto(string description, DataFlow::SourceNode source) {
|
||||
)
|
||||
)
|
||||
or
|
||||
// division - always bad
|
||||
// division - bad if result is rounded.
|
||||
exists(DivExpr div | result.asExpr() = div |
|
||||
goodRandom(source).asExpr() = div.getLeftOperand() and
|
||||
description = "division"
|
||||
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.
|
||||
div.getParentExpr+() =
|
||||
DataFlow::globalVarRef("Math").getAMemberCall(["round", "floor", "ceil"]).asExpr()
|
||||
)
|
||||
or
|
||||
// modulo - only bad if not by a power of 2 - and the result is not checked for bias
|
||||
@@ -111,7 +165,7 @@ DataFlow::Node badCrypto(string description, DataFlow::SourceNode source) {
|
||||
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().getIntValue() = [2, 4, 8, 16, 32, 64, 128] and
|
||||
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())
|
||||
@@ -138,4 +192,4 @@ DataFlow::Node badCrypto(string description, DataFlow::SourceNode source) {
|
||||
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 random number"
|
||||
"cryptographically secure random number"
|
||||
|
||||
@@ -1,15 +1,17 @@
|
||||
| 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 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 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 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 random number |
|
||||
| bad-random.js:9:28:9:43 | buffer[i] / 25.6 | Using division on a $@ produces biased results. | bad-random.js:6:16:6:40 | crypto. ... (bytes) | cryptographically 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 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 random number |
|
||||
| bad-random.js:73:32:73:42 | byte / 25.6 | Using division on a $@ produces biased results. | bad-random.js:70:20:70:44 | crypto. ... (bytes) | cryptographically 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 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 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 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 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 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 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 random number |
|
||||
| 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 |
|
||||
| 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 |
|
||||
|
||||
@@ -84,4 +84,30 @@ 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 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.
|
||||
|
||||
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.
|
||||
|
||||
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