Merge branch 'main' into p--ruby-kernel-open-addition

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

@@ -497,6 +497,7 @@ private module Cached {
       FlowSummaryImplSpecific::ParsePositions::isParsedKeywordParameterPosition(_, name)
     } or
     THashSplatArgumentPosition() or
+    TSplatAllArgumentPosition() or
     TAnyArgumentPosition() or
     TAnyKeywordArgumentPosition()
 
@@ -518,6 +519,7 @@ private module Cached {
       FlowSummaryImplSpecific::ParsePositions::isParsedKeywordArgumentPosition(_, name)
     } or
     THashSplatParameterPosition() or
+    TSplatAllParameterPosition() or
     TAnyParameterPosition() or
     TAnyKeywordParameterPosition()
 }
@@ -1149,6 +1151,8 @@ class ParameterPosition extends TParameterPosition {
   /** Holds if this position represents a hash-splat parameter. */
   predicate isHashSplat() { this = THashSplatParameterPosition() }
 
+  predicate isSplatAll() { this = TSplatAllParameterPosition() }
+
   /**
    * Holds if this position represents any parameter, except `self` parameters. This
    * includes both positional, named, and block parameters.
@@ -1172,6 +1176,8 @@ class ParameterPosition extends TParameterPosition {
     or
     this.isHashSplat() and result = "**"
     or
+    this.isSplatAll() and result = "*"
+    or
     this.isAny() and result = "any"
     or
     this.isAnyNamed() and result = "any-named"
@@ -1207,6 +1213,8 @@ class ArgumentPosition extends TArgumentPosition {
    */
   predicate isHashSplat() { this = THashSplatArgumentPosition() }
 
+  predicate isSplatAll() { this = TSplatAllArgumentPosition() }
+
   /** Gets a textual representation of this position. */
   string toString() {
     this.isSelf() and result = "self"
@@ -1222,6 +1230,8 @@ class ArgumentPosition extends TArgumentPosition {
     this.isAnyNamed() and result = "any-named"
     or
     this.isHashSplat() and result = "**"
+    or
+    this.isSplatAll() and result = "*"
   }
 }
 
@@ -1248,6 +1258,8 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
   or
   ppos.isHashSplat() and apos.isHashSplat()
   or
+  ppos.isSplatAll() and apos.isSplatAll()
+  or
   ppos.isAny() and argumentPositionIsNotSelf(apos)
   or
   apos.isAny() and parameterPositionIsNotSelf(ppos)

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

@@ -241,6 +241,10 @@ private class Argument extends CfgNodes::ExprCfgNode {
     this = call.getAnArgument() and
     this.getExpr() instanceof HashSplatExpr and
     arg.isHashSplat()
+    or
+    this = call.getArgument(0) and
+    this.getExpr() instanceof SplatExpr and
+    arg.isSplatAll()
   }
 
   /** Holds if this expression is the `i`th argument of `c`. */
@@ -276,7 +280,8 @@ private module Cached {
       p instanceof SimpleParameter or
       p instanceof OptionalParameter or
       p instanceof KeywordParameter or
-      p instanceof HashSplatParameter
+      p instanceof HashSplatParameter or
+      p instanceof SplatParameter
     } or
     TSelfParameterNode(MethodBase m) or
     TBlockParameterNode(MethodBase m) or
@@ -616,6 +621,9 @@ private module ParameterNodes {
         or
         parameter = callable.getAParameter().(HashSplatParameter) and
         pos.isHashSplat()
+        or
+        parameter = callable.getParameter(0).(SplatParameter) and
+        pos.isSplatAll()
       )
     }
 

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

