Add rb/path-injection query

2026-05-03 12:45:27 +02:00 · 2021-10-14 11:12:03 +01:00
parent 3f396ac10e
commit 86da3c2db3
13 changed files with 386 additions and 0 deletions
--- a/ruby/ql/lib/codeql/ruby/Concepts.qll
+++ b/ruby/ql/lib/codeql/ruby/Concepts.qll
@@ -583,3 +583,21 @@ module OrmInstantiation {
    abstract predicate methodCallMayAccessField(string methodName);
  }
 }
+
+/** Provides classes for modeling path-related APIs. */
+module Path {
+  /**
+   * A data-flow node that performs path sanitization. This is often needed in order
+   * to safely access paths.
+   */
+  class PathSanitization extends DataFlow::Node instanceof PathSanitization::Range { }
+
+  /** Provides a class for modeling new path sanitization APIs. */
+  module PathSanitization {
+    /**
+     * A data-flow node that performs path sanitization. This is often needed in order
+     * to safely access paths.
+     */
+    abstract class Range extends DataFlow::Node { }
+  }
+}
--- a/ruby/ql/lib/codeql/ruby/Frameworks.qll
+++ b/ruby/ql/lib/codeql/ruby/Frameworks.qll
@@ -4,6 +4,7 @@

 private import codeql.ruby.frameworks.ActionController
 private import codeql.ruby.frameworks.ActiveRecord
+private import codeql.ruby.frameworks.ActiveStorage
 private import codeql.ruby.frameworks.ActionView
 private import codeql.ruby.frameworks.StandardLibrary
 private import codeql.ruby.frameworks.Files
