Merge remote-tracking branch 'origin/main' into ruby/rails-cookie-config

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/src/queries/analysis/Definitions.ql

@@ -78,7 +78,7 @@ ConstantWriteAccess definitionOf(string fqn) {
   fqn = resolveConstant(_) and
   result =
     min(ConstantWriteAccess w, Location l |
-      w.getQualifiedName() = fqn and l = w.getLocation()
+      w.getAQualifiedName() = fqn and l = w.getLocation()
     |
       w
       order by