@@ -0,0 +1,55 @@
+/**
+ * Provides default sources, sinks and sanitizers for detecting stack trace
+ * exposure vulnerabilities, as well as extension points for adding your own.
+ */
+
+private import codeql.ruby.AST
+private import codeql.ruby.Concepts
+private import codeql.ruby.DataFlow
+private import codeql.ruby.controlflow.CfgNodes
+private import codeql.ruby.frameworks.core.Kernel
+
+/**
+ * Provides default sources, sinks and sanitizers for detecting stack trace
+ * exposure vulnerabilities, as well as extension points for adding your own.
+ */
+module StackTraceExposure {
+  /** A data flow source for stack trace exposure vulnerabilities. */
+  abstract class Source extends DataFlow::Node { }
+
+  /** A data flow sink for stack trace exposure vulnerabilities. */
+  abstract class Sink extends DataFlow::Node { }
+
+  /** A data flow sanitizer for stack trace exposure vulnerabilities. */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A call to `backtrace` or `backtrace_locations` on a `rescue` variable,
+   * considered as a flow source.
+   */
+  class BacktraceCall extends Source, DataFlow::CallNode {
+    BacktraceCall() {
+      exists(DataFlow::LocalSourceNode varAccess |
+        varAccess.asExpr().(ExprNodes::VariableReadAccessCfgNode).getExpr().getVariable() =
+          any(RescueClause rc).getVariableExpr().(VariableAccess).getVariable() and
+        varAccess.flowsTo(this.getReceiver())
+      ) and
+      this.getMethodName() = ["backtrace", "backtrace_locations"]
+    }
+  }
+
+  /**
+   * A call to `Kernel#caller`, considered as a flow source.
+   */
+  class KernelCallerCall extends Source, Kernel::KernelMethodCall {
+    KernelCallerCall() { this.getMethodName() = "caller" }
+  }
+
+  /**
+   * The body of an HTTP response that will be returned from a server,
+   * considered as a flow sink.
+   */
+  class ServerHttpResponseBodyAsSink extends Sink {
+    ServerHttpResponseBodyAsSink() { this = any(Http::Server::HttpResponse response).getBody() }
+  }
+}

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

@@ -0,0 +1,25 @@
+/**
+ * Provides a taint-tracking configuration for detecting stack-trace exposure
+ * vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if
+ * `StackTraceExposure::Configuration` is needed; otherwise,
+ * `StackTraceExposureCustomizations` should be imported instead.
+ */
+
+private import codeql.ruby.DataFlow
+private import codeql.ruby.TaintTracking
+private import StackTraceExposureCustomizations::StackTraceExposure
+
+/**
+ * A taint-tracking configuration for detecting "stack trace exposure" vulnerabilities.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "StackTraceExposure" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+}

ruby/ql/src/change-notes/2022-11-14-stack-trace-exposure.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `rb/stack-trace-exposure`, to detect exposure of stack-traces to users via HTTP responses.

ruby/ql/src/queries/security/cwe-209/StackTraceExposure.qhelp

@@ -0,0 +1,49 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+
+<p>
+Software developers often add stack traces to error messages, as a debugging
+aid. Whenever that error message occurs for an end user, the developer can use
+the stack trace to help identify how to fix the problem. In particular, stack
+traces can tell the developer more about the sequence of events that led to a
+failure, as opposed to merely the final state of the software when the error
+occurred.
+</p>
+
+<p>
+Unfortunately, the same information can be useful to an attacker.  The sequence
+of class or method names in a stack trace can reveal the structure of the
+application as well as any internal components it relies on.  Furthermore, the
+error message at the top of a stack trace can include information such as
+server-side file names and SQL code that the application relies on, allowing an
+attacker to fine-tune a subsequent injection attack.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Send the user a more generic error message that reveals less information.
+Either suppress the stack trace entirely, or log it only on the server.
+</p>
+</recommendation>
+
+<example>
+<p>
+In the following example, an exception is handled in two different ways. In the
+first version, labeled BAD, the exception is exposed to the remote user by
+rendering it as an HTTP response. As such, the user is able to see a detailed
+stack trace, which may contain sensitive information. In the second version, the
+error message is logged only on the server, and a generic error message is
+displayed to the user. That way, the developers can still access and use the
+error log, but remote users will not see the information.  </p>
+
+<sample src="examples/StackTraceExposure.rb" />
+</example>
+
+<references>
+<li>OWASP: <a href="https://owasp.org/www-community/Improper_Error_Handling">Improper Error Handling</a>.</li>
+</references>
+</qhelp>

ruby/ql/src/queries/security/cwe-209/StackTraceExposure.ql

@@ -0,0 +1,23 @@
+/**
+ * @name Information exposure through an exception
+ * @description Leaking information about an exception, such as messages and stack traces, to an
+ *              external user can expose implementation details that are useful to an attacker for
+ *              developing a subsequent exploit.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 5.4
+ * @precision high
+ * @id rb/stack-trace-exposure
+ * @tags security
+ *       external/cwe/cwe-209
+ *       external/cwe/cwe-497
+ */
+
+import codeql.ruby.DataFlow
+import codeql.ruby.security.StackTraceExposureQuery
+import DataFlow::PathGraph
+
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "$@ can be exposed to an external user.", source.getNode(),
+  "Error information"

