Merge pull request github#11581 from erik-krogh/stdin

Rb: add stdin as source for unsafe-deserialization

Author: erik-krogh

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

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

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

@@ -0,0 +1,25 @@
+/**
+ * Provides modeling for the `Base64` module.
+ */
+
+private import ruby
+private import codeql.ruby.dataflow.FlowSummary
+private import codeql.ruby.ApiGraphs
+
+private class Base64Decode extends SummarizedCallable {
+  Base64Decode() { this = "Base64.decode64()" }
+
+  override MethodCall getACall() {
+    result =
+      API::getTopLevelMember("Base64")
+          .getAMethodCall(["decode64", "strict_decode64", "urlsafe_decode64"])
+          .asExpr()
+          .getExpr()
+  }
+
+  override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
+    input = "Argument[0]" and
+    output = "ReturnValue" and
+    preservesValue = false
+  }
+}

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

@@ -10,12 +10,16 @@ private import codeql.ruby.DataFlow
 private import codeql.ruby.dataflow.RemoteFlowSources
 private import codeql.ruby.frameworks.ActiveJob
 private import codeql.ruby.frameworks.core.Module
+private import codeql.ruby.frameworks.core.Kernel
 
 module UnsafeDeserialization {
   /**
    * A data flow source for unsafe deserialization vulnerabilities.
    */
-  abstract class Source extends DataFlow::Node { }
+  abstract class Source extends DataFlow::Node {
+    /** Gets a string that describes the source. */
+    string describe() { result = "user-provided value" }
+  }
 
   /**
    * A data flow sink for unsafe deserialization vulnerabilities.
@@ -27,16 +31,39 @@ module UnsafeDeserialization {
    */
   abstract class Sanitizer extends DataFlow::Node { }
 
-  /**
-   * Additional taint steps for "unsafe deserialization" vulnerabilities.
-   */
-  predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
-    base64DecodeTaintStep(fromNode, toNode)
-  }
-
   /** A source of remote user input, considered as a flow source for unsafe deserialization. */
   class RemoteFlowSourceAsSource extends Source instanceof RemoteFlowSource { }
 
+  /** A read of data from `STDIN`/`ARGV`, considered as a flow source for unsafe deserialization. */
+  class StdInSource extends UnsafeDeserialization::Source {
+    boolean stdin;
+
+    StdInSource() {
+      this = API::getTopLevelMember(["STDIN", "ARGF"]).getAMethodCall(["gets", "read"]) and
+      stdin = true
+      or
+      // > $stdin == STDIN
+      // => true
+      // but $stdin is special in that it is a global variable and not a constant. `API::getTopLevelMember` only gets constants.
+      exists(DataFlow::Node dollarStdin |
+        dollarStdin.asExpr().getExpr().(GlobalVariableReadAccess).getVariable().getName() = "$stdin" and
+        this = dollarStdin.getALocalSource().getAMethodCall(["gets", "read"])
+      ) and
+      stdin = true
+      or
+      // ARGV.
+      this.asExpr().getExpr().(GlobalVariableReadAccess).getVariable().getName() = "ARGV" and
+      stdin = false
+      or
+      this.(Kernel::KernelMethodCall).getMethodName() = ["gets", "readline", "readlines"] and
+      stdin = true
+    }
+
+    override string describe() {
+      if stdin = true then result = "value from stdin" else result = "value from ARGV"
+    }
+  }
+
   /**
    * An argument in a call to `Marshal.load` or `Marshal.restore`, considered a
    * sink for unsafe deserialization.
@@ -181,41 +208,4 @@ module UnsafeDeserialization {
       )
     }
   }
-
-  /**
-   * `Base64.decode64` propagates taint from its argument to its return value.
-   */
-  predicate base64DecodeTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
-    exists(DataFlow::CallNode callNode |
-      callNode =
-        API::getTopLevelMember("Base64")
-            .getAMethodCall(["decode64", "strict_decode64", "urlsafe_decode64"])
-    |
-      fromNode = callNode.getArgument(0) and
-      toNode = callNode
-    )
-  }
-
-  /**
-   * A argument in a call to `Module.const_get`, considered as a sink for unsafe
-   * deserialization.
-   *
-   * Calls to `Module.const_get` can return arbitrary classes which can then be
-   * instantiated.
-   */
-  class ConstGetCallArgument extends Sink {
-    ConstGetCallArgument() { this = any(Module::ModuleConstGetCallCodeExecution c).getCode() }
-  }
-
-  /**
-   * A argument in a call to `ActiveJob::Serializers.deserialize`, considered as
-   * a sink for unsafe deserialization.
-   *
-   * This is roughly equivalent to a call to `Module.const_get`.
-   */
-  class ActiveJobSerializersDeserializeArgument extends Sink {
-    ActiveJobSerializersDeserializeArgument() {
-      this = any(ActiveJob::Serializers::DeserializeCall c).getCode()
-    }
-  }
 }

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

