Merge pull request github#8607 from github/nickrolfe/incomplete_sanitization

Ruby: port of `js/incomplete-sanitization`

Author: nickrolfe

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll

@@ -40,9 +40,20 @@ class Node extends TNode {
   }
 
   /**
-   * Gets a local source node from which data may flow to this node in zero or more local data-flow steps.
+   * Gets a local source node from which data may flow to this node in zero or
+   * more local data-flow steps.
    */
   LocalSourceNode getALocalSource() { result.flowsTo(this) }
+
+  /**
+   * Gets a data flow node from which data may flow to this node in one local step.
+   */
+  Node getAPredecessor() { localFlowStep(result, this) }
+
+  /**
+   * Gets a data flow node to which data may flow from this node in one local step.
+   */
+  Node getASuccessor() { localFlowStep(this, result) }
 }
 
 /** A data-flow node corresponding to a call in the control-flow graph. */

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

@@ -5,6 +5,81 @@ private import codeql.ruby.ApiGraphs
 private import codeql.ruby.DataFlow
 private import codeql.ruby.dataflow.FlowSummary
 private import codeql.ruby.dataflow.internal.DataFlowDispatch
+private import codeql.ruby.controlflow.CfgNodes
+private import codeql.ruby.Regexp as RE
+
+/**
+ * A call to a string substitution method, i.e. `String#sub`, `String#sub!`,
+ * `String#gsub`, or `String#gsub!`.
+ *
+ * We heuristically include any call to a method matching the above names,
+ * provided it has exactly two arguments and a receiver.
+ */
+class StringSubstitutionCall extends DataFlow::CallNode {
+  StringSubstitutionCall() {
+    this.getMethodName() = ["sub", "sub!", "gsub", "gsub!"] and
+    exists(this.getReceiver()) and
+    this.getNumberOfArguments() = 2
+  }
+
+  /**
+   * Holds if this is a global substitution, i.e. this is a call to `gsub` or
+   * `gsub!`.
+   */
+  predicate isGlobal() { this.getMethodName() = ["gsub", "gsub!"] }
+
+  /**
+   * Holds if this is a destructive substitution, i.e. this is a call to `sub!`
+   * or `gsub!`.
+   */
+  predicate isDestructive() { this.getMethodName() = ["sub!", "gsub!"] }
+
+  /** Gets the first argument to this call. */
+  DataFlow::Node getPatternArgument() { result = this.getArgument(0) }
+
+  /** Gets the second argument to this call. */
+  DataFlow::Node getReplacementArgument() { result = this.getArgument(1) }
+
+  /**
+   * Gets the regular expression passed as the first (pattern) argument in this
+   * call, if any.
+   */
+  RE::RegExpPatternSource getPatternRegExp() {
+    // TODO: using local flow means we miss regexps defined as constants outside
+    // of the function scope.
+    result.(DataFlow::LocalSourceNode).flowsTo(this.getPatternArgument())
+  }
+
+  /**
+   * Gets the string value passed as the first (pattern) argument in this call,
+   * if any.
+   */
+  string getPatternString() {
+    result = this.getPatternArgument().asExpr().getConstantValue().getString()
+  }
+
+  /**
+   * Gets the string value passed as the second (replacement) argument in this
+   * call, if any.
+   */
+  string getReplacementString() {
+    result = this.getReplacementArgument().asExpr().getConstantValue().getString()
+  }
+
+  /** Gets a string that is being replaced by this call. */
+  string getAReplacedString() {
+    result = this.getPatternRegExp().getRegExpTerm().getAMatchedString()
+    or
+    result = this.getPatternString()
+  }
+
+  /** Holds if this call to `replace` replaces `old` with `new`. */
+  predicate replaces(string old, string new) {
+    old = this.getAReplacedString() and
+    new = this.getReplacementString()
+    // TODO: handle block-variant of the call
+  }
+}
 
 /**
  * Provides flow summaries for the `String` class.

ruby/ql/lib/codeql/ruby/regexp/RegExpTreeView.qll

@@ -246,6 +246,23 @@ class RegExpTerm extends RegExpParent {
     )
   }
 
+  /**
+   * Gets the single string this regular-expression term matches.
+   *
+   * This predicate is only defined for (sequences/groups of) constant regular
+   * expressions.  In particular, terms involving zero-width assertions like `^`
+   * or `\b` are not considered to have a constant value.
+   *
+   * Note that this predicate does not take flags of the enclosing
+   * regular-expression literal into account.
+   */
+  string getConstantValue() { none() }
+
+  /**
+   * Gets a string that is matched by this regular-expression term.
+   */
+  string getAMatchedString() { result = this.getConstantValue() }
+
   /** Gets the primary QL class for this term. */
   override string getAPrimaryQlClass() { result = "RegExpTerm" }
 }
@@ -406,6 +423,19 @@ class RegExpSequence extends RegExpTerm, TRegExpSequence {
     )
   }
 
+  override string getConstantValue() { result = this.getConstantValue(0) }
+
+  /**
+   * Gets the single string matched by the `i`th child and all following
+   * children of this sequence, if any.
+   */
+  private string getConstantValue(int i) {
+    i = this.getNumChild() and
+    result = ""
+    or
+    result = this.getChild(i).getConstantValue() + this.getConstantValue(i + 1)
+  }
+
   override string getAPrimaryQlClass() { result = "RegExpSequence" }
 }
 
