Merge pull request #7305 from github/nickrolfe/user-controlled-bypass

Ruby: query to find user-controlled bypass of sensitive actions

Author: nickrolfe

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

@@ -0,0 +1,61 @@
+/**
+ * Provides default sources, sinks, and sanitizers for reasoning about bypass of
+ * sensitive action guards, as well as extension points for adding your own.
+ */
+
+private import codeql.ruby.CFG
+private import codeql.ruby.DataFlow
+private import codeql.ruby.controlflow.BasicBlocks
+private import codeql.ruby.dataflow.RemoteFlowSources
+private import codeql.ruby.security.SensitiveActions
+
+/**
+ * Provides default sources, sinks, and sanitizers for reasoning about bypass of
+ * sensitive action guards, as well as extension points for adding your own.
+ */
+module ConditionalBypass {
+  /**
+   * A data flow source for bypass of sensitive action guards.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for bypass of sensitive action guards.
+   */
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets the guarded sensitive action.
+     */
+    abstract SensitiveAction getAction();
+  }
+
+  /**
+   * A sanitizer for bypass of sensitive action guards.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A source of remote user input, considered as a flow source for bypass of
+   * sensitive action guards.
+   */
+  class RemoteFlowSourceAsSource extends Source {
+    RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
+  }
+
+  /**
+   * A conditional that guards a sensitive action, e.g. `ok` in `if (ok) login()`.
+   */
+  class SensitiveActionGuardConditional extends Sink {
+    SensitiveAction action;
+
+    SensitiveActionGuardConditional() {
+      exists(ConditionBlock cb, BasicBlock controlled |
+        cb.controls(controlled, _) and
+        controlled.getANode() = action.asExpr() and
+        cb.getLastNode() = this.asExpr()
+      )
+    }
+
+    override SensitiveAction getAction() { result = action }
+  }
+}

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

@@ -0,0 +1,28 @@
+/**
+ * Provides a taint tracking configuration for reasoning about bypass of sensitive action guards.
+ *
+ * Note, for performance reasons: only import this file if
+ * `ConditionalBypass::Configuration` is needed, otherwise
+ * `ConditionalBypassCustomizations` should be imported instead.
+ */
+
+private import codeql.ruby.DataFlow
+private import codeql.ruby.TaintTracking
+private import codeql.ruby.security.SensitiveActions
+import ConditionalBypassCustomizations::ConditionalBypass
+
+/**
+ * A taint tracking configuration for bypass of sensitive action guards.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "ConditionalBypass" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    super.isSanitizer(node) or
+    node instanceof Sanitizer
+  }
+}

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

@@ -0,0 +1,68 @@
+/**
+ * Provides classes and predicates for identifying sensitive data and methods for security.
+ *
+ * 'Sensitive' data in general is anything that should not be sent around in unencrypted form. This
+ * library tries to guess where sensitive data may either be stored in a variable or produced by a
+ * method.
+ *
+ * In addition, there are methods that ought not to be executed or not in a fashion that the user
+ * can control. This includes authorization methods such as logins, and sending of data, etc.
+ */
+
+private import codeql.ruby.AST
+private import codeql.ruby.DataFlow
+
+/**
+ * A sensitive action, such as transfer of sensitive data.
+ */
+abstract class SensitiveAction extends DataFlow::Node { }
+
+/** Holds if the return value from call `c` is ignored. */
+private predicate callWithIgnoredReturnValue(Call c) {
+  exists(StmtSequence s, int i |
+    (
+      // If the call is a top-level statement within a statement sequence, its
+      // return value (if any) is unused.
+      c = s.getStmt(i)
+      or
+      // Or if the statement is an if-/unless-modifier expr and the call is its
+      // branch.
+      exists(ConditionalExpr cond |
+        cond = s.getStmt(i) and
+        c = cond.getBranch(_) and
+        (cond instanceof IfModifierExpr or cond instanceof UnlessModifierExpr)
+      )
+    ) and
+    // But exclude calls that are the last statement, since they are evaluated
+    // as the overall value of the sequence.
+    exists(s.getStmt(i + 1))
+  ) and
+  not c instanceof YieldCall and
+  // Ignore statements in ERB output directives, which are evaluated.
+  not exists(ErbOutputDirective d | d.getAChildStmt() = c)
+}
+
+/** A call that may perform authorization. */
+class AuthorizationCall extends SensitiveAction, DataFlow::CallNode {
+  AuthorizationCall() {
+    exists(MethodCall c, string s |
+      c = this.asExpr().getExpr() and
+      s = c.getMethodName() // name contains `login` or `auth`, but not as part of `loginfo` or `unauth`;
+    |
+      // also exclude `author`
+      s.regexpMatch("(?i).*(log_?in(?!fo)|(?<!un)auth(?!or\\b)|verify).*") and
+      // but it does not start with `get` or `set`
+      not s.regexpMatch("(?i)(get|set).*") and
+      // Setter calls are unlikely to be sensitive actions.
+      not c instanceof SetterMethodCall and
+      (
+        // Calls that have no return value (or ignore it) are likely to be
+        // to methods that are actions.
+        callWithIgnoredReturnValue(c)
+        or
+        // Method names ending in `!` are likely to be actions.
+        s.matches("%!")
+      )
+    )
+  }
+}

