Merge pull request #7021 from erik-krogh/cwe326

JS: Add insufficient key size query

javascript/change-notes/2021-11-02-insufficient-key-size.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The `js/insufficient-key-size` query has been added. It highlights the creation of cryptographic keys with a short key size.

javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll

@@ -25,6 +25,26 @@ abstract class CryptographicOperation extends Expr {
  */
 abstract class CryptographicKey extends DataFlow::ValueNode { }
 
+/**
+ * The creation of a cryptographic key.
+ */
+abstract class CryptographicKeyCreation extends DataFlow::Node {
+  /**
+   * Gets the algorithm used to create the key.
+   */
+  abstract CryptographicAlgorithm getAlgorithm();
+
+  /**
+   * Gets the size of the key.
+   */
+  abstract int getSize();
+
+  /**
+   * Gets whether the key is symmetric.
+   */
+  abstract predicate isSymmetricKey();
+}
+
 /**
  * A key used in a cryptographic algorithm, viewed as a `CredentialsExpr`.
  */
@@ -141,14 +161,9 @@ private module NodeJSCrypto {
        *       Also matches `createHash`, `createHmac`, `createSign` instead of `createCipher`.
        */
 
-      exists(DataFlow::SourceNode mod, string createSuffix |
-        createSuffix = "Hash" or
-        createSuffix = "Hmac" or
-        createSuffix = "Sign" or
-        createSuffix = "Cipher"
-      |
+      exists(DataFlow::SourceNode mod |
         mod = DataFlow::moduleImport("crypto") and
-        this = mod.getAMemberCall("create" + createSuffix) and
+        this = mod.getAMemberCall("create" + ["Hash", "Hmac", "Sign", "Cipher"]) and
         algorithm.matchesName(getArgument(0).getStringValue())
       )
     }
@@ -156,6 +171,52 @@ private module NodeJSCrypto {
     CryptographicAlgorithm getAlgorithm() { result = algorithm }
   }
 
+  private class CreateKey extends CryptographicKeyCreation, DataFlow::CallNode {
+    boolean symmetric;
+
+    CreateKey() {
+      // crypto.generateKey(type, options, callback)
+      // crypto.generateKeyPair(type, options, callback)
+      // crypto.generateKeyPairSync(type, options)
+      // crypto.generateKeySync(type, options)
+      exists(DataFlow::SourceNode mod, string keyType |
+        keyType = "Key" and symmetric = true
+        or
+        keyType = "KeyPair" and symmetric = false
+      |
+        mod = DataFlow::moduleImport("crypto") and
+        this = mod.getAMemberCall("generate" + keyType + ["", "Sync"])
+      )
+    }
+
+    override CryptographicAlgorithm getAlgorithm() {
+      result.matchesName(getArgument(0).getStringValue())
+    }
+
+    override int getSize() {
+      symmetric = true and
+      result = getOptionArgument(1, "length").getIntValue()
+      or
+      symmetric = false and
+      result = getOptionArgument(1, "modulusLength").getIntValue()
+    }
+
+    override predicate isSymmetricKey() { symmetric = true }
+  }
+
+  private class CreateDiffieHellmanKey extends CryptographicKeyCreation, DataFlow::CallNode {
+    // require("crypto").createDiffieHellman(prime_length);
+    CreateDiffieHellmanKey() {
+      this = DataFlow::moduleMember("crypto", "createDiffieHellman").getACall()
+    }
+
+    override CryptographicAlgorithm getAlgorithm() { none() }
+
+    override int getSize() { result = getArgument(0).getIntValue() }
+
+    override predicate isSymmetricKey() { none() }
+  }
+
   private class Apply extends CryptographicOperation, MethodCallExpr {
     InstantiatedAlgorithm instantiation;
 
@@ -282,6 +343,35 @@ private module CryptoJS {
       )
     }
   }
+
+  private class CreateKey extends CryptographicKeyCreation, DataFlow::CallNode {
+    string algorithm;
+    int optionArg;
+
+    CreateKey() {
+      // var key = CryptoJS.PBKDF2(password, salt, { keySize: 8 });
+      this =
+        getAlgorithmExpr(any(CryptographicAlgorithm algo | algo.getName() = algorithm)).getACall() and
+      optionArg = 2
+      or
+      // var key = CryptoJS.algo.PBKDF2.create({ keySize: 8 });
+      this =
+        DataFlow::moduleMember("crypto-js", "algo")
+            .getAPropertyRead(algorithm)
+            .getAMethodCall("create") and
+      optionArg = 0
+    }
+
+    override CryptographicAlgorithm getAlgorithm() { result.matchesName(algorithm) }
+
+    override int getSize() {
+      result = getOptionArgument(optionArg, "keySize").getIntValue() * 32 // size is in words
+      or
+      result = getArgument(optionArg).getIntValue() * 32 // size is in words
+    }
+
+    override predicate isSymmetricKey() { any() }
+  }
 }
 
 /**
@@ -467,6 +557,39 @@ private module Forge {
   private class Key extends CryptographicKey {
     Key() { this = any(KeyCipher cipher).getKey() }
   }
+
+  private class CreateKey extends CryptographicKeyCreation, DataFlow::CallNode {
+    CryptographicAlgorithm algorithm;
+
+    CreateKey() {
+      // var cipher = forge.rc2.createEncryptionCipher(key, 128);
+      this =
+        getAnImportNode()
+            .getAPropertyRead(any(string s | algorithm.matchesName(s)))
+            .getAMemberCall("createEncryptionCipher")
+      or
+      // var key = forge.random.getBytesSync(16);
+      // var cipher = forge.cipher.createCipher('AES-CBC', key);
+      this =
+        getAnImportNode()
+            .getAPropertyRead("cipher")
+            .getAMemberCall(["createCipher", "createDecipher"]) and
+      algorithm.matchesName(this.getArgument(0).getStringValue())
+    }
+
+    override CryptographicAlgorithm getAlgorithm() { result = algorithm }
+
+    override int getSize() {
+      result = this.getArgument(1).getIntValue()
+      or
+      exists(DataFlow::CallNode call | call.getCalleeName() = ["getBytes", "getBytesSync"] |
+        getArgument(1).getALocalSource() = call and
+        result = call.getArgument(0).getIntValue() * 8 // bytes to bits
+      )
+    }
+
+    override predicate isSymmetricKey() { any() }
+  }
 }
 
 /**
@@ -556,13 +679,38 @@ private module Hasha {
 
     override CryptographicAlgorithm getAlgorithm() { result = algorithm }
   }
+}
 
-  /**
+/**
  * Provides classes for working with the `express-jwt` package (https://github.com/auth0/express-jwt);
  */