ruby/ql/src/queries/security/cwe-209/examples/StackTraceExposure.rb

@@ -0,0 +1,18 @@
+class UsersController < ApplicationController
+
+  def update_bad(id)
+    do_computation()
+  rescue => e
+    # BAD
+    render body: e.backtrace, content_type: "text/plain"
+  end
+
+  def update_good(id)
+    do_computation()
+  rescue => e
+    # GOOD
+    logger.error e.backtrace
+    render body: "Computation failed", content_type: "text/plain"
+  end
+
+end

ruby/ql/test/library-tests/dataflow/params/params-flow.expected

@@ -40,6 +40,11 @@ edges
 | params_flow.rb:49:13:49:14 | p1 :  | params_flow.rb:50:10:50:11 | p1 |
 | params_flow.rb:54:9:54:17 | call to taint :  | params_flow.rb:49:13:49:14 | p1 :  |
 | params_flow.rb:57:9:57:17 | call to taint :  | params_flow.rb:49:13:49:14 | p1 :  |
+| params_flow.rb:62:8:62:16 | call to taint :  | params_flow.rb:66:13:66:16 | args :  |
+| params_flow.rb:63:16:63:17 | *x [element 0] :  | params_flow.rb:64:10:64:10 | x [element 0] :  |
+| params_flow.rb:64:10:64:10 | x [element 0] :  | params_flow.rb:64:10:64:13 | ...[...] |
+| params_flow.rb:66:12:66:16 | * ... [element 0] :  | params_flow.rb:63:16:63:17 | *x [element 0] :  |
+| params_flow.rb:66:13:66:16 | args :  | params_flow.rb:66:12:66:16 | * ... [element 0] :  |
 nodes
 | params_flow.rb:9:16:9:17 | p1 :  | semmle.label | p1 :  |
 | params_flow.rb:9:20:9:21 | p2 :  | semmle.label | p2 :  |
@@ -89,6 +94,12 @@ nodes
 | params_flow.rb:50:10:50:11 | p1 | semmle.label | p1 |
 | params_flow.rb:54:9:54:17 | call to taint :  | semmle.label | call to taint :  |
 | params_flow.rb:57:9:57:17 | call to taint :  | semmle.label | call to taint :  |
+| params_flow.rb:62:8:62:16 | call to taint :  | semmle.label | call to taint :  |
+| params_flow.rb:63:16:63:17 | *x [element 0] :  | semmle.label | *x [element 0] :  |
+| params_flow.rb:64:10:64:10 | x [element 0] :  | semmle.label | x [element 0] :  |
+| params_flow.rb:64:10:64:13 | ...[...] | semmle.label | ...[...] |
+| params_flow.rb:66:12:66:16 | * ... [element 0] :  | semmle.label | * ... [element 0] :  |
+| params_flow.rb:66:13:66:16 | args :  | semmle.label | args :  |
 subpaths
 #select
 | params_flow.rb:10:10:10:11 | p1 | params_flow.rb:14:12:14:19 | call to taint :  | params_flow.rb:10:10:10:11 | p1 | $@ | params_flow.rb:14:12:14:19 | call to taint :  | call to taint :  |