ruby/ql/src/experimental/cwe-807/ConditionalBypass.qhelp

@@ -0,0 +1,36 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+  <overview>
+    <p>
+      Using user-controlled data in a permissions check may allow a user to gain
+      unauthorized access to protected functionality or data.
+    </p>
+  </overview>
+  <recommendation>
+    <p>
+      When checking whether a user is authorized for a particular activity, do
+      not use data that is entirely controlled by that user in the permissions
+      check. If necessary, always validate the input, ideally against a fixed
+      list of expected values.
+    </p>
+    <p>
+      Similarly, do not decide which permission to check based on user data. In
+      particular, avoid using computation to decide which permissions to check
+      for. Use fixed permissions for particular actions, rather than generating
+      the permission to check for.
+    </p>
+  </recommendation>
+  <example>
+    <p>
+      In this example, the controller decided whether or not to authenticate the
+      user based on the value of a request parameter.
+    </p>
+    <sample src="examples/bypass.rb" />
+  </example>
+  
+  <references>
+  
+  </references>
+</qhelp>

ruby/ql/src/experimental/cwe-807/ConditionalBypass.ql

@@ -0,0 +1,82 @@
+/**
+ * @name User-controlled bypass of security check
+ * @description Conditions controlled by the user are not suited for making security-related decisions.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 7.8
+ * @precision medium
+ * @id rb/user-controlled-bypass
+ * @tags security
+ *       external/cwe/cwe-807
+ *       external/cwe/cwe-290
+ */
+
+import ruby
+import codeql.ruby.DataFlow
+import codeql.ruby.dataflow.internal.DataFlowPublic
+import codeql.ruby.security.ConditionalBypassQuery
+import codeql.ruby.security.SensitiveActions
+import DataFlow::PathGraph
+
+/**
+ * Holds if the value of `nd` flows into `guard`.
+ */
+predicate flowsToGuardExpr(DataFlow::Node nd, SensitiveActionGuardConditional guard) {
+  nd = guard
+  or
+  exists(DataFlow::Node succ | localFlowStep(nd, succ) | flowsToGuardExpr(succ, guard))
+}
+
+/**
+ * A comparison that guards a sensitive action, e.g. the comparison in:
+ * ```rb
+ * ok = x == y
+ * if ok
+ *   login
+ * end
+ * ```
+ */
+class SensitiveActionGuardComparison extends ComparisonOperation {
+  SensitiveActionGuardConditional guard;
+
+  SensitiveActionGuardComparison() {
+    exists(DataFlow::Node node | this = node.asExpr().getExpr() | flowsToGuardExpr(node, guard))
+  }
+
+  /**
+   * Gets the guard that uses this comparison.
+   */
+  SensitiveActionGuardConditional getGuard() { result = guard }
+}
+
+/**
+ * An intermediary sink to enable reuse of the taint configuration.
+ * This sink should not be presented to the client of this query.
+ */
+class SensitiveActionGuardComparisonOperand extends Sink {
+  SensitiveActionGuardComparison comparison;
+
+  SensitiveActionGuardComparisonOperand() { this.asExpr().getExpr() = comparison.getAnOperand() }
+
+  override SensitiveAction getAction() { result = comparison.getGuard().getAction() }
+}
+
+/**
+ * Holds if `sink` guards `action`, and `source` taints `sink`.
+ *
+ * If flow from `source` taints `sink`, then an attacker can
+ * control if `action` should be executed or not.
+ */
+predicate isTaintedGuardForSensitiveAction(
+  DataFlow::PathNode sink, DataFlow::PathNode source, SensitiveAction action
+) {
+  action = sink.getNode().(Sink).getAction() and
+  // exclude the intermediary sink
+  not sink.getNode() instanceof SensitiveActionGuardComparisonOperand and
+  exists(Configuration cfg | cfg.hasFlowPath(source, sink))
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, SensitiveAction action
+where isTaintedGuardForSensitiveAction(sink, source, action)
+select sink.getNode(), source, sink, "This condition guards a sensitive $@, but $@ controls it.",
+  action, "action", source.getNode(), "a user-provided value"

ruby/ql/src/experimental/cwe-807/examples/bypass.rb

@@ -0,0 +1,10 @@
+class UsersController < ActionController::Base
+  def example
+    user = User.find_by(login: params[:login])
+    if params[:authenticate]
+      # BAD: decision to take sensitive action based on user-controlled data
+      log_in user
+      redirect_to user
+    end
+  end
+end

ruby/ql/test/query-tests/security/cwe-807-user-controlled-bypass/ConditionalBypass.expected

@@ -0,0 +1,21 @@
+edges
+| ConditionalBypass.rb:3:13:3:18 | call to params :  | ConditionalBypass.rb:6:8:6:12 | check |
+| ConditionalBypass.rb:14:14:14:19 | call to params :  | ConditionalBypass.rb:14:14:14:27 | ...[...] |
+| ConditionalBypass.rb:25:10:25:15 | call to params :  | ConditionalBypass.rb:25:10:25:22 | ...[...] |
+| ConditionalBypass.rb:25:10:25:15 | call to params :  | ConditionalBypass.rb:25:10:25:22 | ...[...] :  |
+| ConditionalBypass.rb:25:10:25:15 | call to params :  | ConditionalBypass.rb:27:8:27:8 | p |
+| ConditionalBypass.rb:25:10:25:22 | ...[...] :  | ConditionalBypass.rb:27:8:27:8 | p |
+nodes
+| ConditionalBypass.rb:3:13:3:18 | call to params :  | semmle.label | call to params :  |
+| ConditionalBypass.rb:6:8:6:12 | check | semmle.label | check |
+| ConditionalBypass.rb:14:14:14:19 | call to params :  | semmle.label | call to params :  |
+| ConditionalBypass.rb:14:14:14:27 | ...[...] | semmle.label | ...[...] |
+| ConditionalBypass.rb:25:10:25:15 | call to params :  | semmle.label | call to params :  |
+| ConditionalBypass.rb:25:10:25:22 | ...[...] | semmle.label | ...[...] |
+| ConditionalBypass.rb:25:10:25:22 | ...[...] :  | semmle.label | ...[...] :  |
+| ConditionalBypass.rb:27:8:27:8 | p | semmle.label | p |
+subpaths
+#select
+| ConditionalBypass.rb:6:8:6:12 | check | ConditionalBypass.rb:3:13:3:18 | call to params :  | ConditionalBypass.rb:6:8:6:12 | check | This condition guards a sensitive $@, but $@ controls it. | ConditionalBypass.rb:8:7:8:29 | call to authenticate_user! | action | ConditionalBypass.rb:3:13:3:18 | call to params | a user-provided value |
+| ConditionalBypass.rb:14:14:14:27 | ...[...] | ConditionalBypass.rb:14:14:14:19 | call to params :  | ConditionalBypass.rb:14:14:14:27 | ...[...] | This condition guards a sensitive $@, but $@ controls it. | ConditionalBypass.rb:14:5:14:9 | call to login | action | ConditionalBypass.rb:14:14:14:19 | call to params | a user-provided value |
+| ConditionalBypass.rb:27:8:27:8 | p | ConditionalBypass.rb:25:10:25:15 | call to params :  | ConditionalBypass.rb:27:8:27:8 | p | This condition guards a sensitive $@, but $@ controls it. | ConditionalBypass.rb:28:7:28:13 | call to verify! | action | ConditionalBypass.rb:25:10:25:15 | call to params | a user-provided value |

ruby/ql/test/query-tests/security/cwe-807-user-controlled-bypass/ConditionalBypass.qlref

@@ -0,0 +1 @@
+experimental/cwe-807/ConditionalBypass.ql

ruby/ql/test/query-tests/security/cwe-807-user-controlled-bypass/ConditionalBypass.rb

@@ -0,0 +1,39 @@
+class FooController < ActionController::Base
+  def bad_handler1
+    check = params[:check]
+    name = params[:name]
+
+    if check
+      # BAD
+      authenticate_user! name
+    end
+  end
+
+  def bad_handler2
+    # BAD
+    login if params[:login]
+    do_something_else
+  end
+
+  def bad_handler3
+    # BAD. Not detected: its the last statement in the method, so it doesn't
+    # match the heuristic for an action.
+    login if params[:login]
+  end
+
+  def bad_handler4
+    p = (params[:name] == "foo")
+    # BAD
+    if p
+      verify!
+    end
+  end
+
+  def good_handler
+    name = params[:name]
+    # Call to a sensitive action, but the guard is not derived from user input.
+    if should_auth_user?
+      authenticate_user! name
+    end
+  end
+end