-  module ExpressJwt {
+private module ExpressJwt {
   private class Key extends CryptographicKey {
     Key() { this = DataFlow::moduleMember("express-jwt", "sign").getACall().getArgument(1) }
   }
+}
+
+/**
+ * Provides classes for working with the `node-rsa` package (https://www.npmjs.com/package/node-rsa)
+ */
+private module NodeRsa {
+  private class CreateKey extends CryptographicKeyCreation, API::InvokeNode {
+    CryptographicAlgorithm algorithm;
+
+    CreateKey() {
+      this = API::moduleImport("node-rsa").getAnInstantiation()
+      or
+      this = API::moduleImport("node-rsa").getInstance().getMember("generateKeyPair").getACall()
+    }
+
+    override CryptographicAlgorithm getAlgorithm() { result.matchesName("rsa") }
+
+    override int getSize() {
+      result = this.getArgument(0).getIntValue()
+      or
+      result = this.getOptionArgument(0, "b").getIntValue()
+    }
+
+    override predicate isSymmetricKey() { none() }
   }
 }

javascript/ql/lib/semmle/javascript/security/CryptoAlgorithms.qll

@@ -46,7 +46,7 @@ private module AlgorithmNames {
     name = ["ARGON2", "PBKDF2", "BCRYPT", "SCRYPT"]
   }
 
-  predicate isWeakPasswordHashingAlgorithm(string name) { none() }
+  predicate isWeakPasswordHashingAlgorithm(string name) { name = "EVPKDF" }
 }
 
 private import AlgorithmNames