@@ -111,3 +122,4 @@ subpaths
 | params_flow.rb:29:10:29:22 | ( ... ) | params_flow.rb:34:14:34:22 | call to taint :  | params_flow.rb:29:10:29:22 | ( ... ) | $@ | params_flow.rb:34:14:34:22 | call to taint :  | call to taint :  |
 | params_flow.rb:50:10:50:11 | p1 | params_flow.rb:54:9:54:17 | call to taint :  | params_flow.rb:50:10:50:11 | p1 | $@ | params_flow.rb:54:9:54:17 | call to taint :  | call to taint :  |
 | params_flow.rb:50:10:50:11 | p1 | params_flow.rb:57:9:57:17 | call to taint :  | params_flow.rb:50:10:50:11 | p1 | $@ | params_flow.rb:57:9:57:17 | call to taint :  | call to taint :  |
+| params_flow.rb:64:10:64:13 | ...[...] | params_flow.rb:62:8:62:16 | call to taint :  | params_flow.rb:64:10:64:13 | ...[...] | $@ | params_flow.rb:62:8:62:16 | call to taint :  | call to taint :  |

ruby/ql/test/library-tests/dataflow/params/params_flow.rb

@@ -57,4 +57,10 @@ args = [taint(22)]
 posargs(taint(23), *args)
 
 args = [taint(24), taint(25)]
-posargs(*args)
+posargs(*args)
+
+args = taint(26)
+def splatstuff(*x)
+    sink x[0] # $ hasValueFlow=26
+end
+splatstuff(*args)

ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.expected

@@ -0,0 +1,12 @@
+edges
+| StackTraceExposure.rb:11:10:11:17 | call to caller :  | StackTraceExposure.rb:12:18:12:19 | bt |
+nodes
+| StackTraceExposure.rb:6:18:6:28 | call to backtrace | semmle.label | call to backtrace |
+| StackTraceExposure.rb:11:10:11:17 | call to caller :  | semmle.label | call to caller :  |
+| StackTraceExposure.rb:12:18:12:19 | bt | semmle.label | bt |
+| StackTraceExposure.rb:18:18:18:28 | call to backtrace | semmle.label | call to backtrace |
+subpaths
+#select
+| StackTraceExposure.rb:6:18:6:28 | call to backtrace | StackTraceExposure.rb:6:18:6:28 | call to backtrace | StackTraceExposure.rb:6:18:6:28 | call to backtrace | $@ can be exposed to an external user. | StackTraceExposure.rb:6:18:6:28 | call to backtrace | Error information |
+| StackTraceExposure.rb:12:18:12:19 | bt | StackTraceExposure.rb:11:10:11:17 | call to caller :  | StackTraceExposure.rb:12:18:12:19 | bt | $@ can be exposed to an external user. | StackTraceExposure.rb:11:10:11:17 | call to caller | Error information |
+| StackTraceExposure.rb:18:18:18:28 | call to backtrace | StackTraceExposure.rb:18:18:18:28 | call to backtrace | StackTraceExposure.rb:18:18:18:28 | call to backtrace | $@ can be exposed to an external user. | StackTraceExposure.rb:18:18:18:28 | call to backtrace | Error information |

ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.qlref

@@ -0,0 +1 @@
+queries/security/cwe-209/StackTraceExposure.ql

ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.rb

@@ -0,0 +1,21 @@
+class FooController < ApplicationController
+
+  def show
+    something_that_might_fail()
+  rescue => e
+    render body: e.backtrace, content_type: "text/plain"
+  end
+
+
+  def show2
+    bt = caller()
+    render body: bt, content_type: "text/plain"
+  end
+
+  def show3
+    not_a_method()
+  rescue NoMethodError => e
+    render body: e.backtrace, content_type: "text/plain"
+  end
+
+end