--- a/ruby/ql/lib/codeql/ruby/dataflow/internal/FlowSummaryImplSpecific.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/FlowSummaryImplSpecific.qll
@@ -64,6 +64,9 @@ predicate summaryElement(DataFlowCallable c, string input, string output, string
 SummaryComponent interpretComponentSpecific(string c) {
  c = "BlockArgument" and
  result = FlowSummary::SummaryComponent::block()
+  or
+  c = "Argument[_]" and
+  result = FlowSummary::SummaryComponent::argument(any(int i | i >= 0))
 }

 /** Gets the return kind corresponding to specification `"ReturnValue"`. */
--- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveStorage.qll
@@ -0,0 +1,12 @@
+private import codeql.ruby.AST
+private import codeql.ruby.ApiGraphs
+private import codeql.ruby.Concepts
+private import codeql.ruby.DataFlow
+
+class ActiveStorageFilenameSanitizedCall extends Path::PathSanitization::Range, DataFlow::CallNode {
+  ActiveStorageFilenameSanitizedCall() {
+    this.getReceiver() =
+      API::getTopLevelMember("ActiveStorage").getMember("Filename").getAnInstantiation() and
+    this.asExpr().getExpr().(MethodCall).getMethodName() = "sanitized"
+  }
+}
--- a/ruby/ql/lib/codeql/ruby/frameworks/Files.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/Files.qll
@@ -7,6 +7,7 @@ private import codeql.ruby.Concepts
 private import codeql.ruby.ApiGraphs
 private import codeql.ruby.DataFlow
 private import codeql.ruby.frameworks.StandardLibrary
+private import codeql.ruby.dataflow.FlowSummary

 private DataFlow::Node ioInstanceInstantiation() {
  result = API::getTopLevelMember("IO").getAnInstantiation() or
@@ -253,6 +254,47 @@ module File {

    override DataFlow::Node getAPermissionNode() { result = permissionArg }
  }
+
+  /**
+   * Flow summary for several methods on the `File` class that propagate taint
+   * from their first argument to the return value.
+   */
+  class FilePathConversionSummary extends SummarizedCallable {
+    string methodName;
+
+    FilePathConversionSummary() {
+      methodName = ["absolute_path", "dirname", "expand_path", "path", "realdirpath", "realpath"] and
+      this = "File." + methodName
+    }
+
+    override MethodCall getACall() {
+      result = API::getTopLevelMember("File").getAMethodCall(methodName).asExpr().getExpr()
+    }
+
+    override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
+      input = "Argument[0]" and
+      output = "ReturnValue" and
+      preservesValue = false
+    }
+  }
+
+  /**
+   * Flow summary for `File.join`, which propagates taint from every argument to
+   * its return value.
+   */
+  class FileJoinSummary extends SummarizedCallable {
+    FileJoinSummary() { this = "File.join" }
+
+    override MethodCall getACall() {
+      result = API::getTopLevelMember("File").getAMethodCall("join").asExpr().getExpr()
+    }
+
+    override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
+      input = "Argument[_]" and
+      output = "ReturnValue" and
+      preservesValue = false
+    }
+  }
 }

 /**
--- a/ruby/ql/lib/codeql/ruby/security/PathInjectionCustomizations.qll
+++ b/ruby/ql/lib/codeql/ruby/security/PathInjectionCustomizations.qll
@@ -0,0 +1,54 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * path injection vulnerabilities, as well as extension points for
+ * adding your own.
+ */
+
+private import ruby
+private import codeql.ruby.ApiGraphs
+private import codeql.ruby.CFG
+private import codeql.ruby.Concepts
+private import codeql.ruby.DataFlow
+private import codeql.ruby.dataflow.BarrierGuards
+private import codeql.ruby.dataflow.RemoteFlowSources
+
+module PathInjection {
+  /**
+   * A data flow source for path injection vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for path injection vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer guard for path injection vulnerabilities.
+   */
+  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
+
+  /**
+   * A source of remote user input, considered as a flow source.
+   */
+  class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
+
+  /**
+   * A file system access, considered as a flow sink.
+   */
+  class FileSystemAccessAsSink extends Sink {
+    FileSystemAccessAsSink() { this = any(FileSystemAccess e).getAPathArgument() }
+  }
+
+  /**
+   * A comparison with a constant string, considered as a sanitizer-guard.
+   */
+  class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }
+
+  /**
+   * An inclusion check against an array of constant strings, considered as a
+   * sanitizer-guard.
+   */
+  class StringConstArrayInclusionCallAsSanitizerGuard extends SanitizerGuard,
+    StringConstArrayInclusionCall { }
+}
--- a/ruby/ql/lib/codeql/ruby/security/PathInjectionQuery.qll
+++ b/ruby/ql/lib/codeql/ruby/security/PathInjectionQuery.qll
@@ -0,0 +1,31 @@
+/**
+ * Provides a taint tracking configuration for reasoning about
+ * path injection vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if
+ * `PathInjection::Configuration` is needed, otherwise
+ * `PathInjectionCustomizations` should be imported instead.
+ */
+
+import PathInjectionCustomizations
+private import codeql.ruby.Concepts
+private import codeql.ruby.DataFlow
+private import codeql.ruby.TaintTracking
+
+/**
+ * A taint-tracking configuration for reasoning about path injection
+ * vulnerabilities.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "PathInjection" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof PathInjection::Source }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof PathInjection::Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof Path::PathSanitization }
+
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof PathInjection::SanitizerGuard
+  }
+}
--- a/ruby/ql/src/queries/security/cwe-022/PathInjection.qhelp
+++ b/ruby/ql/src/queries/security/cwe-022/PathInjection.qhelp
@@ -0,0 +1,61 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Accessing files using paths constructed from user-controlled data can allow an
+attacker to access unexpected resources. This can result in sensitive
+information being revealed or deleted, or an attacker being able to influence
+behavior by modifying unexpected files.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Validate user input before using it to construct a file path, either using an
+off-the-shelf library like <code>ActiveStorage::Filename#sanitized</code> in
+Rails, or by performing custom validation.
+</p>
+
+<p>
+Ideally, follow these rules:
+</p>
+
+<ul>
+<li>Do not allow more than a single "." character.</li>
+<li>Do not allow directory separators such as "/" or "\" (depending on the file
+system).</li>
+<li>Do not rely on simply replacing problematic sequences such as "../". For
+example, after applying this filter to ".../...//", the resulting string would
+still be "../".</li>
+<li>Use a whitelist of known good patterns.</li>
+</ul>
+</recommendation>
+
+<example>
+<p>
+In the first example, a file name is read from an HTTP request and then used to
+access a file.  However, a malicious user could enter a file name which is an
+absolute path, such as <code>"/etc/passwd"</code>.
+</p>
+
+<p>
+In the second example, it appears that the user is restricted to opening a file
+within the <code>"user"</code> home directory. However, a malicious user could
+enter a file name containing special characters. For example, the string
+<code>"../../etc/passwd"</code> will result in the code reading the file located
+at <code>"/home/user/../../etc/passwd"</code>, which is the system's password
+file. This file would then be sent back to the user, giving them access to all
+the system's passwords.
+</p>
+
+<sample src="examples/tainted_path.rb" />
+</example>
+
+<references>
+<li>OWASP: <a href="https://owasp.org/www-community/attacks/Path_Traversal">Path Traversal</a>.</li>
+<li>Rails: <a href="https://api.rubyonrails.org/classes/ActiveStorage/Filename.html#method-i-sanitized">ActiveStorage::Filename#sanitized</a>.</li>
+</references>
+</qhelp>
--- a/ruby/ql/src/queries/security/cwe-022/PathInjection.ql
+++ b/ruby/ql/src/queries/security/cwe-022/PathInjection.ql
@@ -0,0 +1,26 @@
+/**
+ * @name Uncontrolled data used in path expression
+ * @description Accessing paths influenced by users can allow an attacker to access
+ *              unexpected resources.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 7.5
+ * @precision high
+ * @id rb/path-injection
+ * @tags security
+ *       external/cwe/cwe-022
+ *       external/cwe/cwe-023
+ *       external/cwe/cwe-036
+ *       external/cwe/cwe-073
+ *       external/cwe/cwe-099
+ */
+
+import ruby
+import codeql.ruby.security.PathInjectionQuery
+import codeql.ruby.DataFlow
+import DataFlow::PathGraph
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "This path depends on $@.", source.getNode(),
+  "a user-provided value"
--- a/ruby/ql/src/queries/security/cwe-022/examples/tainted_path.rb
+++ b/ruby/ql/src/queries/security/cwe-022/examples/tainted_path.rb
@@ -0,0 +1,11 @@
+class FilesController < ActionController::Base
+  def first_example
+    # BAD: This could read any file on the file system
+    @content = File.read params[:path]
+  end
+
+  def second_example
+    # BAD: This could still read any file on the file system
+    @content = File.read "/home/user/#{ params[:path] }"
+  end
+end
--- a/ruby/ql/test/query-tests/security/cwe-022/PathInjection.expected
+++ b/ruby/ql/test/query-tests/security/cwe-022/PathInjection.expected
@@ -0,0 +1,64 @@
+edges
+| tainted_path.rb:4:12:4:17 | call to params :  | tainted_path.rb:5:26:5:29 | path |
+| tainted_path.rb:10:12:10:43 | call to absolute_path :  | tainted_path.rb:11:26:11:29 | path |
+| tainted_path.rb:10:31:10:36 | call to params :  | tainted_path.rb:10:31:10:43 | ...[...] :  |
+| tainted_path.rb:10:31:10:43 | ...[...] :  | tainted_path.rb:10:12:10:43 | call to absolute_path :  |
+| tainted_path.rb:16:15:16:41 | call to dirname :  | tainted_path.rb:17:26:17:29 | path |
+| tainted_path.rb:16:28:16:33 | call to params :  | tainted_path.rb:16:28:16:40 | ...[...] :  |
+| tainted_path.rb:16:28:16:40 | ...[...] :  | tainted_path.rb:16:15:16:41 | call to dirname :  |
+| tainted_path.rb:22:12:22:41 | call to expand_path :  | tainted_path.rb:23:26:23:29 | path |
+| tainted_path.rb:22:29:22:34 | call to params :  | tainted_path.rb:22:29:22:41 | ...[...] :  |
+| tainted_path.rb:22:29:22:41 | ...[...] :  | tainted_path.rb:22:12:22:41 | call to expand_path :  |
+| tainted_path.rb:28:12:28:34 | call to path :  | tainted_path.rb:29:26:29:29 | path |
+| tainted_path.rb:28:22:28:27 | call to params :  | tainted_path.rb:28:22:28:34 | ...[...] :  |
+| tainted_path.rb:28:22:28:34 | ...[...] :  | tainted_path.rb:28:12:28:34 | call to path :  |
+| tainted_path.rb:34:12:34:41 | call to realdirpath :  | tainted_path.rb:35:26:35:29 | path |
+| tainted_path.rb:34:29:34:34 | call to params :  | tainted_path.rb:34:29:34:41 | ...[...] :  |
+| tainted_path.rb:34:29:34:41 | ...[...] :  | tainted_path.rb:34:12:34:41 | call to realdirpath :  |
+| tainted_path.rb:40:12:40:38 | call to realpath :  | tainted_path.rb:41:26:41:29 | path |
+| tainted_path.rb:40:26:40:31 | call to params :  | tainted_path.rb:40:26:40:38 | ...[...] :  |
+| tainted_path.rb:40:26:40:38 | ...[...] :  | tainted_path.rb:40:12:40:38 | call to realpath :  |
+| tainted_path.rb:47:12:47:63 | call to join :  | tainted_path.rb:48:26:48:29 | path |
+| tainted_path.rb:47:43:47:48 | call to params :  | tainted_path.rb:47:43:47:55 | ...[...] :  |
+| tainted_path.rb:47:43:47:55 | ...[...] :  | tainted_path.rb:47:12:47:63 | call to join :  |
+nodes
+| tainted_path.rb:4:12:4:17 | call to params :  | semmle.label | call to params :  |
+| tainted_path.rb:5:26:5:29 | path | semmle.label | path |
+| tainted_path.rb:10:12:10:43 | call to absolute_path :  | semmle.label | call to absolute_path :  |
+| tainted_path.rb:10:31:10:36 | call to params :  | semmle.label | call to params :  |
+| tainted_path.rb:10:31:10:43 | ...[...] :  | semmle.label | ...[...] :  |
+| tainted_path.rb:11:26:11:29 | path | semmle.label | path |
+| tainted_path.rb:16:15:16:41 | call to dirname :  | semmle.label | call to dirname :  |
+| tainted_path.rb:16:28:16:33 | call to params :  | semmle.label | call to params :  |
+| tainted_path.rb:16:28:16:40 | ...[...] :  | semmle.label | ...[...] :  |
+| tainted_path.rb:17:26:17:29 | path | semmle.label | path |
+| tainted_path.rb:22:12:22:41 | call to expand_path :  | semmle.label | call to expand_path :  |
+| tainted_path.rb:22:29:22:34 | call to params :  | semmle.label | call to params :  |
+| tainted_path.rb:22:29:22:41 | ...[...] :  | semmle.label | ...[...] :  |
+| tainted_path.rb:23:26:23:29 | path | semmle.label | path |
+| tainted_path.rb:28:12:28:34 | call to path :  | semmle.label | call to path :  |
+| tainted_path.rb:28:22:28:27 | call to params :  | semmle.label | call to params :  |
+| tainted_path.rb:28:22:28:34 | ...[...] :  | semmle.label | ...[...] :  |
+| tainted_path.rb:29:26:29:29 | path | semmle.label | path |
+| tainted_path.rb:34:12:34:41 | call to realdirpath :  | semmle.label | call to realdirpath :  |
+| tainted_path.rb:34:29:34:34 | call to params :  | semmle.label | call to params :  |
+| tainted_path.rb:34:29:34:41 | ...[...] :  | semmle.label | ...[...] :  |
+| tainted_path.rb:35:26:35:29 | path | semmle.label | path |
+| tainted_path.rb:40:12:40:38 | call to realpath :  | semmle.label | call to realpath :  |
+| tainted_path.rb:40:26:40:31 | call to params :  | semmle.label | call to params :  |
+| tainted_path.rb:40:26:40:38 | ...[...] :  | semmle.label | ...[...] :  |
+| tainted_path.rb:41:26:41:29 | path | semmle.label | path |
+| tainted_path.rb:47:12:47:63 | call to join :  | semmle.label | call to join :  |
+| tainted_path.rb:47:43:47:48 | call to params :  | semmle.label | call to params :  |
+| tainted_path.rb:47:43:47:55 | ...[...] :  | semmle.label | ...[...] :  |
+| tainted_path.rb:48:26:48:29 | path | semmle.label | path |
+subpaths
+#select
+| tainted_path.rb:5:26:5:29 | path | tainted_path.rb:4:12:4:17 | call to params :  | tainted_path.rb:5:26:5:29 | path | This path depends on $@. | tainted_path.rb:4:12:4:17 | call to params | a user-provided value |
+| tainted_path.rb:11:26:11:29 | path | tainted_path.rb:10:31:10:36 | call to params :  | tainted_path.rb:11:26:11:29 | path | This path depends on $@. | tainted_path.rb:10:31:10:36 | call to params | a user-provided value |
+| tainted_path.rb:17:26:17:29 | path | tainted_path.rb:16:28:16:33 | call to params :  | tainted_path.rb:17:26:17:29 | path | This path depends on $@. | tainted_path.rb:16:28:16:33 | call to params | a user-provided value |
+| tainted_path.rb:23:26:23:29 | path | tainted_path.rb:22:29:22:34 | call to params :  | tainted_path.rb:23:26:23:29 | path | This path depends on $@. | tainted_path.rb:22:29:22:34 | call to params | a user-provided value |
+| tainted_path.rb:29:26:29:29 | path | tainted_path.rb:28:22:28:27 | call to params :  | tainted_path.rb:29:26:29:29 | path | This path depends on $@. | tainted_path.rb:28:22:28:27 | call to params | a user-provided value |
+| tainted_path.rb:35:26:35:29 | path | tainted_path.rb:34:29:34:34 | call to params :  | tainted_path.rb:35:26:35:29 | path | This path depends on $@. | tainted_path.rb:34:29:34:34 | call to params | a user-provided value |
+| tainted_path.rb:41:26:41:29 | path | tainted_path.rb:40:26:40:31 | call to params :  | tainted_path.rb:41:26:41:29 | path | This path depends on $@. | tainted_path.rb:40:26:40:31 | call to params | a user-provided value |
+| tainted_path.rb:48:26:48:29 | path | tainted_path.rb:47:43:47:48 | call to params :  | tainted_path.rb:48:26:48:29 | path | This path depends on $@. | tainted_path.rb:47:43:47:48 | call to params | a user-provided value |
--- a/ruby/ql/test/query-tests/security/cwe-022/PathInjection.qlref
+++ b/ruby/ql/test/query-tests/security/cwe-022/PathInjection.qlref
@@ -0,0 +1 @@
+queries/security/cwe-022/PathInjection.ql
--- a/ruby/ql/test/query-tests/security/cwe-022/tainted_path.rb
+++ b/ruby/ql/test/query-tests/security/cwe-022/tainted_path.rb
@@ -0,0 +1,62 @@
+class FooController < ActionController::Base
+  # BAD
+  def route0
+    path = params[:path]
+    @content = File.read path
+  end
+
+  # BAD - File.absolute_path preserves taint
+  def route1
+    path = File.absolute_path params[:path]
+    @content = File.read path
+  end
+
+  # BAD - File.dirname preserves taint
+  def route2
+    path = "#{File.dirname(params[:path])}/foo"
+    @content = File.read path
+  end
+
+  # BAD - File.expand_path preserves taint
+  def route3
+    path = File.expand_path params[:path]
+    @content = File.read path
+  end
+
+  # BAD - File.path preserves taint
+  def route4
+    path = File.path params[:path]
+    @content = File.read path
+  end
+
+  # BAD - File.realdirpath preserves taint
+  def route5
+    path = File.realdirpath params[:path]
+    @content = File.read path
+  end
+
+  # BAD - File.realpath preserves taint
+  def route6
+    path = File.realpath params[:path]
+    @content = File.read path
+  end
+
+  # BAD - tainted arguments in any position propagate to the return value of
+  # File.join
+  def route7
+    path = File.join("foo", "bar", "baz", params[:path], "qux")
+    @content = File.read path
+  end
+
+  # GOOD - File.basename does not preserve taint
+  def route8
+    path = File.basename params[:path]
+    @content = File.read path
+  end
+
+  # GOOD - explicitly sanitized
+  def route9
+    path = ActiveStorage::Filename.new(params[:path]).sanitized
+    @content = File.read path
+  end
+end