Merge pull request #15520 from hmac/hmac-erb-raw-output-directive

Ruby: Recognise raw Erb output as XSS sink

ruby/ql/lib/change-notes/2024-02-12-raw-erb-output.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Raw output ERB tags of the form `<%== ... %>` are now recognised as cross-site scripting sinks.

ruby/ql/lib/codeql/ruby/ast/Erb.qll

@@ -256,10 +256,30 @@ class ErbOutputDirective extends ErbDirective {
 
   override ErbCode getToken() { toGenerated(result) = g.getChild() }
 
+  /**
+   * Holds if this is a raw Erb output directive.
+   * ```erb
+   * <%== foo %>
+   * ```
+   */
+  predicate isRaw() {
+    exists(Erb::Token t | t.getParentIndex() = 0 and t.getParent() = g and t.getValue() = "<%==")
+  }
+
   final override string toString() {
-    result = "<%=" + this.getToken().toString() + "%>"
+    this.isRaw() and
+    (
+      result = "<%==" + this.getToken().toString() + "%>"
+      or
+      not exists(this.getToken()) and result = "<%==%>"
+    )
     or
-    not exists(this.getToken()) and result = "<%=%>"
+    not this.isRaw() and
+    (
+      result = "<%=" + this.getToken().toString() + "%>"
+      or
+      not exists(this.getToken()) and result = "<%=%>"
+    )
   }
 
   final override string getAPrimaryQlClass() { result = "ErbOutputDirective" }

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

@@ -48,6 +48,18 @@ private module Shared {
     MethodCall getCall() { result = call }
   }
 
+  /**
+   * A value interpolated using a raw erb output directive, which does not perform HTML escaping.
+   * ```erb
+   * <%== sink %>
+   * ```
+   */
+  class ErbRawOutputDirective extends Sink {
+    ErbRawOutputDirective() {
+      exists(ErbOutputDirective d | d.isRaw() | this.asExpr().getExpr() = d.getTerminalStmt())
+    }
+  }
+
   /**
    * An `html_safe` call marking the output as not requiring HTML escaping,
    * considered as a flow sink.

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

@@ -21,6 +21,7 @@ edges
 | app/controllers/foo/bars_controller.rb:26:53:26:54 | dt | app/views/foo/bars/show.html.erb:17:15:17:27 | call to local_assigns [element :display_text] | provenance |  |
 | app/controllers/foo/bars_controller.rb:26:53:26:54 | dt | app/views/foo/bars/show.html.erb:35:3:35:14 | call to display_text | provenance |  |
 | app/controllers/foo/bars_controller.rb:26:53:26:54 | dt | app/views/foo/bars/show.html.erb:43:76:43:87 | call to display_text | provenance |  |
+| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt | app/views/foo/bars/show.html.erb:82:6:82:17 | call to display_text | provenance |  |
 | app/controllers/foo/bars_controller.rb:30:5:30:7 | str | app/controllers/foo/bars_controller.rb:31:5:31:7 | str | provenance |  |
 | app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params | app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] | provenance |  |
 | app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] | app/controllers/foo/bars_controller.rb:30:5:30:7 | str | provenance |  |
@@ -90,6 +91,7 @@ nodes
 | app/views/foo/bars/show.html.erb:73:19:73:34 | ...[...] | semmle.label | ...[...] |
 | app/views/foo/bars/show.html.erb:76:28:76:33 | call to params | semmle.label | call to params |
 | app/views/foo/bars/show.html.erb:76:28:76:39 | ...[...] | semmle.label | ...[...] |
+| app/views/foo/bars/show.html.erb:82:6:82:17 | call to display_text | semmle.label | call to display_text |
 subpaths
 #select
 | app/controllers/foo/bars_controller.rb:24:39:24:59 | ... = ... | app/controllers/foo/bars_controller.rb:24:39:24:44 | call to params | app/controllers/foo/bars_controller.rb:24:39:24:59 | ... = ... | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:24:39:24:44 | call to params | user-provided value |
@@ -109,3 +111,4 @@ subpaths
 | app/views/foo/bars/show.html.erb:56:13:56:28 | ...[...] | app/views/foo/bars/show.html.erb:56:13:56:18 | call to params | app/views/foo/bars/show.html.erb:56:13:56:28 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:56:13:56:18 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:73:19:73:34 | ...[...] | app/views/foo/bars/show.html.erb:73:19:73:24 | call to params | app/views/foo/bars/show.html.erb:73:19:73:34 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:73:19:73:24 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:76:28:76:39 | ...[...] | app/views/foo/bars/show.html.erb:76:28:76:33 | call to params | app/views/foo/bars/show.html.erb:76:28:76:39 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:76:28:76:33 | call to params | user-provided value |
+| app/views/foo/bars/show.html.erb:82:6:82:17 | call to display_text | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | app/views/foo/bars/show.html.erb:82:6:82:17 | call to display_text | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |

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

@@ -77,3 +77,6 @@
 
 <%# GOOD: input is sanitized %>
 <%= sanitize(params[:comment]).html_safe %>
+
+<%# BAD: A local rendered raw as a local variable %>
+<%== display_text %>