Merge branch 'main' into experimental-decompression-api

ruby/extractor/Cargo.toml

@@ -17,4 +17,4 @@ tracing = "0.1"
 tracing-subscriber = { version = "0.3.3", features = ["env-filter"] }
 rayon = "1.5.0"
 num_cpus = "1.13.0"
-regex = "1.4.3"
+regex = "1.5.5"

ruby/ql/lib/codeql/ruby/Concepts.qll

@@ -826,7 +826,19 @@ module Logging {
  * to improve our libraries in the future to more precisely capture this aspect.
  */
 module Cryptography {
-  import security.CryptoAlgorithms
+  // Since we still rely on `isWeak` predicate on `CryptographicOperation` in Ruby, we
+  // modify that part of the shared concept... which means we have to explicitly
+  // re-export everything else.
+  // Using SC shorthand for "Shared Cryptography"
+  import codeql.ruby.internal.ConceptsShared::Cryptography as SC
+
+  class CryptographicAlgorithm = SC::CryptographicAlgorithm;
+
+  class EncryptionAlgorithm = SC::EncryptionAlgorithm;
+
+  class HashingAlgorithm = SC::HashingAlgorithm;
+
+  class PasswordHashingAlgorithm = SC::PasswordHashingAlgorithm;
 
   /**
    * A data-flow node that is an application of a cryptographic algorithm. For example,
@@ -835,15 +847,9 @@ module Cryptography {
    * Extend this class to refine existing API models. If you want to model new APIs,
    * extend `CryptographicOperation::Range` instead.
    */
-  class CryptographicOperation extends DataFlow::Node instanceof CryptographicOperation::Range {
-    /** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */
-    CryptographicAlgorithm getAlgorithm() { result = super.getAlgorithm() }
-
-    /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */
-    DataFlow::Node getAnInput() { result = super.getAnInput() }
-
-    /** Holds if this encryption operation is known to be weak. */
-    predicate isWeak() { super.isWeak() }
+  class CryptographicOperation extends SC::CryptographicOperation instanceof CryptographicOperation::Range {
+    /** DEPRECATED: Use `getAlgorithm().isWeak() or getBlockMode().isWeak()` instead */
+    deprecated predicate isWeak() { super.isWeak() }
   }
 
   /** Provides classes for modeling new applications of a cryptographic algorithms. */
@@ -855,15 +861,11 @@ module Cryptography {
      * Extend this class to model new APIs. If you want to refine existing API models,
      * extend `CryptographicOperation` instead.
      */
-    abstract class Range extends DataFlow::Node {
-      /** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */
-      abstract CryptographicAlgorithm getAlgorithm();
-
-      /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */
-      abstract DataFlow::Node getAnInput();
-
-      /** Holds if this encryption operation is known to be weak. */
-      abstract predicate isWeak();
+    abstract class Range extends SC::CryptographicOperation::Range {
+      /** DEPRECATED: Use `getAlgorithm().isWeak() or getBlockMode().isWeak()` instead */
+      deprecated predicate isWeak() { this.getAlgorithm().isWeak() or this.getBlockMode().isWeak() }
     }
   }
+
+  class BlockMode = SC::BlockMode;
 }

ruby/ql/lib/codeql/ruby/frameworks/stdlib/README.md

@@ -1,3 +1,3 @@
 This directory contains QL modules that model classes and modules in the Ruby standard library.
 Files are named after the most common or top-level class in each library.
-See https://docs.ruby-lang.org/en/3.1/doc/standard_library_rdoc.html for a full list.
+See https://docs.ruby-lang.org/en/3.1/standard_library_rdoc.html for a full list.

ruby/ql/lib/codeql/ruby/internal/ConceptsImports.qll

@@ -4,3 +4,4 @@
  */
 
 import codeql.ruby.DataFlow
+import codeql.ruby.security.CryptoAlgorithms as CryptoAlgorithms

ruby/ql/lib/codeql/ruby/internal/ConceptsShared.qll

@@ -11,3 +11,79 @@
  */
 
 private import ConceptsImports
+
+/**
+ * Provides models for cryptographic concepts.
+ *
+ * Note: The `CryptographicAlgorithm` class currently doesn't take weak keys into
+ * consideration for the `isWeak` member predicate. So RSA is always considered
+ * secure, although using a low number of bits will actually make it insecure. We plan
+ * to improve our libraries in the future to more precisely capture this aspect.
+ */
+module Cryptography {
+  class CryptographicAlgorithm = CryptoAlgorithms::CryptographicAlgorithm;
+
+  class EncryptionAlgorithm = CryptoAlgorithms::EncryptionAlgorithm;
+
+  class HashingAlgorithm = CryptoAlgorithms::HashingAlgorithm;
+
+  class PasswordHashingAlgorithm = CryptoAlgorithms::PasswordHashingAlgorithm;
+
+  /**
+   * A data-flow node that is an application of a cryptographic algorithm. For example,
+   * encryption, decryption, signature-validation.
+   *
+   * Extend this class to refine existing API models. If you want to model new APIs,
+   * extend `CryptographicOperation::Range` instead.
+   */
+  class CryptographicOperation extends DataFlow::Node instanceof CryptographicOperation::Range {
+    /** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */
+    CryptographicAlgorithm getAlgorithm() { result = super.getAlgorithm() }
+
+    /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */
+    DataFlow::Node getAnInput() { result = super.getAnInput() }
+
+    /**
+     * Gets the block mode used to perform this cryptographic operation.
+     * This may have no result - for example if the `CryptographicAlgorithm` used
+     * is a stream cipher rather than a block cipher.
+     */
+    BlockMode getBlockMode() { result = super.getBlockMode() }
+  }
+
+  /** Provides classes for modeling new applications of a cryptographic algorithms. */
+  module CryptographicOperation {
+    /**
+     * A data-flow node that is an application of a cryptographic algorithm. For example,
+     * encryption, decryption, signature-validation.
+     *
+     * Extend this class to model new APIs. If you want to refine existing API models,
+     * extend `CryptographicOperation` instead.
+     */
+    abstract class Range extends DataFlow::Node {
+      /** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */
+      abstract CryptographicAlgorithm getAlgorithm();
+
+      /** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */
+      abstract DataFlow::Node getAnInput();
+
+      /**
+       * Gets the block mode used to perform this cryptographic operation.
+       * This may have no result - for example if the `CryptographicAlgorithm` used
+       * is a stream cipher rather than a block cipher.
+       */
+      abstract BlockMode getBlockMode();
+    }
+  }
+
+  /**
+   * A cryptographic block cipher mode of operation. This can be used to encrypt
+   * data of arbitrary length using a block encryption algorithm.
+   */
+  class BlockMode extends string {
+    BlockMode() { this = ["ECB", "CBC", "GCM", "CCM", "CFB", "OFB", "CTR", "OPENPGP"] }
+
+    /** Holds if this block mode is considered to be insecure. */
+    predicate isWeak() { this = "ECB" }
+  }
+}

ruby/ql/lib/codeql/ruby/security/CryptoAlgorithms.qll

@@ -81,6 +81,9 @@ class EncryptionAlgorithm extends MkEncryptionAlgorithm, CryptographicAlgorithm
   override string getName() { result = name }
 
   override predicate isWeak() { isWeak = true }
+
+  /** Holds if this algorithm is a stream cipher. */
+  predicate isStreamCipher() { isStreamCipher(name) }
 }
 
 /**

ruby/ql/lib/codeql/ruby/security/OpenSSL.qll

@@ -29,7 +29,9 @@ private string rankedInsecureAlgorithm(int i) {
   // weak hash algorithms and block modes as well.
   result =
     rank[i](string s |
-      isWeakEncryptionAlgorithm(s) or isWeakHashingAlgorithm(s) or isWeakBlockMode(s)
+      isWeakEncryptionAlgorithm(s) or
+      isWeakHashingAlgorithm(s) or
+      s.(Cryptography::BlockMode).isWeak()
     )
 }
 
@@ -329,13 +331,9 @@ private API::Node cipherApi() {
   result = API::getTopLevelMember("OpenSSL").getMember("Cipher").getMember("Cipher")
 }
 
-private class BlockMode extends string {
-  BlockMode() { this = ["ECB", "CBC", "GCM", "CCM", "CFB", "OFB", "CTR"] }
-}
-
 private newtype TCipherMode =
   TStreamCipher() or
-  TBlockMode(BlockMode blockMode)
+  TBlockMode(Cryptography::BlockMode blockMode)
 
 /**
  * Represents the mode used by this stream cipher.
@@ -343,7 +341,8 @@ private newtype TCipherMode =
  * block mode.
  */
 private class CipherMode extends TCipherMode {
-  private BlockMode getBlockMode() { this = TBlockMode(result) }
+  /** Gets the underlying block mode, if any. */
+  Cryptography::BlockMode getBlockMode() { this = TBlockMode(result) }
 
   /** Gets a textual representation of this node. */
   string toString() {
@@ -360,7 +359,7 @@ private class CipherMode extends TCipherMode {
   predicate isBlockMode(string s) { this.getBlockMode() = s.toUpperCase() }
 
   /** Holds if this cipher mode is a weak block mode. */
-  predicate isWeak() { isWeakBlockMode(this.getBlockMode()) }
+  predicate isWeak() { this.getBlockMode().isWeak() }
 }
 
 private string getStringArgument(DataFlow::CallNode call, int i) {
@@ -371,6 +370,25 @@ private int getIntArgument(DataFlow::CallNode call, int i) {
   result = call.getArgument(i).asExpr().getConstantValue().getInt()
 }
 
+bindingset[blockCipherName]
+private Cryptography::BlockMode getCandidateBlockModeFromCipherName(string blockCipherName) {
+  result = blockCipherName.splitAt("-", [1, 2]).toUpperCase()
+}
+
+/**
+ * Gets the block mode specified as part of a block cipher name used to
+ * instantiate an `OpenSSL::Cipher` instance. If the block mode is not
+ * explicitly specified, this defaults to "CBC".
+ */
+bindingset[blockCipherName]
+private Cryptography::BlockMode getBlockModeFromCipherName(string blockCipherName) {
+  // Extract the block mode from the cipher name
+  result = getCandidateBlockModeFromCipherName(blockCipherName)
+  or
+  // Fall back on the OpenSSL default of CBC if the block mode is unspecified
+  not exists(getCandidateBlockModeFromCipherName(blockCipherName)) and result = "CBC"
+}
+
 /**
  * Holds if `call` is a call to `OpenSSL::Cipher.new` that instantiates a
  * `cipher` instance with mode `cipherMode`.
@@ -382,8 +400,9 @@ private predicate cipherInstantiationGeneric(
     // `OpenSSL::Cipher.new('<cipherName>')`
     call = cipherApi().getAnInstantiation() and
     cipherName = getStringArgument(call, 0) and
-    // CBC is used by default
-    cipherMode.isBlockMode("CBC")
+    if cipher.getAlgorithm().isStreamCipher()
+    then cipherMode = TStreamCipher()
+    else cipherMode.isBlockMode(getBlockModeFromCipherName(cipherName))
   )
 }
 
@@ -398,12 +417,12 @@ private predicate cipherInstantiationAES(
   exists(string cipherName | cipher.matchesName(cipherName) |
     // `OpenSSL::Cipher::AES` instantiations
     call = cipherApi().getMember("AES").getAnInstantiation() and
-    exists(string keyLength, string blockMode |
+    exists(string keyLength, Cryptography::BlockMode blockMode |
       // `OpenSSL::Cipher::AES.new('<keyLength-blockMode>')
       exists(string arg0 |
         arg0 = getStringArgument(call, 0) and
         keyLength = arg0.splitAt("-", 0) and
-        blockMode = arg0.splitAt("-", 1).toUpperCase()
+        blockMode = getBlockModeFromCipherName(arg0)
       )
       or
       // `OpenSSL::Cipher::AES.new(<keyLength>, '<blockMode>')`
@@ -419,7 +438,7 @@ private predicate cipherInstantiationAES(
       call = cipherApi().getMember(mod).getAnInstantiation() and
       // Canonical representation is `AES-<keyLength>`
       blockAlgo = "AES-" + mod.suffix(3) and
-      exists(string blockMode |
+      exists(Cryptography::BlockMode blockMode |
         if exists(getStringArgument(call, 0))
         then
           // `OpenSSL::Cipher::<blockAlgo>.new('<blockMode>')`
@@ -446,7 +465,7 @@ private predicate cipherInstantiationSpecific(
     // Block ciphers with dedicated modules
     exists(string blockAlgo | blockAlgo = ["BF", "CAST5", "DES", "IDEA", "RC2"] |
       call = cipherApi().getMember(blockAlgo).getAnInstantiation() and
-      exists(string blockMode |
+      exists(Cryptography::BlockMode blockMode |
         if exists(getStringArgument(call, 0))
         then
           // `OpenSSL::Cipher::<blockAlgo>.new('<blockMode>')`
@@ -528,25 +547,25 @@ private class CipherNode extends DataFlow::Node {
 private class CipherOperation extends Cryptography::CryptographicOperation::Range,
   DataFlow::CallNode {
   private CipherNode cipherNode;
-  private DataFlow::Node input;
 
   CipherOperation() {
     // cipher instantiation is counted as a cipher operation with no input
     cipherNode = this and cipherNode instanceof CipherInstantiation
     or
     this.getReceiver() = cipherNode and
-    this.getMethodName() = "update" and
-    input = this.getArgument(0)
+    this.getMethodName() = "update"
   }
 
   override Cryptography::EncryptionAlgorithm getAlgorithm() {
     result = cipherNode.getCipher().getAlgorithm()
   }
 
-  override DataFlow::Node getAnInput() { result = input }
+  override DataFlow::Node getAnInput() {
+    this.getMethodName() = "update" and
+    result = this.getArgument(0)
+  }
 
-  override predicate isWeak() {
-    cipherNode.getCipher().isWeak() or
-    cipherNode.getCipherMode().isWeak()
+  override Cryptography::BlockMode getBlockMode() {
+    result = cipherNode.getCipherMode().getBlockMode()
   }
 }

ruby/ql/lib/codeql/ruby/security/internal/CryptoAlgorithmNames.qll

@@ -67,6 +67,6 @@ predicate isStrongPasswordHashingAlgorithm(string name) {
 predicate isWeakPasswordHashingAlgorithm(string name) { name = "EVPKDF" }
 
 /**
- * Holds if `name` corresponds to a weak block cipher mode of operation.
+ * Holds if `name` corresponds to a stream cipher.
  */
-predicate isWeakBlockMode(string name) { name = "ECB" }
+predicate isStreamCipher(string name) { name = ["CHACHA", "RC4", "ARC4", "ARCFOUR", "RABBIT"] }

ruby/ql/src/change-notes/2022-05-16-broken-crypto-message.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The query "Use of a broken or weak cryptographic algorithm" (`rb/weak-cryptographic-algorithm`) now report if a cryptographic operation is potentially insecure due to use of a weak block mode.

ruby/ql/src/queries/security/cwe-327/BrokenCryptoAlgorithm.ql

@@ -13,8 +13,10 @@
 import ruby
 import codeql.ruby.Concepts
 
-from Cryptography::CryptographicOperation operation
-where operation.isWeak()
-select operation,
-  "The cryptographic algorithm " + operation.getAlgorithm().getName() +
-    " is broken or weak, and should not be used."
+from Cryptography::CryptographicOperation operation, string msgPrefix
+where
+  operation.getAlgorithm().isWeak() and
+  msgPrefix = "The cryptographic algorithm " + operation.getAlgorithm().getName()
+  or
+  operation.getBlockMode().isWeak() and msgPrefix = "The block mode " + operation.getBlockMode()
+select operation, msgPrefix + " is broken or weak, and should not be used."

ruby/ql/test/library-tests/concepts/CryptographicOperation.ql

@@ -0,0 +1,33 @@
+import ruby
+import codeql.ruby.Concepts
+import TestUtilities.InlineExpectationsTest
+
+class CryptographicOperationTest extends InlineExpectationsTest {
+  CryptographicOperationTest() { this = "CryptographicOperationTest" }
+
+  override string getARelevantTag() {
+    result in [
+        "CryptographicOperation", "CryptographicOperationInput", "CryptographicOperationAlgorithm",
+        "CryptographicOperationBlockMode"
+      ]
+  }
+
+  override predicate hasActualResult(Location location, string element, string tag, string value) {
+    exists(Cryptography::CryptographicOperation cryptoOperation |
+      location = cryptoOperation.getLocation() and
+      (
+        element = cryptoOperation.toString() and
+        value = "" and
+        tag = "CryptographicOperation"
+        or
+        element = cryptoOperation.toString() and
+        value = cryptoOperation.getAlgorithm().getName() and
+        tag = "CryptographicOperationAlgorithm"
+        or
+        element = cryptoOperation.toString() and
+        value = cryptoOperation.getBlockMode() and
+        tag = "CryptographicOperationBlockMode"
+      )
+    )
+  }
+}

ruby/ql/test/library-tests/concepts/cryptographic_operations.rb

@@ -0,0 +1,11 @@
+require 'openssl'
+
+strong = OpenSSL::Cipher.new 'AES-128-CBC' # $ CryptographicOperation CryptographicOperationAlgorithm=AES CryptographicOperationBlockMode=CBC
+
+weak_algorithm = OpenSSL::Cipher::DES.new # $ CryptographicOperation CryptographicOperationAlgorithm=DES CryptographicOperationBlockMode=CBC
+
+weak_block_mode = OpenSSL::Cipher::AES.new(128, :ecb) # $ CryptographicOperation CryptographicOperationAlgorithm=AES CryptographicOperationBlockMode=ECB
+
+weak_block_mode = OpenSSL::Cipher.new 'bf-ecb' # $ CryptographicOperation CryptographicOperationAlgorithm=BF CryptographicOperationBlockMode=ECB
+
+stream_cipher = OpenSSL::Cipher.new 'chacha' # $ CryptographicOperation CryptographicOperationAlgorithm=CHACHA

ruby/ql/test/query-tests/security/cwe-327/BrokenCryptoAlgorithm.expected

@@ -1,17 +1,18 @@
 | broken_crypto.rb:4:8:4:34 | call to new | The cryptographic algorithm DES is broken or weak, and should not be used. |
 | broken_crypto.rb:8:1:8:18 | call to update | The cryptographic algorithm DES is broken or weak, and should not be used. |
-| broken_crypto.rb:12:8:12:43 | call to new | The cryptographic algorithm AES is broken or weak, and should not be used. |
-| broken_crypto.rb:16:1:16:18 | call to update | The cryptographic algorithm AES is broken or weak, and should not be used. |
-| broken_crypto.rb:28:1:28:35 | call to new | The cryptographic algorithm AES is broken or weak, and should not be used. |
-| broken_crypto.rb:37:1:37:33 | call to new | The cryptographic algorithm AES is broken or weak, and should not be used. |
-| broken_crypto.rb:42:1:42:33 | call to new | The cryptographic algorithm AES is broken or weak, and should not be used. |
-| broken_crypto.rb:47:1:47:33 | call to new | The cryptographic algorithm AES is broken or weak, and should not be used. |
-| broken_crypto.rb:52:1:52:29 | call to new | The cryptographic algorithm BF is broken or weak, and should not be used. |
-| broken_crypto.rb:57:1:57:32 | call to new | The cryptographic algorithm CAST5 is broken or weak, and should not be used. |
+| broken_crypto.rb:12:8:12:43 | call to new | The block mode ECB is broken or weak, and should not be used. |
+| broken_crypto.rb:16:1:16:18 | call to update | The block mode ECB is broken or weak, and should not be used. |
+| broken_crypto.rb:28:1:28:35 | call to new | The block mode ECB is broken or weak, and should not be used. |
+| broken_crypto.rb:37:1:37:33 | call to new | The block mode ECB is broken or weak, and should not be used. |
+| broken_crypto.rb:42:1:42:33 | call to new | The block mode ECB is broken or weak, and should not be used. |
+| broken_crypto.rb:47:1:47:33 | call to new | The block mode ECB is broken or weak, and should not be used. |
+| broken_crypto.rb:52:1:52:29 | call to new | The block mode ECB is broken or weak, and should not be used. |
+| broken_crypto.rb:57:1:57:32 | call to new | The block mode ECB is broken or weak, and should not be used. |
 | broken_crypto.rb:60:1:60:24 | call to new | The cryptographic algorithm DES is broken or weak, and should not be used. |
 | broken_crypto.rb:62:1:62:30 | call to new | The cryptographic algorithm DES is broken or weak, and should not be used. |
-| broken_crypto.rb:67:1:67:31 | call to new | The cryptographic algorithm IDEA is broken or weak, and should not be used. |
+| broken_crypto.rb:67:1:67:31 | call to new | The block mode ECB is broken or weak, and should not be used. |
 | broken_crypto.rb:70:1:70:24 | call to new | The cryptographic algorithm RC2 is broken or weak, and should not be used. |
+| broken_crypto.rb:72:1:72:30 | call to new | The block mode ECB is broken or weak, and should not be used. |
 | broken_crypto.rb:72:1:72:30 | call to new | The cryptographic algorithm RC2 is broken or weak, and should not be used. |
 | broken_crypto.rb:75:1:75:24 | call to new | The cryptographic algorithm RC4 is broken or weak, and should not be used. |
 | broken_crypto.rb:77:1:77:29 | call to new | The cryptographic algorithm RC4 is broken or weak, and should not be used. |