Merge pull request #8395 from alexrford/ruby/clear-text-storage

Ruby: add `rb/clear-text-storage-sensitive-data` query

Author: alexrford

javascript/ql/lib/semmle/javascript/security/internal/SensitiveDataHeuristics.qll

@@ -98,7 +98,8 @@ module HeuristicNames {
    * suggesting nouns within the string do not represent the meaning of the whole string (e.g. a URL or a SQL query).
    */
   string notSensitiveRegexp() {
-    result = "(?is).*([^\\w$.-]|redact|censor|obfuscate|hash|md5|sha|((?<!un)(en))?(crypt|code)).*"
+    result =
+      "(?is).*([^\\w$.-]|redact|censor|obfuscate|hash|md5|sha|random|((?<!un)(en))?(crypt|code)).*"
   }
 
   /**

python/ql/lib/semmle/python/security/internal/SensitiveDataHeuristics.qll

@@ -98,7 +98,8 @@ module HeuristicNames {
    * suggesting nouns within the string do not represent the meaning of the whole string (e.g. a URL or a SQL query).
    */
   string notSensitiveRegexp() {
-    result = "(?is).*([^\\w$.-]|redact|censor|obfuscate|hash|md5|sha|((?<!un)(en))?(crypt|code)).*"
+    result =
+      "(?is).*([^\\w$.-]|redact|censor|obfuscate|hash|md5|sha|random|((?<!un)(en))?(crypt|code)).*"
   }
 
   /**

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

@@ -6,12 +6,8 @@
 
 private import ruby
 private import codeql.ruby.DataFlow
-private import codeql.ruby.TaintTracking::TaintTracking
 private import codeql.ruby.Concepts
-private import codeql.ruby.dataflow.RemoteFlowSources
-private import internal.SensitiveDataHeuristics::HeuristicNames
-private import codeql.ruby.CFG
-private import codeql.ruby.dataflow.SSA
+private import internal.CleartextSources
 
 /**
  * Provides default sources, sinks and sanitizers for reasoning about
@@ -22,265 +18,25 @@ module CleartextLogging {
   /**
    * A data flow source for cleartext logging of sensitive information.
    */
-  abstract class Source extends DataFlow::Node {
-    /** Gets a string that describes the type of this data flow source. */
-    abstract string describe();
-  }
-
-  /**
-   * A data flow sink for cleartext logging of sensitive information.
-   */
-  abstract class Sink extends DataFlow::Node { }
+  class Source = CleartextSources::Source;
 
   /**
    * A sanitizer for cleartext logging of sensitive information.
    */
-  abstract class Sanitizer extends DataFlow::Node { }
+  class Sanitizer = CleartextSources::Sanitizer;
 
-  /**
-   * Holds if `re` may be a regular expression that can be used to sanitize
-   * sensitive data with a call to `sub`.
-   */
-  private predicate effectiveSubRegExp(CfgNodes::ExprNodes::RegExpLiteralCfgNode re) {
-    re.getConstantValue().getStringOrSymbol().matches([".*", ".+"])
-  }
-
-  /**
-   * Holds if `re` may be a regular expression that can be used to sanitize
-   * sensitive data with a call to `gsub`.
-   */
-  private predicate effectiveGsubRegExp(CfgNodes::ExprNodes::RegExpLiteralCfgNode re) {
-    re.getConstantValue().getStringOrSymbol().matches(".")
-  }
-
-  /**
-   * A call to `sub`/`sub!` or `gsub`/`gsub!` that seems to mask sensitive information.
-   */
-  private class MaskingReplacerSanitizer extends Sanitizer, DataFlow::CallNode {
-    MaskingReplacerSanitizer() {
-      exists(CfgNodes::ExprNodes::RegExpLiteralCfgNode re |
-        re = this.getArgument(0).asExpr() and
-        (
-          this.getMethodName() = ["sub", "sub!"] and effectiveSubRegExp(re)
-          or
-          this.getMethodName() = ["gsub", "gsub!"] and effectiveGsubRegExp(re)
-        )
-      )
-    }
-  }
-
-  /**
-   * Like `MaskingReplacerSanitizer` but updates the receiver for methods that
-   * sanitize the receiver.
-   * Taint is thereby cleared for any subsequent read.
-   */
-  private class InPlaceMaskingReplacerSanitizer extends Sanitizer {
-    InPlaceMaskingReplacerSanitizer() {
-      exists(MaskingReplacerSanitizer m | m.getMethodName() = ["gsub!", "sub!"] |
-        m.getReceiver() = this
-      )
-    }
-  }
-
-  /**
-   * Holds if `name` is for a method or variable that appears, syntactically, to
-   * not be sensitive.
-   */
-  bindingset[name]
-  private predicate nameIsNotSensitive(string name) {
-    name.regexpMatch(notSensitiveRegexp()) and
-    // By default `notSensitiveRegexp()` includes some false positives for
-    // common ruby method names that are not necessarily non-sensitive.
-    // We explicitly exclude element references, element assignments, and
-    // mutation methods.
-    not name = ["[]", "[]="] and
-    not name.matches("%!")
-  }
-
-  /**
-   * A call that might obfuscate a password, for example through hashing.
-   */
-  private class ObfuscatorCall extends Sanitizer, DataFlow::CallNode {
-    ObfuscatorCall() { nameIsNotSensitive(this.getMethodName()) }
-  }
-
-  /**
-   * A data flow node that does not contain a clear-text password, according to its syntactic name.
-   */
-  private class NameGuidedNonCleartextPassword extends NonCleartextPassword {
-    NameGuidedNonCleartextPassword() {
-      exists(string name | nameIsNotSensitive(name) |
-        // accessing a non-sensitive variable
-        this.asExpr().getExpr().(VariableReadAccess).getVariable().getName() = name
-        or
-        // dereferencing a non-sensitive field
-        this.asExpr()
-            .(CfgNodes::ExprNodes::ElementReferenceCfgNode)
-            .getArgument(0)
-            .getConstantValue()
-            .getStringOrSymbol() = name
-        or
-        // calling a non-sensitive method
-        this.(DataFlow::CallNode).getMethodName() = name
-      )
-      or
-      // avoid i18n strings
-      this.asExpr()
-          .(CfgNodes::ExprNodes::ElementReferenceCfgNode)
-          .getReceiver()
-          .getConstantValue()
-          .getStringOrSymbol()
-          .regexpMatch("(?is).*(messages|strings).*")
-    }
-  }
-
-  /**
-   * A data flow node that receives flow that is not a clear-text password.
-   */
-  private class NonCleartextPasswordFlow extends NonCleartextPassword {
-    NonCleartextPasswordFlow() {
-      any(NonCleartextPassword other).(DataFlow::LocalSourceNode).flowsTo(this)
-    }
-  }
-
-  /**
-   * A data flow node that does not contain a clear-text password.
-   */
-  abstract private class NonCleartextPassword extends DataFlow::Node { }
-
-  // `writeNode` assigns pair with key `name` to `val`
-  private predicate hashKeyWrite(DataFlow::CallNode writeNode, string name, DataFlow::Node val) {
-    writeNode.asExpr().getExpr() instanceof SetterMethodCall and
-    // hash[name]
-    writeNode.getArgument(0).asExpr().getConstantValue().getStringOrSymbol() = name and
-    // val
-    writeNode.getArgument(1).asExpr().(CfgNodes::ExprNodes::AssignExprCfgNode).getRhs() =
-      val.asExpr()
-  }
-
-  /**
-   * A write to a hash entry with a value that may contain password information.
-   */
-  private class HashKeyWritePasswordSource extends Source {
-    private string name;
-    private DataFlow::ExprNode recv;
-
-    HashKeyWritePasswordSource() {
-      exists(DataFlow::Node val |
-        name.regexpMatch(maybePassword()) and
-        not nameIsNotSensitive(name) and
-        // avoid safe values assigned to presumably unsafe names
-        not val instanceof NonCleartextPassword and
-        (
-          // hash[name] = val
-          hashKeyWrite(this, name, val) and
-          recv = this.(DataFlow::CallNode).getReceiver()
-        )
-      )
-    }
-
-    override string describe() { result = "a write to " + name }
-
-    /** Gets the name of the key */
-    string getName() { result = name }
-
-    /**
-     * Gets the name of the hash variable that this password source is assigned
-     * to, if applicable.
-     */
-    LocalVariable getVariable() {
-      result = recv.getExprNode().getExpr().(VariableReadAccess).getVariable()
-    }
-  }
+  /** Holds if `nodeFrom` taints `nodeTo`. */
+  predicate isAdditionalTaintStep = CleartextSources::isAdditionalTaintStep/2;
 
   /**
-   * A hash literal with an entry that may contain a password
+   * A data flow sink for cleartext logging of sensitive information.
    */
-  private class HashLiteralPasswordSource extends Source {
-    private string name;
-
-    HashLiteralPasswordSource() {
-      exists(DataFlow::Node val, CfgNodes::ExprNodes::HashLiteralCfgNode lit |
-        name.regexpMatch(maybePassword()) and
-        not name.regexpMatch(notSensitiveRegexp()) and
-        // avoid safe values assigned to presumably unsafe names
-        not val instanceof NonCleartextPassword and
-        // hash = { name: val }
-        exists(CfgNodes::ExprNodes::PairCfgNode p |
-          this.asExpr() = lit and p = lit.getAKeyValuePair()
-        |
-          p.getKey().getConstantValue().getStringOrSymbol() = name and
-          p.getValue() = val.asExpr()
-        )
-      )
-    }
-
-    override string describe() { result = "an write to " + name }
-  }
-
-  /** An assignment that may assign a password to a variable */
-  private class AssignPasswordVariableSource extends Source {
-    string name;
-
-    AssignPasswordVariableSource() {
-      // avoid safe values assigned to presumably unsafe names
-      not this instanceof NonCleartextPassword and
-      name.regexpMatch(maybePassword()) and
-      exists(Assignment a |
-        this.asExpr().getExpr() = a.getRightOperand() and
-        a.getLeftOperand().getAVariable().getName() = name
-      )
-    }
-
-    override string describe() { result = "an assignment to " + name }
-  }
-
-  /** A parameter that may contain a password. */
-  private class ParameterPasswordSource extends Source {
-    private string name;
-
-    ParameterPasswordSource() {
-      name.regexpMatch(maybePassword()) and
-      not this instanceof NonCleartextPassword and
-      exists(Parameter p, LocalVariable v |
-        v = p.getAVariable() and
-        v.getName() = name and
-        this.asExpr().getExpr() = v.getAnAccess()
-      )
-    }
-
-    override string describe() { result = "a parameter " + name }
-  }
-
-  /** A call that might return a password. */
-  private class CallPasswordSource extends DataFlow::CallNode, Source {
-    private string name;
-
-    CallPasswordSource() {
-      name = this.getMethodName() and
-      name.regexpMatch("(?is)getPassword")
-    }
-
-    override string describe() { result = "a call to " + name }
-  }
+  abstract class Sink extends DataFlow::Node { }
 
   private string commonLogMethodName() {
     result = ["info", "debug", "warn", "warning", "error", "log"]
   }
 
-  /** Holds if `nodeFrom` taints `nodeTo`. */
-  predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
-    exists(string name, ElementReference ref, LocalVariable hashVar |
-      // from `hsh[password] = "changeme"` to a `hsh[password]` read
-      nodeFrom.(HashKeyWritePasswordSource).getName() = name and
-      nodeTo.asExpr().getExpr() = ref and
-      ref.getArgument(0).getConstantValue().getStringOrSymbol() = name and
-      nodeFrom.(HashKeyWritePasswordSource).getVariable() = hashVar and
-      ref.getReceiver().(VariableReadAccess).getVariable() = hashVar and
-      nodeFrom.asExpr().getASuccessor*() = nodeTo.asExpr()
-    )
-  }
-
   /**
    * A node representing an expression whose value is logged.
    */

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

@@ -0,0 +1,49 @@
+/**
+ * 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 persistent data store.
+   */
+  private class PersistentWriteAccessAsSink extends Sink {
+    PersistentWriteAccessAsSink() { this = any(PersistentWriteAccess write).getValue() }
+  }
+}

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

@@ -0,0 +1,33 @@
+/**
+ * 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
+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)
+  }
+}