Merge branch 'main' into actiondispatch-response

ruby/ql/lib/change-notes/2022-10-14-digest-model.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The hashing algorithms from `Digest` and `OpenSSL::Digest` are now recognized and can be flagged by the `rb/weak-cryptographic-algorithm` query.

ruby/ql/lib/codeql/ruby/ApiGraphs.qll

@@ -203,7 +203,7 @@ module API {
     /**
      * Gets a node representing a call to `method` on the receiver represented by this node.
      */
-    Node getMethod(string method) {
+    MethodAccessNode getMethod(string method) {
       result = this.getASubclass().getASuccessor(Label::method(method))
     }
 

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll

@@ -1003,6 +1003,8 @@ predicate jumpStep(Node pred, Node succ) {
     succ.(SsaDefinitionNode).getDefinition())
   or
   succ.asExpr().getExpr().(ConstantReadAccess).getValue() = pred.asExpr().getExpr()
+  or
+  FlowSummaryImpl::Private::Steps::summaryJumpStep(pred, succ)
 }
 
 private ContentSet getKeywordContent(string name) {

ruby/ql/lib/codeql/ruby/dataflow/internal/FlowSummaryImpl.qll

@@ -61,6 +61,20 @@ module Public {
 
     /** Gets a summary component for a return of kind `rk`. */
     SummaryComponent return(ReturnKind rk) { result = TReturnSummaryComponent(rk) }
+
+    /** Gets a summary component for synthetic global `sg`. */
+    SummaryComponent syntheticGlobal(SyntheticGlobal sg) {
+      result = TSyntheticGlobalSummaryComponent(sg)
+    }
+
+    /**
+     * A synthetic global. This represents some form of global state, which
+     * summaries can read and write individually.
+     */
+    abstract class SyntheticGlobal extends string {
+      bindingset[this]
+      SyntheticGlobal() { any() }
+    }
   }
 
   /**
@@ -256,6 +270,7 @@ module Private {
     TParameterSummaryComponent(ArgumentPosition pos) or
     TArgumentSummaryComponent(ParameterPosition pos) or
     TReturnSummaryComponent(ReturnKind rk) or
+    TSyntheticGlobalSummaryComponent(SummaryComponent::SyntheticGlobal sg) or
     TWithoutContentSummaryComponent(ContentSet c) or
     TWithContentSummaryComponent(ContentSet c)
 
@@ -563,6 +578,11 @@ module Private {
             getCallbackReturnType(getNodeType(summaryNodeInputState(pragma[only_bind_out](c),
                   s.tail())), rk)
         )
+        or
+        exists(SummaryComponent::SyntheticGlobal sg |
+          head = TSyntheticGlobalSummaryComponent(sg) and
+          result = getSyntheticGlobalType(sg)
+        )
       )
       or
       n = summaryNodeOutputState(c, s) and
@@ -582,6 +602,11 @@ module Private {
             getCallbackParameterType(getNodeType(summaryNodeInputState(pragma[only_bind_out](c),
                   s.tail())), pos)
         )
+        or
+        exists(SummaryComponent::SyntheticGlobal sg |
+          head = TSyntheticGlobalSummaryComponent(sg) and
+          result = getSyntheticGlobalType(sg)
+        )
       )
     )
   }
@@ -692,6 +717,18 @@ module Private {
       )
     }
 
+    /**
+     * Holds if there is a jump step from `pred` to `succ`, which is synthesized
+     * from a flow summary.
+     */
+    predicate summaryJumpStep(Node pred, Node succ) {
+      exists(SummaryComponentStack s |
+        s = SummaryComponentStack::singleton(SummaryComponent::syntheticGlobal(_)) and
+        pred = summaryNodeOutputState(_, s) and
+        succ = summaryNodeInputState(_, s)
+      )
+    }
+
     /**
      * Holds if values stored inside content `c` are cleared at `n`. `n` is a
      * synthesized summary node, so in order for values to be cleared at calls
@@ -871,18 +908,28 @@ module Private {
       AccessPathRange() { relevantSpec(this) }
     }
 
-    /** Holds if specification component `c` parses as parameter `n`. */
+    /** Holds if specification component `token` parses as parameter `pos`. */
     predicate parseParam(AccessPathToken token, ArgumentPosition pos) {
       token.getName() = "Parameter" and
       pos = parseParamBody(token.getAnArgument())
     }
 
-    /** Holds if specification component `c` parses as argument `n`. */
+    /** Holds if specification component `token` parses as argument `pos`. */
     predicate parseArg(AccessPathToken token, ParameterPosition pos) {
       token.getName() = "Argument" and
       pos = parseArgBody(token.getAnArgument())
     }
 
+    /** Holds if specification component `token` parses as synthetic global `sg`. */
+    predicate parseSynthGlobal(AccessPathToken token, string sg) {
+      token.getName() = "SyntheticGlobal" and
+      sg = token.getAnArgument()
+    }
+
+    private class SyntheticGlobalFromAccessPath extends SummaryComponent::SyntheticGlobal {
+      SyntheticGlobalFromAccessPath() { parseSynthGlobal(_, this) }
+    }
+
     private SummaryComponent interpretComponent(AccessPathToken token) {
       exists(ParameterPosition pos |
         parseArg(token, pos) and result = SummaryComponent::argument(pos)
@@ -894,6 +941,10 @@ module Private {
       or
       token = "ReturnValue" and result = SummaryComponent::return(getReturnValueKind())
       or
+      exists(string sg |
+        parseSynthGlobal(token, sg) and result = SummaryComponent::syntheticGlobal(sg)
+      )
+      or
       result = interpretComponentSpecific(token)
     }
 

ruby/ql/lib/codeql/ruby/dataflow/internal/FlowSummaryImplSpecific.qll

@@ -44,6 +44,9 @@ DataFlowType getCallbackParameterType(DataFlowType t, ArgumentPosition pos) { an
  */
 DataFlowType getCallbackReturnType(DataFlowType t, ReturnKind rk) { any() }
 
+/** Gets the type of synthetic global `sg`. */
+DataFlowType getSyntheticGlobalType(SummaryComponent::SyntheticGlobal sg) { any() }
+
 /**
  * Holds if an external flow summary exists for `c` with input specification
  * `input`, output specification `output`, kind `kind`, and a flag `generated`

ruby/ql/lib/codeql/ruby/frameworks/Core.qll

@@ -14,6 +14,7 @@ import core.Hash
 import core.String
 import core.Regexp
 import core.IO
+import core.Digest
 
 /**
  * A system command executed via subshell literal syntax.

ruby/ql/lib/codeql/ruby/frameworks/core/Digest.qll

@@ -0,0 +1,34 @@
+/**
+ * Provides modeling for the `Digest` module.
+ */
+
+private import codeql.ruby.ApiGraphs
+private import codeql.ruby.Concepts
+private import codeql.ruby.DataFlow
+
+/** Gets an API node for a Digest class that hashes using `algo`. */
+private API::Node digest(Cryptography::HashingAlgorithm algo) {
+  exists(string name | result = API::getTopLevelMember("Digest").getMember(name) |
+    name = ["MD5", "SHA1", "SHA2", "RMD160"] and
+    algo.matchesName(name)
+  )
+}
+
+/** A call that hashes some input using a hashing algorithm from the `Digest` module. */
+private class DigestCall extends Cryptography::CryptographicOperation::Range instanceof DataFlow::CallNode {
+  Cryptography::HashingAlgorithm algo;
+
+  DigestCall() {
+    this = digest(algo).getAMethodCall(["hexdigest", "base64digest", "bubblebabble"])
+    or
+    this = digest(algo).getAMethodCall("file") // it's directly hashing the contents of a file, but that's close enough for us.
+    or
+    this = digest(algo).getInstance().getAMethodCall(["digest", "update", "<<"])
+  }
+
+  override Cryptography::HashingAlgorithm getAlgorithm() { result = algo }
+
+  override DataFlow::Node getAnInput() { result = super.getArgument(0) }
+
+  override Cryptography::BlockMode getBlockMode() { none() }
+}

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

@@ -581,3 +581,49 @@ private class CipherOperation extends Cryptography::CryptographicOperation::Rang
     result = cipherNode.getCipherMode().getBlockMode()
   }
 }
+
+/** Predicates and classes modeling the `OpenSSL::Digest` module */
+private module Digest {
+  private import codeql.ruby.ApiGraphs
+
+  /** A call that hashes some input using a hashing algorithm from the `OpenSSL::Digest` module. */
+  private class DigestCall extends Cryptography::CryptographicOperation::Range instanceof DataFlow::CallNode {
+    Cryptography::HashingAlgorithm algo;
+
+    DigestCall() {
+      exists(API::MethodAccessNode call |
+        call = API::getTopLevelMember("OpenSSL").getMember("Digest").getMethod("new")
+      |
+        this = call.getReturn().getAMethodCall(["digest", "update", "<<"]) and
+        algo.matchesName(call.getCallNode()
+              .getArgument(0)
+              .asExpr()
+              .getExpr()
+              .getConstantValue()
+              .getString())
+      )
+    }
+
+    override Cryptography::HashingAlgorithm getAlgorithm() { result = algo }
+
+    override DataFlow::Node getAnInput() { result = super.getArgument(0) }
+
+    override Cryptography::BlockMode getBlockMode() { none() }
+  }
+
+  /** A call to `OpenSSL::Digest.digest` that hashes input directly without constructing a digest instance. */
+  private class DigestCallDirect extends Cryptography::CryptographicOperation::Range instanceof DataFlow::CallNode {
+    Cryptography::HashingAlgorithm algo;
+
+    DigestCallDirect() {
+      this = API::getTopLevelMember("OpenSSL").getMember("Digest").getMethod("digest").getCallNode() and
+      algo.matchesName(this.getArgument(0).asExpr().getExpr().getConstantValue().getString())
+    }
+
+    override Cryptography::HashingAlgorithm getAlgorithm() { result = algo }
+
+    override DataFlow::Node getAnInput() { result = super.getArgument(1) }
+
+    override Cryptography::BlockMode getBlockMode() { none() }
+  }
+}

ruby/ql/test/library-tests/modules/modules.expected

@@ -1701,7 +1701,7 @@ enclosingModule
 | toplevel_self_singleton.rb:10:9:10:27 | self | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
 | toplevel_self_singleton.rb:12:5:12:7 | obj | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
 | toplevel_self_singleton.rb:12:5:12:12 | ... = ... | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
-| toplevel_self_singleton.rb:12:9:12:12 | (no string representation) | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
+| toplevel_self_singleton.rb:12:9:12:12 | self | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
 | toplevel_self_singleton.rb:13:5:15:7 | method_in_block | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
 | toplevel_self_singleton.rb:13:9:13:11 | obj | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
 | toplevel_self_singleton.rb:14:9:14:27 | call to ab_singleton_method | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
@@ -1716,7 +1716,7 @@ enclosingModule
 | toplevel_self_singleton.rb:18:29:18:32 | bar | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
 | toplevel_self_singleton.rb:18:35:22:1 | { ... } | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
 | toplevel_self_singleton.rb:19:5:21:7 | method_in_struct | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
-| toplevel_self_singleton.rb:19:9:19:12 | (no string representation) | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
+| toplevel_self_singleton.rb:19:9:19:12 | self | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
 | toplevel_self_singleton.rb:20:9:20:27 | call to ab_singleton_method | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
 | toplevel_self_singleton.rb:20:9:20:27 | self | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |
 | toplevel_self_singleton.rb:24:1:34:3 | Good | toplevel_self_singleton.rb:1:1:34:4 | toplevel_self_singleton.rb |

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

@@ -17,3 +17,13 @@
 | 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. |
 | broken_crypto.rb:79:1:79:35 | call to new | The cryptographic algorithm RC4 is broken or weak, and should not be used. |
+| broken_crypto.rb:81:1:81:28 | call to hexdigest | The cryptographic algorithm MD5 is broken or weak, and should not be used. |
+| broken_crypto.rb:84:1:84:31 | call to base64digest | The cryptographic algorithm MD5 is broken or weak, and should not be used. |
+| broken_crypto.rb:87:1:87:20 | call to digest | The cryptographic algorithm MD5 is broken or weak, and should not be used. |
+| broken_crypto.rb:89:1:89:21 | call to update | The cryptographic algorithm MD5 is broken or weak, and should not be used. |
+| broken_crypto.rb:90:1:90:17 | ... << ... | The cryptographic algorithm MD5 is broken or weak, and should not be used. |
+| broken_crypto.rb:95:1:95:34 | call to bubblebabble | The cryptographic algorithm MD5 is broken or weak, and should not be used. |
+| broken_crypto.rb:97:11:97:37 | call to file | The cryptographic algorithm MD5 is broken or weak, and should not be used. |
+| broken_crypto.rb:103:1:103:21 | call to digest | The cryptographic algorithm SHA1 is broken or weak, and should not be used. |
+| broken_crypto.rb:104:1:104:17 | ... << ... | The cryptographic algorithm SHA1 is broken or weak, and should not be used. |
+| broken_crypto.rb:106:1:106:37 | call to digest | The cryptographic algorithm SHA1 is broken or weak, and should not be used. |

ruby/ql/test/query-tests/security/cwe-327/broken_crypto.rb

@@ -77,3 +77,31 @@ OpenSSL::Cipher::RC4.new
 OpenSSL::Cipher::RC4.new '40'
 # BAD: weak encryption algorithm
 OpenSSL::Cipher::RC4.new 'hmac-md5'
+
+Digest::MD5.hexdigest('foo') # BAD: weak hash algorithm
+Digest::SHA256.hexdigest('foo') # GOOD: strong hash algorithm
+
+Digest::MD5.base64digest('foo') # BAD: weak hash algorithm
+
+md5 = Digest::MD5.new
+md5.digest 'message' # BAD: weak hash algorithm
+
+md5.update 'message1' # BAD: weak hash algorithm
+md5 << 'message2' # << is an alias for update
+
+sha256 = Digest::SHA256.new
+sha256.digest 'message' # GOOD: strong hash algorithm
+
+Digest::MD5.bubblebabble 'message' # BAD: weak hash algorithm
+
+filemd5 = Digest::MD5.file 'testfile'
+filemd5.hexdigest
+
+Digest("MD5").hexdigest('foo') # BAD: weak hash algorithm
+
+sha1 = OpenSSL::Digest.new('SHA1')
+sha1.digest 'message' # BAD: weak hash algorithm
+sha1 << 'message' # << is an alias for update
+
+OpenSSL::Digest.digest('SHA1', "abc") # BAD: weak hash algorithm
+OpenSSL::Digest.digest('SHA3-512', "abc") # GOOD: strong hash algorithm