@@ -85,11 +85,13 @@ abstract class CryptographicAlgorithm extends TCryptographicAlgorithm {
 
   /**
    * Holds if the name of this algorithm matches `name` modulo case,
-   * white space, dashes, and underscores.
+   * white space, dashes, underscores, and anything after a dash in the name
+   * (to ignore modes of operation, such as CBC or ECB).
    */
   bindingset[name]
   predicate matchesName(string name) {
-    name.toUpperCase().regexpReplaceAll("[-_ ]", "") = getName()
+    [name.toUpperCase(), name.toUpperCase().regexpCapture("^(\\w+)(?:-.*)?$", 1)]
+        .regexpReplaceAll("[-_ ]", "") = getName()
   }
 
   /**

javascript/ql/src/Security/CWE-326/InsufficientKeySize.qhelp

@@ -0,0 +1,43 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Modern encryption relies on it being computationally infeasible to break the cipher and decode a message without the key.
+As computational power increases, the ability to break ciphers grows and keys need to become larger.
+</p>
+</overview>
+
+<recommendation>
+<p>
+An encryption key should be at least 2048-bit long when using RSA encryption, and 128-bit long when using 
+symmetric encryption.
+</p>
+</recommendation>
+
+<references>
+<li>
+Wikipedia:
+<a href="https://en.wikipedia.org/wiki/RSA_(cryptosystem)">RSA</a>.
+</li>
+<li>
+Wikipedia:
+<a href="https://en.wikipedia.org/wiki/Advanced_Encryption_Standard">AES</a>.
+</li>
+<li>
+NodeJS: 
+<a href="https://nodejs.org/api/crypto.html">Crypto</a>.
+</li>
+<li>
+NIST:
+<a href="https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar1.pdf">
+Recommendation for Transitioning the Use of Cryptographic Algorithms and Key Lengths</a>.
+</li>
+<li>
+Wikipedia:
+<a href="https://en.wikipedia.org/wiki/Key_size">Key size</a>
+</li>
+</references>
+</qhelp>

javascript/ql/src/Security/CWE-326/InsufficientKeySize.ql

@@ -0,0 +1,36 @@
+/**
+ * @name Use of a weak cryptographic key
+ * @description Using a weak cryptographic key can allow an attacker to compromise security.
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity 7.5
+ * @precision high
+ * @id js/insufficient-key-size
+ * @tags security
+ *       external/cwe/cwe-326
+ */
+
+import javascript
+
+from CryptographicKeyCreation key, int size, string msg, string algo
+where
+  size = key.getSize() and
+  (
+    algo = key.getAlgorithm() + " "
+    or
+    not exists(key.getAlgorithm()) and algo = ""
+  ) and
+  (
+    size < 128 and
+    key.isSymmetricKey() and
+    msg =
+      "Creation of an symmetric " + algo + "key uses " + size +
+        " bits, which is below 128 and considered breakable."
+    or
+    size < 2048 and
+    not key.isSymmetricKey() and
+    msg =
+      "Creation of an asymmetric " + algo + "key uses " + size +
+        " bits, which is below 2048 and considered breakable."
+  )
+select key, msg

javascript/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.expected

@@ -0,0 +1,11 @@
+| tst.js:3:14:3:71 | crypto. ... 1024 }) | Creation of an asymmetric RSA key uses 1024 bits, which is below 2048 and considered breakable. |
+| tst.js:7:14:7:59 | crypto. ... : 64 }) | Creation of an symmetric key uses 64 bits, which is below 128 and considered breakable. |
+| tst.js:13:14:13:56 | CryptoJ ... e: 2 }) | Creation of an symmetric PBKDF2 key uses 64 bits, which is below 128 and considered breakable. |
+| tst.js:14:14:14:60 | CryptoJ ... e: 2 }) | Creation of an symmetric PBKDF2 key uses 64 bits, which is below 128 and considered breakable. |
+| tst.js:15:14:15:60 | CryptoJ ... e: 2 }) | Creation of an symmetric EVPKDF key uses 64 bits, which is below 128 and considered breakable. |
+| tst.js:19:12:19:57 | forge.r ... rd, 64) | Creation of an symmetric RC2 key uses 64 bits, which is below 128 and considered breakable. |
+| tst.js:26:12:26:53 | forge.c ... , key2) | Creation of an symmetric AES key uses 64 bits, which is below 128 and considered breakable. |
+| tst.js:30:12:30:56 | forge.c ... , key3) | Creation of an symmetric 3DES key uses 64 bits, which is below 128 and considered breakable. |
+| tst.js:35:13:35:43 | crypto. ... an(512) | Creation of an asymmetric key uses 512 bits, which is below 2048 and considered breakable. |
+| tst.js:39:13:39:33 | new Nod ... : 512}) | Creation of an asymmetric RSA key uses 512 bits, which is below 2048 and considered breakable. |
+| tst.js:43:1:43:31 | key.gen ...  65537) | Creation of an asymmetric RSA key uses 512 bits, which is below 2048 and considered breakable. |