@@ -27,8 +27,4 @@ class Configuration extends TaintTracking::Configuration {
     super.isSanitizer(node) or
     node instanceof UnsafeDeserialization::Sanitizer
   }
-
-  override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
-    UnsafeDeserialization::isAdditionalTaintStep(fromNode, toNode)
-  }
 }

ruby/ql/src/change-notes/2023-01-06-stdin-as-deserialization-source.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `rb/unsafe-deserialization` query now recognizes input from STDIN as a source.

ruby/ql/src/queries/security/cwe-502/UnsafeDeserialization.ql

@@ -11,12 +11,11 @@
  *       external/cwe/cwe-502
  */
 
-import codeql.ruby.AST
-import DataFlow::PathGraph
-import codeql.ruby.DataFlow
+import ruby
 import codeql.ruby.security.UnsafeDeserializationQuery
+import DataFlow::PathGraph
 
 from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
 where cfg.hasFlowPath(source, sink)
 select sink.getNode(), source, sink, "Unsafe deserialization depends on a $@.", source.getNode(),
-  "user-provided value"
+  source.getNode().(UnsafeDeserialization::Source).describe()

ruby/ql/test/library-tests/dataflow/local/TaintStep.expected

@@ -2804,6 +2804,7 @@
 | file://:0:0:0:0 | parameter position 0 of ActionController::Parameters#merge! | file://:0:0:0:0 | [summary] to write: argument self in ActionController::Parameters#merge! |
 | file://:0:0:0:0 | parameter position 0 of ActionController::Parameters#merge! | file://:0:0:0:0 | [summary] to write: return (return) in ActionController::Parameters#merge! |
 | file://:0:0:0:0 | parameter position 0 of Arel.sql | file://:0:0:0:0 | [summary] to write: return (return) in Arel.sql |
+| file://:0:0:0:0 | parameter position 0 of Base64.decode64() | file://:0:0:0:0 | [summary] to write: return (return) in Base64.decode64() |
 | file://:0:0:0:0 | parameter position 0 of File.absolute_path | file://:0:0:0:0 | [summary] to write: return (return) in File.absolute_path |
 | file://:0:0:0:0 | parameter position 0 of File.dirname | file://:0:0:0:0 | [summary] to write: return (return) in File.dirname |
 | file://:0:0:0:0 | parameter position 0 of File.expand_path | file://:0:0:0:0 | [summary] to write: return (return) in File.expand_path |

ruby/ql/test/query-tests/security/cwe-502/unsafe-deserialization/UnsafeDeserialization.expected

@@ -1,8 +1,10 @@
 edges
+| UnsafeDeserialization.rb:10:23:10:50 | call to decode64 :  | UnsafeDeserialization.rb:11:27:11:41 | serialized_data |
 | UnsafeDeserialization.rb:10:39:10:44 | call to params :  | UnsafeDeserialization.rb:10:39:10:50 | ...[...] :  |
-| UnsafeDeserialization.rb:10:39:10:50 | ...[...] :  | UnsafeDeserialization.rb:11:27:11:41 | serialized_data |
+| UnsafeDeserialization.rb:10:39:10:50 | ...[...] :  | UnsafeDeserialization.rb:10:23:10:50 | call to decode64 :  |
+| UnsafeDeserialization.rb:16:23:16:50 | call to decode64 :  | UnsafeDeserialization.rb:17:30:17:44 | serialized_data |
 | UnsafeDeserialization.rb:16:39:16:44 | call to params :  | UnsafeDeserialization.rb:16:39:16:50 | ...[...] :  |
