don't report insecure randomness when the insecure random is just a fallback

Author: erik-krogh

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

@@ -55,26 +55,6 @@ private DataFlow::Node isPowerOfTwoMinusOne() {
   )
 }
 
-/**
- * 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.
@@ -86,7 +66,7 @@ private string prop() { result = DataFlow::PseudoProperties::setElement() }
  */
 private DataFlow::Node goodRandom(DataFlow::TypeTracker t, DataFlow::SourceNode source) {
   t.startInProp(prop()) and
-  result = randomBufferSource() and
+  result = InsecureRandomness::randomBufferSource() and
   result = source
   or
   // Loading a number from a `Buffer`.

javascript/ql/src/semmle/javascript/security/dataflow/InsecureRandomnessCustomizations.qll

@@ -30,39 +30,50 @@ module InsecureRandomness {
     override InvokeExpr astNode;
 
     DefaultSource() {
-      exists(DataFlow::ModuleImportNode mod, string name | mod.getPath() = name |
-        // require("random-number")();
-        name = "random-number" and
-        this = mod.getACall()
+      not this.getContainer() = getASecureRandomGeneratingFunction() and
+      (
+        exists(DataFlow::ModuleImportNode mod, string name | mod.getPath() = name |
+          // require("random-number")();
+          name = "random-number" and
+          this = mod.getACall()
+          or
+          // require("random-int")();
+          name = "random-int" and
+          this = mod.getACall()
+          or
+          // require("random-float")();
+          name = "random-float" and
+          this = mod.getACall()
+          or
+          // require('random-seed').create()();
+          name = "random-seed" and
+          this = mod.getAMemberCall("create").getACall()
+          or
+          // require('unique-random')()();
+          name = "unique-random" and
+          this = mod.getACall().getACall()
+        )
         or
-        // require("random-int")();
-        name = "random-int" and
-        this = mod.getACall()
+        // Math.random()
+        this = DataFlow::globalVarRef("Math").getAMemberCall("random")
         or
-        // require("random-float")();
-        name = "random-float" and
-        this = mod.getACall()
+        // (new require('chance')).<name>()
+        this = DataFlow::moduleImport("chance").getAnInstantiation().getAMemberInvocation(_)
         or
-        // require('random-seed').create()();
-        name = "random-seed" and
-        this = mod.getAMemberCall("create").getACall()
-        or
-        // require('unique-random')()();
-        name = "unique-random" and
-        this = mod.getACall().getACall()
+        // require('crypto').pseudoRandomBytes()
+        this = DataFlow::moduleMember("crypto", "pseudoRandomBytes").getAnInvocation()
       )
-      or
-      // Math.random()
-      this = DataFlow::globalVarRef("Math").getAMemberCall("random")
-      or
-      // (new require('chance')).<name>()
-      this = DataFlow::moduleImport("chance").getAnInstantiation().getAMemberInvocation(_)
-      or
-      // require('crypto').pseudoRandomBytes()
-      this = DataFlow::moduleMember("crypto", "pseudoRandomBytes").getAnInvocation()
     }
   }
 
+  /**
+   * Gets a container that at some point generates a secure random value.
+   */
+  pragma[noinline]
+  private StmtContainer getASecureRandomGeneratingFunction() {
+    result = randomBufferSource().getContainer()
+  }
+
   /**
    * A sensitive write, considered as a sink for random values that are not cryptographically
    * secure.
@@ -94,4 +105,24 @@ module InsecureRandomness {
       succ = mc
     )
   }
+
+  /**
+   * Gets a Buffer/TypedArray containing cryptographically secure random numbers.
+   */
+  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"])
+  }
 }

javascript/ql/test/query-tests/Security/CWE-338/tst.js

@@ -93,4 +93,20 @@ function f18() {
 (function(){
     var crypto = require('crypto');
     crypto.createHmac('sha256', Math.random());
-})()
+})();
+
+(function () {
+    function genRandom() {
+        if (window.crypto && crypto.getRandomValues && !isSafari()) {
+            var a = window.crypto.getRandomValues(new Uint32Array(3)),
+                token = '';
+            for (var i = 0, l = a.length; i < l; i++) {
+                token += a[i].toString(36);
+            }
+            return token;
+        } else {
+            return (Math.random() * new Date().getTime()).toString(36).replace(/\./g, '');
+        }
+    };
+    var secret = genRandom(); // OK - Math.random() is only a fallback.
+})();