Ruby: Implement new disablesCertificateValidation for RestClient

Author: RasmusWL

ruby/ql/lib/codeql/ruby/frameworks/http_clients/RestClient.qll

@@ -7,6 +7,7 @@ private import codeql.ruby.CFG
 private import codeql.ruby.Concepts
 private import codeql.ruby.ApiGraphs
 private import codeql.ruby.DataFlow
+private import codeql.ruby.dataflow.internal.DataFlowImplForLibraries as DataFlowImplForLibraries
 
 /**
  * A call that makes an HTTP request using `RestClient`.
@@ -46,57 +47,50 @@ class RestClientHttpRequest extends HTTP::Client::Request::Range, DataFlow::Call
 
   override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") }
 
-  override predicate disablesCertificateValidation(DataFlow::Node disablingNode) {
+  /** Gets the value that controls certificate validation, if any. */
+  DataFlow::Node getCertificateValidationControllingValue() {
     // `RestClient::Resource::new` takes an options hash argument, and we're
     // looking for `{ verify_ssl: OpenSSL::SSL::VERIFY_NONE }`.
-    exists(DataFlow::Node arg, int i |
-      i > 0 and
-      arg = connectionNode.getAValueReachableFromSource().(DataFlow::CallNode).getArgument(i)
-    |
-      // Either passed as an individual key:value argument, e.g.:
-      // RestClient::Resource.new(..., verify_ssl: OpenSSL::SSL::VERIFY_NONE)
-      isVerifySslNonePair(arg.asExpr()) and
-      disablingNode = arg
+    exists(DataFlow::CallNode newCall | newCall = connectionNode.getAValueReachableFromSource() |
+      result = newCall.getKeywordArgument("verify_ssl")
       or
-      // Or as a single hash argument, e.g.:
-      // RestClient::Resource.new(..., { verify_ssl: OpenSSL::SSL::VERIFY_NONE })
-      exists(DataFlow::LocalSourceNode optionsNode, CfgNodes::ExprNodes::PairCfgNode p |
+      // using a hashliteral
+      exists(
+        DataFlow::LocalSourceNode optionsNode, CfgNodes::ExprNodes::PairCfgNode p,
+        DataFlow::Node key
+      |
+        // can't flow to argument 0, since that's the URL
+        optionsNode.flowsTo(newCall.getArgument(any(int i | i > 0))) and
         p = optionsNode.asExpr().(CfgNodes::ExprNodes::HashLiteralCfgNode).getAKeyValuePair() and
-        isVerifySslNonePair(p) and
-        optionsNode.flowsTo(arg) and
-        disablingNode.asExpr() = p
+        key.asExpr() = p.getKey() and
+        key.getALocalSource().asExpr().getExpr().getConstantValue().isStringlikeValue("verify_ssl") and
+        result.asExpr() = p.getValue()
       )
     )
   }
 
   override predicate disablesCertificateValidation(
     DataFlow::Node disablingNode, DataFlow::Node argumentOrigin
   ) {
-    disablesCertificateValidation(disablingNode) and
-    argumentOrigin = disablingNode
+    any(RestClientDisablesCertificateValidationConfiguration config)
+        .hasFlow(argumentOrigin, disablingNode) and
+    disablingNode = this.getCertificateValidationControllingValue()
   }
 
   override string getFramework() { result = "RestClient" }
 }
 
-/** Holds if `p` is the pair `verify_ssl: OpenSSL::SSL::VERIFY_NONE`. */
-private predicate isVerifySslNonePair(CfgNodes::ExprNodes::PairCfgNode p) {
-  exists(DataFlow::Node key, DataFlow::Node value |
-    key.asExpr() = p.getKey() and
-    value.asExpr() = p.getValue() and
-    isSslVerifyModeLiteral(key) and
-    value =
-      API::getTopLevelMember("OpenSSL")
-          .getMember("SSL")
-          .getMember("VERIFY_NONE")
-          .getAValueReachableFromSource()
-  )
-}
+/** A configuration to track values that can disable certificate validation for RestClient. */
+private class RestClientDisablesCertificateValidationConfiguration extends DataFlowImplForLibraries::Configuration {
+  RestClientDisablesCertificateValidationConfiguration() {
+    this = "RestClientDisablesCertificateValidationConfiguration"
+  }
 
-/** Holds if `node` can represent the symbol literal `:verify_ssl`. */
-private predicate isSslVerifyModeLiteral(DataFlow::Node node) {
-  exists(DataFlow::LocalSourceNode literal |
-    literal.asExpr().getExpr().getConstantValue().isStringlikeValue("verify_ssl") and
-    literal.flowsTo(node)
-  )
+  override predicate isSource(DataFlow::Node source) {
+    source = API::getTopLevelMember("OpenSSL").getMember("SSL").getMember("VERIFY_NONE").asSource()
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink = any(RestClientHttpRequest req).getCertificateValidationControllingValue()
+  }
 }

ruby/ql/test/query-tests/security/cwe-295/RequestWithoutValidation.expected

@@ -19,9 +19,9 @@
 | OpenURI.rb:14:1:14:81 | call to open | This request may run without certificate validation because it is $@. | OpenURI.rb:14:39:14:80 | Pair | disabled here | OpenURI.rb:14:39:14:80 | Pair | here |
 | OpenURI.rb:17:1:17:85 | call to open | This request may run without certificate validation because it is $@. | OpenURI.rb:17:41:17:82 | Pair | disabled here | OpenURI.rb:17:41:17:82 | Pair | here |
 | OpenURI.rb:21:1:21:46 | call to open | This request may run without certificate validation because it is $@. | OpenURI.rb:20:13:20:54 | Pair | disabled here | OpenURI.rb:20:13:20:54 | Pair | here |
-| RestClient.rb:5:12:5:23 | call to get | This request may run without certificate validation because it is $@. | RestClient.rb:4:60:4:96 | Pair | disabled here | RestClient.rb:4:60:4:96 | Pair | here |
-| RestClient.rb:9:12:9:23 | call to get | This request may run without certificate validation because it is $@. | RestClient.rb:8:62:8:98 | Pair | disabled here | RestClient.rb:8:62:8:98 | Pair | here |
-| RestClient.rb:14:12:14:23 | call to get | This request may run without certificate validation because it is $@. | RestClient.rb:12:13:12:49 | Pair | disabled here | RestClient.rb:12:13:12:49 | Pair | here |
-| RestClient.rb:19:12:19:23 | call to get | This request may run without certificate validation because it is $@. | RestClient.rb:18:60:18:76 | Pair | disabled here | RestClient.rb:18:60:18:76 | Pair | here |
+| RestClient.rb:5:12:5:23 | call to get | This request may run without certificate validation because it is $@. | RestClient.rb:4:72:4:96 | VERIFY_NONE | disabled here | RestClient.rb:4:72:4:96 | VERIFY_NONE | here |
+| RestClient.rb:9:12:9:23 | call to get | This request may run without certificate validation because it is $@. | RestClient.rb:8:74:8:98 | VERIFY_NONE | disabled here | RestClient.rb:8:74:8:98 | VERIFY_NONE | here |
+| RestClient.rb:14:12:14:23 | call to get | This request may run without certificate validation because it is $@. | RestClient.rb:12:25:12:49 | VERIFY_NONE | disabled here | RestClient.rb:12:25:12:49 | VERIFY_NONE | here |
+| RestClient.rb:19:12:19:23 | call to get | This request may run without certificate validation because it is $@ by the value from $@. | RestClient.rb:18:72:18:76 | value | disabled here | RestClient.rb:17:9:17:33 | VERIFY_NONE | here |
 | Typhoeus.rb:4:1:4:62 | call to get | This request may run without certificate validation because it is $@. | Typhoeus.rb:4:41:4:61 | Pair | disabled here | Typhoeus.rb:4:41:4:61 | Pair | here |
 | Typhoeus.rb:8:1:8:54 | call to post | This request may run without certificate validation because it is $@. | Typhoeus.rb:7:37:7:57 | Pair | disabled here | Typhoeus.rb:7:37:7:57 | Pair | here |