rb/reflected-xss

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

@@ -0,0 +1,192 @@
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "reflected server-side cross-site scripting"
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+
+private import ruby
+private import codeql.ruby.DataFlow
+private import codeql.ruby.CFG
+private import codeql.ruby.Concepts
+private import codeql.ruby.Frameworks
+private import codeql.ruby.frameworks.ActionController
+private import codeql.ruby.frameworks.ActionView
+private import codeql.ruby.dataflow.RemoteFlowSources
+private import codeql.ruby.dataflow.BarrierGuards
+private import codeql.ruby.typetracking.TypeTracker
+
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "reflected server-side cross-site scripting"
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+module ReflectedXSS {
+  /**
+   * A data flow source for "reflected server-side cross-site scripting" vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for "reflected server-side cross-site scripting" vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer for "reflected server-side cross-site scripting" vulnerabilities.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A sanitizer guard for "reflected server-side cross-site scripting" vulnerabilities.
+   */
+  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
+
+  /**
+   * A source of remote user input, considered as a flow source.
+   */
+  class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
+
+  private class ErbOutputMethodCallArgumentNode extends DataFlow::Node {
+    private MethodCall call;
+
+    ErbOutputMethodCallArgumentNode() {
+      exists(ErbOutputDirective d |
+        call = d.getTerminalStmt() and
+        this.asExpr().getExpr() = call.getAnArgument()
+      )
+    }
+
+    MethodCall getCall() { result = call }
+  }
+
+  /**
+   * A node on which `html_safe` has been called to mark it as not requiring
+   * HTML escaping, considered as a flow sink.
+   */
+  class HtmlSafeCallAsSink extends Sink {
+    // TODO: extend this to track strings that have been marked as html_safe
+    // earlier in the control flow graph
+    HtmlSafeCallAsSink() {
+      exists(HtmlSafeCall c, ErbOutputDirective d |
+        this.asExpr().getExpr() = c.getReceiver() and
+        c = d.getTerminalStmt()
+      )
+    }
+  }
+
+  /**
+   * An argument to a call to the `raw` method, considered as a flow sink.
+   */
+  class RawCallArgumentAsSink extends Sink, ErbOutputMethodCallArgumentNode {
+    RawCallArgumentAsSink() { this.getCall() instanceof RawCall }
+  }
+
+  /**
+   * A argument to a call to the `link_to` method, which does not expect
+   * unsanitized user-input, considered as a flow sink a flow sink.
+   */
+  class LinkToCallArgumentAsSink extends Sink, ErbOutputMethodCallArgumentNode {
+    LinkToCallArgumentAsSink() {
+      this.asExpr().getExpr() = this.getCall().(LinkToCall).getPathArgument()
+    }
+  }
+
+  /**
+   * An HTML escaping, considered as a sanitizer.
+   */
+  class HtmlEscapingAsSanitizer extends Sanitizer {
+    HtmlEscapingAsSanitizer() { this = any(HtmlEscaping esc).getOutput() }
+  }
+
+  /**
+   * A comparison with a constant string, considered as a sanitizer-guard.
+   */
+  class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }
+
+  /**
+   * An inclusion check against an array of constant strings, considered as a sanitizer-guard.
+   */
+  class StringConstArrayInclusionCallAsSanitizerGuard extends SanitizerGuard,
+    StringConstArrayInclusionCall { }
+
+  /**
+   * An additional step that is taint-preserving in the context of reflected XSS.
+   */
+  predicate isAdditionalXSSTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    // node1 is a `locals` argument to a render call...
+    exists(RenderCall call, Pair kvPair, string hashKey |
+      call.getLocals().getAKeyValuePair() = kvPair and
+      kvPair.getValue() = node1.asExpr().getExpr() and
+      kvPair.getKey().(StringlikeLiteral).getValueText() = hashKey and
+      // `node2` appears in the rendered template file
+      call.getTemplateFile() = node2.getLocation().getFile() and
+      (
+        // ...node2 is a variable read
+        exists(CfgNodes::ExprNodes::VariableReadAccessCfgNode readNode |
+          readNode = node2.asExpr() and
+          readNode.getExpr().getVariable().getName() = "@" + hashKey
+        )
+        or
+        // ...node2 is an element reference against `locals`
+        exists(
+          CfgNodes::ExprNodes::ElementReferenceCfgNode refNode, DataFlow::Node argNode,
+          CfgNodes::ExprNodes::StringlikeLiteralCfgNode strNode
+        |
+          refNode = node2.asExpr() and
+          argNode.asExpr() = refNode.getArgument(0) and
+          refNode.getReceiver().getExpr().(MethodCall).getMethodName() = "locals" and
+          argNode.getALocalSource() = DataFlow::exprNode(strNode) and
+          strNode.getExpr().getValueText() = hashKey
+        )
+        or
+        // ...node2 is a "method call" to a "method" with `hashKey` as its name
+        // TODO: This may be a variable read in reality that we interpret as a method call
+        exists(MethodCall varAcc |
+          varAcc = node2.asExpr().(CfgNodes::ExprNodes::MethodCallCfgNode).getExpr() and
+          varAcc.getMethodName() = hashKey
+        )
+      )
+    )
+    or
+    // instance variables in the controller
+    exists(
+      ActionControllerActionMethod action, VariableReadAccess viewVarRead,
+      VariableWriteAccess controllerVarWrite
+    |
+      viewVarRead = node2.asExpr().(CfgNodes::ExprNodes::VariableReadAccessCfgNode).getExpr() and
+      action.getDefaultTemplateFile() = viewVarRead.getLocation().getFile() and
+      controllerVarWrite.getVariable() instanceof InstanceVariable and
+      viewVarRead.getVariable().getName() = controllerVarWrite.getVariable().getName() and
+      // TODO: include only final assignment along a path
+      node1.asExpr().getExpr() = controllerVarWrite and
+      controllerVarWrite.getParent+() = action
+    )
+    or
+    // flow from template into controller helper method
+    exists(
+      ErbFile template, ActionControllerHelperMethod helperMethod,
+      CfgNodes::ExprNodes::MethodCallCfgNode helperMethodCall, int argIdx
+    |
+      template = node1.getLocation().getFile() and
+      helperMethod.getName() = helperMethodCall.getExpr().getMethodName() and
+      helperMethod.getControllerClass() = getAssociatedControllerClass(template) and
+      helperMethodCall.getArgument(argIdx) = node1.asExpr() and
+      helperMethod.getParameter(argIdx) = node2.asExpr().getExpr()
+    )
+    or
+    // flow out of controller helper method into template
+    exists(
+      ErbFile template, ActionControllerHelperMethod helperMethod,
+      CfgNodes::ExprNodes::MethodCallCfgNode helperMethodCall, ReturnStmt ret
+    |
+      template = node2.getLocation().getFile() and
+      helperMethod.getName() = helperMethodCall.getExpr().getMethodName() and
+      helperMethod.getControllerClass() = getAssociatedControllerClass(template) and
+      // `node1` is a returned value
+      // TODO: we don't pick up implicit returns with this approach
+      node1.asExpr().getExpr().getParent() = ret and
+      ret.getParent+() = helperMethod and
+      node2.asExpr() = helperMethodCall
+    )
+  }
+}

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