-| UnsafeDeserialization.rb:16:39:16:50 | ...[...] :  | UnsafeDeserialization.rb:17:30:17:44 | serialized_data |
+| UnsafeDeserialization.rb:16:39:16:50 | ...[...] :  | UnsafeDeserialization.rb:16:23:16:50 | call to decode64 :  |
 | UnsafeDeserialization.rb:22:17:22:22 | call to params :  | UnsafeDeserialization.rb:22:17:22:28 | ...[...] :  |
 | UnsafeDeserialization.rb:22:17:22:28 | ...[...] :  | UnsafeDeserialization.rb:23:24:23:32 | json_data |
 | UnsafeDeserialization.rb:28:17:28:22 | call to params :  | UnsafeDeserialization.rb:28:17:28:28 | ...[...] :  |
@@ -18,12 +20,12 @@ edges
 | UnsafeDeserialization.rb:81:11:81:22 | ...[...] :  | UnsafeDeserialization.rb:82:34:82:36 | xml |
 | UnsafeDeserialization.rb:87:17:87:22 | call to params :  | UnsafeDeserialization.rb:87:17:87:28 | ...[...] :  |
 | UnsafeDeserialization.rb:87:17:87:28 | ...[...] :  | UnsafeDeserialization.rb:88:25:88:33 | yaml_data |
-| UnsafeDeserialization.rb:93:30:93:35 | call to params :  | UnsafeDeserialization.rb:93:30:93:43 | ...[...] |
-| UnsafeDeserialization.rb:99:48:99:53 | call to params :  | UnsafeDeserialization.rb:99:48:99:61 | ...[...] |
 nodes
+| UnsafeDeserialization.rb:10:23:10:50 | call to decode64 :  | semmle.label | call to decode64 :  |
 | UnsafeDeserialization.rb:10:39:10:44 | call to params :  | semmle.label | call to params :  |
 | UnsafeDeserialization.rb:10:39:10:50 | ...[...] :  | semmle.label | ...[...] :  |
 | UnsafeDeserialization.rb:11:27:11:41 | serialized_data | semmle.label | serialized_data |
+| UnsafeDeserialization.rb:16:23:16:50 | call to decode64 :  | semmle.label | call to decode64 :  |
 | UnsafeDeserialization.rb:16:39:16:44 | call to params :  | semmle.label | call to params :  |
 | UnsafeDeserialization.rb:16:39:16:50 | ...[...] :  | semmle.label | ...[...] :  |
 | UnsafeDeserialization.rb:17:30:17:44 | serialized_data | semmle.label | serialized_data |
@@ -49,10 +51,11 @@ nodes
 | UnsafeDeserialization.rb:87:17:87:22 | call to params :  | semmle.label | call to params :  |
 | UnsafeDeserialization.rb:87:17:87:28 | ...[...] :  | semmle.label | ...[...] :  |
 | UnsafeDeserialization.rb:88:25:88:33 | yaml_data | semmle.label | yaml_data |
-| UnsafeDeserialization.rb:93:30:93:35 | call to params :  | semmle.label | call to params :  |
-| UnsafeDeserialization.rb:93:30:93:43 | ...[...] | semmle.label | ...[...] |
-| UnsafeDeserialization.rb:99:48:99:53 | call to params :  | semmle.label | call to params :  |
-| UnsafeDeserialization.rb:99:48:99:61 | ...[...] | semmle.label | ...[...] |
+| UnsafeDeserialization.rb:92:24:92:34 | call to read | semmle.label | call to read |
+| UnsafeDeserialization.rb:95:24:95:33 | call to gets | semmle.label | call to gets |
+| UnsafeDeserialization.rb:98:24:98:32 | call to read | semmle.label | call to read |
+| UnsafeDeserialization.rb:101:24:101:27 | call to gets | semmle.label | call to gets |
+| UnsafeDeserialization.rb:104:24:104:32 | call to readlines | semmle.label | call to readlines |
 subpaths
 #select
 | UnsafeDeserialization.rb:11:27:11:41 | serialized_data | UnsafeDeserialization.rb:10:39:10:44 | call to params :  | UnsafeDeserialization.rb:11:27:11:41 | serialized_data | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:10:39:10:44 | call to params | user-provided value |
