Ruby: Add rb/clear-text-storage-sensitive-data query

Author: alexrford

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

@@ -0,0 +1,57 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * cleartext storage of sensitive information, as well as extension points for
+ * adding your own.
+ */
+
+private import ruby
+private import codeql.ruby.DataFlow
+private import codeql.ruby.Concepts
+private import internal.CleartextSources
+
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * cleartext storage of sensitive information, as well as extension points for
+ * adding your own.
+ */
+module CleartextStorage {
+  /**
+   * A data flow source for cleartext storage of sensitive information.
+   */
+  class Source = CleartextSources::Source;
+
+  /**
+   * A sanitizer for cleartext storage of sensitive information.
+   */
+  class Sanitizer = CleartextSources::Sanitizer;
+
+  /** Holds if `nodeFrom` taints `nodeTo`. */
+  predicate isAdditionalTaintStep = CleartextSources::isAdditionalTaintStep/2;
+
+  /**
+   * A data flow sink for cleartext storage of sensitive information.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A node representing data written to the filesystem.
+   */
+  private class FileSystemWriteAccessDataNodeAsSink extends Sink {
+    FileSystemWriteAccessDataNodeAsSink() { this = any(FileSystemWriteAccess write).getADataNode() }
+  }
+
+  /**
+   * A node representing data written to a database using an ORM system.
+   */
+  private class OrmWriteAccessValueAsSink extends Sink {
+    //  instanceof OrmWriteAccess {
+    // TODO: we generally won't get flow from `value` to `this`
+    // Should the node be on value? Or should there be an additional flow step from
+    // value to the write node?
+    OrmWriteAccessValueAsSink() {
+      exists(OrmWriteAccess write, string fieldName |
+        fieldName = write.getFieldNameAssignedTo(this)
+      )
+    }
+  }
+}

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

@@ -0,0 +1,34 @@
+/**
+ * Provides a taint-tracking configuration for "Clear-text storage of sensitive information".
+ *
+ * Note, for performance reasons: only import this file if
+ * `Configuration` is needed, otherwise `CleartextStorageCustomizations` should be
+ * imported instead.
+ */
+
+private import ruby
+private import codeql.ruby.DataFlow
+private import codeql.ruby.TaintTracking
+import CleartextStorageCustomizations::CleartextStorage
+private import CleartextStorageCustomizations::CleartextStorage as CleartextStorage
+
+/**
+ * A taint-tracking configuration for detecting "Clear-text storage of sensitive information".
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "CleartextStorage" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof CleartextStorage::Source }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof CleartextStorage::Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    super.isSanitizer(node)
+    or
+    node instanceof CleartextStorage::Sanitizer
+  }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    CleartextStorage::isAdditionalTaintStep(nodeFrom, nodeTo)
+  }
+}

ruby/ql/src/queries/security/cwe-312/CleartextStorage.qhelp

@@ -0,0 +1,51 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Sensitive information that is stored unencrypted is accessible to an attacker
+who gains access to the storage.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Ensure that sensitive information is always encrypted before being stored.
+</p>
+<p>
+In general, decrypt sensitive information only at the point where it is
+necessary for it to be used in cleartext.
+</p>
+
+<p>
+
+Be aware that external processes often store the <code>standard
+out</code> and <code>standard error</code> streams of the application,
+causing logged sensitive information to be stored as well.
+
+</p>
+
+</recommendation>
+
+<example>
+<p>
+The following example code stores user credentials (in this case, their password)
+to disk in plain text:
+</p>
+<sample src="examples/CleartextStorageBad.rb"/>
+<p>
+Instead, the credentials should be masked or redacted before storing:
+</p>
+<sample src="examples/CleartextStorageGood.rb"/>
+</example>
+
+
+<references>
+
+<li>M. Dowd, J. McDonald and J. Schuhm, <i>The Art of Software Security Assessment</i>, 1st Edition, Chapter 2 - 'Common Vulnerabilities of Encryption', p. 43. Addison Wesley, 2006.</li>
+<li>M. Howard and D. LeBlanc, <i>Writing Secure Code</i>, 2nd Edition, Chapter 9 - 'Protecting Secret Data', p. 299. Microsoft, 2002.</li>
+
+</references>
+</qhelp>

ruby/ql/src/queries/security/cwe-312/CleartextStorage.ql

@@ -0,0 +1,24 @@
+/**
+ * @name Clear-text storage of sensitive information
+ * @description Storing sensitive information without encryption or hashing can
+ *              expose it to an attacker.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 7.5
+ * @precision high
+ * @id rb/clear-text-storage-sensitive-data
+ * @tags security
+ *       external/cwe/cwe-312
+ *       external/cwe/cwe-359
+ *       external/cwe/cwe-532
+ */
+
+import ruby
+import codeql.ruby.security.CleartextStorageQuery
+import codeql.ruby.DataFlow
+import DataFlow::PathGraph
+
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Sensitive data returned by $@ is stored here.",
+  source.getNode(), source.getNode().(Source).describe()

ruby/ql/src/queries/security/cwe-312/examples/CleartextStorageBad.rb

@@ -0,0 +1,7 @@
+class UserSession
+  def login(username, password)
+    # ...
+    logfile = File.open("login_attempts.log")
+    logfile.puts "login with password: #{password})"
+  end
+end

ruby/ql/src/queries/security/cwe-312/examples/CleartextStorageGood.rb

@@ -0,0 +1,8 @@
+class UserSession
+  def login(username, password)
+    # ...
+    password_escaped = password.sub(/.*/, "[redacted]")
+    logfile = File.open("login_attempts.log")
+    logfile.puts "login with password: #{password_escaped})"
+  end
+end