@@ -0,0 +1,39 @@
+/**
+ * Provides a taint-tracking configuration for detecting "reflected server-side cross-site scripting" vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if
+ * `ReflectedXSS::Configuration` is needed, otherwise
+ * `ReflectedXSSCustomizations` should be imported instead.
+ */
+
+private import ruby
+import codeql.ruby.DataFlow
+import codeql.ruby.TaintTracking
+
+/**
+ * Provides a taint-tracking configuration for detecting "reflected server-side cross-site scripting" vulnerabilities.
+ */
+module ReflectedXSS {
+  import ReflectedXSSCustomizations::ReflectedXSS
+
+  /**
+   * A taint-tracking configuration for detecting "reflected server-side cross-site scripting" vulnerabilities.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "ReflectedXSS" }
+
+    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 }
+
+    override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+      guard instanceof SanitizerGuard
+    }
+
+    override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+      isAdditionalXSSTaintStep(node1, node2)
+    }
+  }
+}

ql/src/queries/security/cwe-079/ReflectedXSS.qhelp

@@ -0,0 +1,59 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Directly writing user input (for example, an HTTP request parameter) to a webpage
+without properly sanitizing the input first, allows for a cross-site scripting
+vulnerability.
+</p>
+</overview>
+
+<recommendation>
+<p>
+To guard against cross-site scripting, consider escaping the input before
+writing user input to the page. In some frameworks, such as Rails, escaping will
+be performed implicitly and by default.
+</p>
+</recommendation>
+
+<example>
+<p>
+For instance, the following example is safe because the
+<code>params[:user_name]</code> content within the output tags will be
+automatically HTML escaped before being output.
+</p>
+<sample src="examples/safe.html.erb" />
+</example>
+
+<recommendation>
+<p>
+Care should be taken when using methods such as <code>html_safe</code> or
+<code>raw</code>. These methods can be used to output a string without escaping
+it. As such, they should only be used when the string has already been manually
+escaped (for example, with the Rails <code>html_escape</code> method), or when
+the content is otherwise guaranteed to be safe (such as a hard-coded string).
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example is unsafe because user-controlled input is output without
+escaping due to being marked as <code>html_safe</code>.
+</p>
+<sample src="examples/reflective_xss.html.erb" />
+</example>
+
+<references>
+<li>
+OWASP:
+<a href="https://cheatsheetseries.owasp.org/cheatsheets/Ruby_on_Rails_Cheat_Sheet.html#cross-site-scripting-xss">XSS
+Ruby on Rails Cheatsheet</a>.
+</li>
+<li>
+Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
+</li>
+</references>
+</qhelp>

ql/src/queries/security/cwe-079/ReflectedXSS.ql

@@ -0,0 +1,23 @@
+/**
+ * @name Reflected server-side cross-site scripting
+ * @description Writing user input directly to a web page
+ *              allows for a cross-site scripting vulnerability.
+ * @kind path-problem
+ * @problem.severity error
+ * @sub-severity high
+ * @precision high
+ * @id rb/reflected-xss
+ * @tags security
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-116
+ */
+
+import ruby
+import codeql.ruby.security.ReflectedXSSQuery
+import codeql.ruby.DataFlow
+import DataFlow::PathGraph
+
+from ReflectedXSS::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Cross-site scripting vulnerability due to $@.",
+  source.getNode(), "a user-provided value"

ql/src/queries/security/cwe-079/examples/reflective_xss.html.erb

@@ -0,0 +1 @@
+<p>Hello <%= params[:user_name].html_safe %>!</p>

ql/src/queries/security/cwe-079/examples/safe.html.erb

@@ -0,0 +1 @@
+<p>Hello <%= params[:user_name] %>!</p>

ql/test/query-tests/security/cwe-079/ReflectedXSS.expected

@@ -0,0 +1,44 @@
+edges
+| app/controllers/foo/bars_controller.rb:10:12:10:17 | call to params :  | app/controllers/foo/bars_controller.rb:10:12:10:29 | ...[...] :  |
+| app/controllers/foo/bars_controller.rb:10:12:10:29 | ...[...] :  | app/views/foo/bars/show.html.erb:49:5:49:13 | call to user_name |
+| app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params :  | app/controllers/foo/bars_controller.rb:20:53:20:54 | dt :  |
+| app/controllers/foo/bars_controller.rb:20:53:20:54 | dt :  | app/views/foo/bars/show.html.erb:5:9:5:21 | @display_text |
+| app/controllers/foo/bars_controller.rb:20:53:20:54 | dt :  | app/views/foo/bars/show.html.erb:8:9:8:20 | call to display_text |
+| app/controllers/foo/bars_controller.rb:20:53:20:54 | dt :  | app/views/foo/bars/show.html.erb:11:9:11:29 | ...[...] |
+| app/controllers/foo/bars_controller.rb:20:53:20:54 | dt :  | app/views/foo/bars/show.html.erb:15:9:15:19 | ...[...] |
+| app/controllers/foo/bars_controller.rb:20:53:20:54 | dt :  | app/views/foo/bars/show.html.erb:36:3:36:15 | @display_text |
+| app/controllers/foo/bars_controller.rb:20:53:20:54 | dt :  | app/views/foo/bars/show.html.erb:46:76:46:87 | call to display_text :  |
+| app/views/foo/bars/show.html.erb:46:64:46:87 | ... + ... :  | app/views/foo/bars/_widget.html.erb:2:9:2:21 | @display_text |
+| app/views/foo/bars/show.html.erb:46:64:46:87 | ... + ... :  | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text |
+| app/views/foo/bars/show.html.erb:46:64:46:87 | ... + ... :  | app/views/foo/bars/_widget.html.erb:8:9:8:29 | ...[...] |
+| app/views/foo/bars/show.html.erb:46:76:46:87 | call to display_text :  | app/views/foo/bars/show.html.erb:46:64:46:87 | ... + ... :  |
+| app/views/foo/bars/show.html.erb:56:29:56:34 | call to params :  | app/views/foo/bars/show.html.erb:56:29:56:44 | ...[...] |
+nodes
+| app/controllers/foo/bars_controller.rb:10:12:10:17 | call to params :  | semmle.label | call to params :  |
+| app/controllers/foo/bars_controller.rb:10:12:10:29 | ...[...] :  | semmle.label | ...[...] :  |
+| app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params :  | semmle.label | call to params :  |
+| app/controllers/foo/bars_controller.rb:20:53:20:54 | dt :  | semmle.label | dt :  |
+| app/views/foo/bars/_widget.html.erb:2:9:2:21 | @display_text | semmle.label | @display_text |
+| app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | semmle.label | call to display_text |
+| app/views/foo/bars/_widget.html.erb:8:9:8:29 | ...[...] | semmle.label | ...[...] |
+| app/views/foo/bars/show.html.erb:5:9:5:21 | @display_text | semmle.label | @display_text |
+| app/views/foo/bars/show.html.erb:8:9:8:20 | call to display_text | semmle.label | call to display_text |
+| app/views/foo/bars/show.html.erb:11:9:11:29 | ...[...] | semmle.label | ...[...] |
+| app/views/foo/bars/show.html.erb:15:9:15:19 | ...[...] | semmle.label | ...[...] |
+| app/views/foo/bars/show.html.erb:36:3:36:15 | @display_text | semmle.label | @display_text |
+| app/views/foo/bars/show.html.erb:46:64:46:87 | ... + ... :  | semmle.label | ... + ... :  |
+| app/views/foo/bars/show.html.erb:46:76:46:87 | call to display_text :  | semmle.label | call to display_text :  |
+| app/views/foo/bars/show.html.erb:49:5:49:13 | call to user_name | semmle.label | call to user_name |
+| app/views/foo/bars/show.html.erb:56:29:56:34 | call to params :  | semmle.label | call to params :  |
+| app/views/foo/bars/show.html.erb:56:29:56:44 | ...[...] | semmle.label | ...[...] |
+#select
+| app/views/foo/bars/_widget.html.erb:2:9:2:21 | @display_text | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params :  | app/views/foo/bars/_widget.html.erb:2:9:2:21 | @display_text | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
+| app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params :  | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
+| app/views/foo/bars/_widget.html.erb:8:9:8:29 | ...[...] | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params :  | app/views/foo/bars/_widget.html.erb:8:9:8:29 | ...[...] | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
+| app/views/foo/bars/show.html.erb:5:9:5:21 | @display_text | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params :  | app/views/foo/bars/show.html.erb:5:9:5:21 | @display_text | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
+| app/views/foo/bars/show.html.erb:8:9:8:20 | call to display_text | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params :  | app/views/foo/bars/show.html.erb:8:9:8:20 | call to display_text | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
+| app/views/foo/bars/show.html.erb:11:9:11:29 | ...[...] | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params :  | app/views/foo/bars/show.html.erb:11:9:11:29 | ...[...] | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
+| app/views/foo/bars/show.html.erb:15:9:15:19 | ...[...] | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params :  | app/views/foo/bars/show.html.erb:15:9:15:19 | ...[...] | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
+| app/views/foo/bars/show.html.erb:36:3:36:15 | @display_text | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params :  | app/views/foo/bars/show.html.erb:36:3:36:15 | @display_text | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:19:10:19:15 | call to params | a user-provided value |
+| app/views/foo/bars/show.html.erb:49:5:49:13 | call to user_name | app/controllers/foo/bars_controller.rb:10:12:10:17 | call to params :  | app/views/foo/bars/show.html.erb:49:5:49:13 | call to user_name | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:10:12:10:17 | call to params | a user-provided value |
+| app/views/foo/bars/show.html.erb:56:29:56:44 | ...[...] | app/views/foo/bars/show.html.erb:56:29:56:34 | call to params :  | app/views/foo/bars/show.html.erb:56:29:56:44 | ...[...] | Cross-site scripting vulnerability due to $@. | app/views/foo/bars/show.html.erb:56:29:56:34 | call to params | a user-provided value |

ql/test/query-tests/security/cwe-079/ReflectedXSS.qlref

@@ -0,0 +1 @@
+queries/security/cwe-079/ReflectedXSS.ql

ql/test/query-tests/security/cwe-079/app/controllers/foo/bars_controller.rb

@@ -0,0 +1,22 @@
+class BarsController < ApplicationController
+
+  helper_method :user_name, :user_name_memo
+
+  def index
+    render template: "foo/bars/index"
+  end
+
+  def user_name
+    return params[:user_name]
+  end
+
+  def user_name_memo
+    @user_name ||= params[:user_name]
+  end
+
+  def show
+    @user_website = params[:website]
+    dt = params[:text]
+    render "foo/bars/show", locals: { display_text: dt, safe_text: "hello" }
+  end
+end

ql/test/query-tests/security/cwe-079/app/views/foo/bars/_widget.html.erb

@@ -0,0 +1,11 @@
+<%# BAD: A local rendered raw as an instance variable %>
+<%= raw @display_text %>
+
+<%# BAD: A local rendered raw as a local variable %>
+<%= raw display_text %>
+
+<%# BAD: A local rendered raw via the locals hash %>
+<%= raw locals[:display_text] %>
+
+<%# GOOD: A local rendered with default escaping via the locals hash %>
+<%= @display_text %>

ql/test/query-tests/security/cwe-079/app/views/foo/bars/show.html.erb

@@ -0,0 +1,56 @@
+<%# BAD: An instance variable rendered without escaping %>
+<a href="<%= raw user_website %>">website</a>
+
+<%# BAD: A local rendered raw as an instance variable %>
+<%= raw @display_text %>
+
+<%# BAD: A local rendered raw as a local variable %>
+<%= raw display_text %>
+
+<%# BAD: A local rendered raw via the locals hash %>
+<%= raw locals[:display_text] %>
+
+<% key = :display_text %>
+<%# BAD: A local rendered raw via the locals hash %>
+<%= raw locals[key] %>
+
+<ul>
+<% for key in [:display_text, :safe_text] do %>
+  <%# BAD: A local rendered raw via the locals hash %>
+  <%# TODO: we miss that `key` can take `:display_text` as a value here %>
+  <li><%= raw locals[key] %></li>
+<% end %>
+</ul>
+
+<%# GOOD: A local rendered with default escaping via the locals hash %>
+<%= @display_text %>
+
+<%# GOOD: default escaping of rendered text %>
+<%=
+  full_text = prefix + locals[:display_text]
+  full_text
+%>
+
+<%# BAD: html_safe marks string as not requiring HTML escaping %>
+<%=
+  @display_text.html_safe
+%>
+
+<%# BAD: html_safe marks string as not requiring HTML escaping %>
+<%# TODO: we miss that `@display_text` is marked here %>
+<%=
+  @display_text.html_safe
+  @display_text
+%>
+
+<%= render partial: 'foo/bars/widget', locals: { display_text: "widget_" + display_text } %>
+
+<%# BAD: user_name is a helper method that returns unsanitized user-input %>
+<%= user_name.html_safe %>
+
+<%# BAD: user_name_memo is a helper method that returns unsanitized user-input %>
+<%# TODO: we miss this because the return value from user_name_memo is not properly linked to this call %>
+<%= user_name_memo.html_safe %>
+
+<%# BAD: unsanitized user-input should not be passed to link_to as the URL %>
+<%= link_to "user website", params[:website] %>