@@ -65,5 +68,8 @@ subpaths
 | UnsafeDeserialization.rb:69:23:69:31 | json_data | UnsafeDeserialization.rb:59:17:59:22 | call to params :  | UnsafeDeserialization.rb:69:23:69:31 | json_data | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:59:17:59:22 | call to params | user-provided value |
 | UnsafeDeserialization.rb:82:34:82:36 | xml | UnsafeDeserialization.rb:81:11:81:16 | call to params :  | UnsafeDeserialization.rb:82:34:82:36 | xml | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:81:11:81:16 | call to params | user-provided value |
 | UnsafeDeserialization.rb:88:25:88:33 | yaml_data | UnsafeDeserialization.rb:87:17:87:22 | call to params :  | UnsafeDeserialization.rb:88:25:88:33 | yaml_data | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:87:17:87:22 | call to params | user-provided value |
-| UnsafeDeserialization.rb:93:30:93:43 | ...[...] | UnsafeDeserialization.rb:93:30:93:35 | call to params :  | UnsafeDeserialization.rb:93:30:93:43 | ...[...] | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:93:30:93:35 | call to params | user-provided value |
-| UnsafeDeserialization.rb:99:48:99:61 | ...[...] | UnsafeDeserialization.rb:99:48:99:53 | call to params :  | UnsafeDeserialization.rb:99:48:99:61 | ...[...] | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:99:48:99:53 | call to params | user-provided value |
+| UnsafeDeserialization.rb:92:24:92:34 | call to read | UnsafeDeserialization.rb:92:24:92:34 | call to read | UnsafeDeserialization.rb:92:24:92:34 | call to read | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:92:24:92:34 | call to read | value from stdin |
+| UnsafeDeserialization.rb:95:24:95:33 | call to gets | UnsafeDeserialization.rb:95:24:95:33 | call to gets | UnsafeDeserialization.rb:95:24:95:33 | call to gets | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:95:24:95:33 | call to gets | value from stdin |
+| UnsafeDeserialization.rb:98:24:98:32 | call to read | UnsafeDeserialization.rb:98:24:98:32 | call to read | UnsafeDeserialization.rb:98:24:98:32 | call to read | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:98:24:98:32 | call to read | value from stdin |
+| UnsafeDeserialization.rb:101:24:101:27 | call to gets | UnsafeDeserialization.rb:101:24:101:27 | call to gets | UnsafeDeserialization.rb:101:24:101:27 | call to gets | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:101:24:101:27 | call to gets | value from stdin |
+| UnsafeDeserialization.rb:104:24:104:32 | call to readlines | UnsafeDeserialization.rb:104:24:104:32 | call to readlines | UnsafeDeserialization.rb:104:24:104:32 | call to readlines | Unsafe deserialization depends on a $@. | UnsafeDeserialization.rb:104:24:104:32 | call to readlines | value from stdin |

ruby/ql/test/query-tests/security/cwe-502/unsafe-deserialization/UnsafeDeserialization.rb

@@ -88,15 +88,19 @@ def route11
     object = Psych.load yaml_data
   end
 
-  # BAD - user input determines which class is instantiated
-  def route12
-    klass = Module.const_get(params[:class])
-    object = klass.new
-  end
+  def stdin
+    object = YAML.load $stdin.read
+
+    # STDIN
+    object = YAML.load STDIN.gets
+
+    # ARGF
+    object = YAML.load ARGF.read
+
+    # Kernel.gets
+    object = YAML.load gets
 
-  # BAD - user input determines which class is instantiated
-  def route13
-    klass = ActiveJob::Serializers.deserialize(params[:class])
-    object = klass.new
+    # Kernel.readlines
+    object = YAML.load readlines
   end
 end