javascript/ql/test/query-tests/Security/CWE-326/InsufficientKeySize.qlref

@@ -0,0 +1 @@
+Security/CWE-326/InsufficientKeySize.ql

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

@@ -0,0 +1,44 @@
+const crypto = require("crypto");
+
+const bad1 = crypto.generateKeyPairSync("rsa", { modulusLength: 1024 }); // NOT OK
+
+const good1 = crypto.generateKeyPairSync("rsa", { modulusLength: 4096 }); // OK
+
+const bad2 = crypto.generateKeySync("hmac", { length: 64 }); // NOT OK
+
+const good2 = crypto.generateKeySync("aes", { length: 256 }); // OK
+
+var CryptoJS = require("crypto-js");
+
+const bad3 = CryptoJS.algo.PBKDF2.create({ keySize: 2 }); // NOT OK
+const bad4 = CryptoJS.PBKDF2(password, salt, { keySize: 2 }); // NOT OK
+const bad5 = CryptoJS.EvpKDF(password, salt, { keySize: 2 }); // NOT OK
+const bad6 = CryptoJS.PBKDF2(password, salt, { keySize: 8 }); // OK
+
+const forge = require("node-forge");
+var bad7 = forge.rc2.createEncryptionCipher(password, 64); // NOT OK
+var good3 = forge.rc2.createEncryptionCipher(password, 128); // OK
+
+var key1 = forge.random.getBytesSync(16);
+var good4 = forge.cipher.createCipher('AES-CBC', key1); // OK
+
+var key2 = forge.random.getBytesSync(8);
+var bad8 = forge.cipher.createCipher('AES-CBC', key2); // NOT OK
+
+var myBuffer = forge.util.createBuffer(manyBytes);
+var key3 = myBuffer.getBytes(8);
+var bad9 = forge.cipher.createDecipher('3DES-CBC', key3); // NOT OK
+
+var key4 = myBuffer.getBytes(16);
+var good5 = forge.cipher.createDecipher('AES-CBC', key4); // OK
+
+var bad10 = crypto.createDiffieHellman(512);
+var good6 = crypto.createDiffieHellman(2048);
+
+const NodeRSA = require('node-rsa');
+var bad11 = new NodeRSA({b: 512}); // NOT OK
+var good7 = new NodeRSA({b: 4096}); // OK
+
+var key = new NodeRSA(); // OK
+key.generateKeyPair(512, 65537); // NOT OK
+key.generateKeyPair(4096, 65537); // OK

python/ql/lib/semmle/python/concepts/CryptoAlgorithms.qll

@@ -46,7 +46,7 @@ private module AlgorithmNames {
     name = ["ARGON2", "PBKDF2", "BCRYPT", "SCRYPT"]
   }
 
-  predicate isWeakPasswordHashingAlgorithm(string name) { none() }
+  predicate isWeakPasswordHashingAlgorithm(string name) { name = "EVPKDF" }
 }
 
 private import AlgorithmNames
@@ -85,11 +85,13 @@ abstract class CryptographicAlgorithm extends TCryptographicAlgorithm {
 
   /**
    * Holds if the name of this algorithm matches `name` modulo case,
-   * white space, dashes, and underscores.
+   * white space, dashes, underscores, and anything after a dash in the name
+   * (to ignore modes of operation, such as CBC or ECB).
    */
   bindingset[name]
   predicate matchesName(string name) {
-    name.toUpperCase().regexpReplaceAll("[-_ ]", "") = getName()
+    [name.toUpperCase(), name.toUpperCase().regexpCapture("^(\\w+)(?:-.*)?$", 1)]
+        .regexpReplaceAll("[-_ ]", "") = getName()
   }
 
   /**