@@ -466,6 +496,11 @@ class RegExpAlt extends RegExpTerm, TRegExpAlt {
     )
   }
 
+  /** Gets an alternative of this term. */
+  RegExpTerm getAlternative() { result = this.getAChild() }
+
+  override string getAMatchedString() { result = this.getAlternative().getAMatchedString() }
+
   override string getAPrimaryQlClass() { result = "RegExpAlt" }
 }
 
@@ -611,6 +646,10 @@ class RegExpCharacterClass extends RegExpTerm, TRegExpCharacterClass {
     )
   }
 
+  override string getAMatchedString() {
+    not this.isInverted() and result = this.getAChild().getAMatchedString()
+  }
+
   override string getAPrimaryQlClass() { result = "RegExpCharacterClass" }
 }
 
@@ -719,6 +758,8 @@ class RegExpConstant extends RegExpTerm {
 
   override RegExpTerm getChild(int i) { none() }
 
+  override string getConstantValue() { result = this.getValue() }
+
   override string getAPrimaryQlClass() { result = "RegExpConstant" }
 }
 
@@ -762,6 +803,10 @@ class RegExpGroup extends RegExpTerm, TRegExpGroup {
     re.groupContents(start, end, result.getStart(), result.getEnd())
   }
 
+  override string getConstantValue() { result = this.getAChild().getConstantValue() }
+
+  override string getAMatchedString() { result = this.getAChild().getAMatchedString() }
+
   override string getAPrimaryQlClass() { result = "RegExpGroup" }
 }
 

ruby/ql/src/change-notes/2022-04-13-incomplete-sanitization.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `rb/incomplete-sanitization`. The query finds string transformations that do not replace or escape all occurrences of a meta-character.

ruby/ql/src/queries/security/cwe-116/IncompleteSanitization.qhelp

@@ -0,0 +1,87 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Sanitizing untrusted input is a common technique for preventing injection attacks such as SQL
+injection or cross-site scripting. Usually, this is done by escaping meta-characters such as quotes
+in a domain-specific way so that they are treated as normal characters.
+</p>
+<p>
+However, directly using the <code>String#sub</code> method to perform escaping is notoriously
+error-prone. Common mistakes include only replacing the first occurrence of a meta-character, or
+backslash-escaping various meta-characters but not the backslash itself.
+</p>
+<p>
+In the former case, later meta-characters are left undisturbed and can be used to subvert the
+sanitization. In the latter case, preceding a meta-character with a backslash leads to the backslash
+being escaped, but the meta-character appearing un-escaped, which again makes the sanitization
+ineffective.
+</p>
+<p>
+Even if the escaped string is not used in a security-critical context, incomplete escaping may still
+have undesirable effects, such as badly rendered or confusing output.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Use a (well-tested) sanitization library if at all possible. These libraries are much more likely to
+handle corner cases correctly than a custom implementation.
+</p>
+
+<p>
+An even safer alternative is to design the application so that sanitization is not needed.
+Otherwise, make sure to use <code>String#gsub</code> rather than <code>String#sub</code>, to ensure
+that all occurrences are replaced, and remember to escape backslashes if applicable.
+</p>
+<p>
+Note, however, that this is generally <i>not</i> sufficient for replacing multi-character strings:
+the <code>String#gsub</code> method performs only one pass over the input string, and will not
+replace further instances of the string that result from earlier replacements.
+</p>
+<p>
+For example, consider the code snippet <code>s.gsub /\/\.\.\//, ""</code>, which attempts to strip
+out all occurences of <code>/../</code> from <code>s</code>. This will not work as expected: for the
+string <code>/./.././</code>, for example, it will remove the single occurrence of <code>/../</code>
+in the middle, but the remainder of the string then becomes <code>/../</code>, which is another
+instance of the substring we were trying to remove.
+</p>
+</recommendation>
+
+<example>
+<p>
+As an example, assume that we want to embed a user-controlled string <code>account_number</code> into
+a SQL query as part of a string literal. To avoid SQL injection, we need to ensure that the string
+does not contain un-escaped single-quote characters. The following method attempts to ensure this by
+doubling single quotes, and thereby escaping them:
+</p>
+
+<sample src="examples/IncompleteSanitization.rb" />
+
+<p>
+As written, this sanitizer is ineffective: <code>String#sub</code> will replace only the
+<i>first</i> occurrence of that string.
+</p>
+
+<p>
+As mentioned above, the method <code>escape_quotes</code> should be replaced with a purpose-built
+sanitizer, such as <code>ActiveRecord::Base::sanitize_sql</code> in Rails, or by using ORM methods
+that automatically sanitize parameters.
+</p>
+
+<p>
+If this is not an option, <code>escape_quotes</code> should be rewritten to use the
+<code>String#gsub</code> method instead:
+</p>
+
+<sample src="examples/IncompleteSanitizationGood.rb" />
+</example>
+
+<references>
+<li>OWASP Top 10: <a href="https://www.owasp.org/index.php/Top_10-2017_A1-Injection">A1 Injection</a>.</li>
+<li>Rails: <a href="https://api.rubyonrails.org/classes/ActiveRecord/Sanitization/ClassMethods.html">ActiveRecord::Base::sanitize_sql</a>.</li>
+</references>
+</qhelp>