Ruby: Model javascript_include_tag and friends

Author: hmac

ruby/ql/lib/codeql/ruby/frameworks/ActionView.qll

@@ -218,114 +218,134 @@ class FileSystemResolverAccess extends DataFlow::CallNode, FileSystemAccess::Ran
 }
 
 // TODO: model flow in/out of template files properly,
-//
-/**
- * Action view helper methods which are XSS sinks.
- */
-module ActionViewHelpers {
+// TODO: Move the classes and predicates above inside this module.
+/** Modeling for `ActionView`. */
+module ActionView {
   /**
-   * Calls to ActionView helpers which render their argument without escaping.
-   * These arguments should be treated as XSS sinks.
-   * In the documentation for classes in this module, the vulnerable argument is
-   * named `x`.
+   * Action view helper methods which are XSS sinks.
    */
-  abstract class RawHelperCall extends MethodCall {
-    abstract Expr getRawArgument();
-  }
-
-  /**
-   * `ActionView::Helpers::TextHelper#simple_format`.
-   *
-   * `simple_format(x, y, sanitize: false)`.
-   */
-  private class SimpleFormat extends ActionViewContextCall, RawHelperCall {
-    SimpleFormat() {
-      this.getMethodName() = "simple_format" and
-      this.getKeywordArgument("sanitize").getConstantValue().isBoolean(false)
+  module Helpers {
+    /**
+     * Calls to ActionView helpers which render their argument without escaping.
+     * These arguments should be treated as XSS sinks.
+     * In the documentation for classes in this module, the vulnerable argument is
+     * named `x`.
+     */
+    abstract class RawHelperCall extends MethodCall {
+      abstract Expr getRawArgument();
     }
 
-    override Expr getRawArgument() { result = this.getArgument(0) }
-  }
-
-  /**
-   * `ActionView::Helpers::TextHelper#truncate`.
-   *
-   * `truncate(x, escape: false)`.
-   */
-  private class Truncate extends ActionViewContextCall, RawHelperCall {
-    Truncate() {
-      this.getMethodName() = "truncate" and
-      this.getKeywordArgument("escape").getConstantValue().isBoolean(false)
+    /**
+     * `ActionView::Helpers::TextHelper#simple_format`.
+     *
+     * `simple_format(x, y, sanitize: false)`.
+     */
+    private class SimpleFormat extends ActionViewContextCall, RawHelperCall {
+      SimpleFormat() {
+        this.getMethodName() = "simple_format" and
+        this.getKeywordArgument("sanitize").getConstantValue().isBoolean(false)
+      }
+
+      override Expr getRawArgument() { result = this.getArgument(0) }
     }
 
-    override Expr getRawArgument() { result = this.getArgument(0) }
-  }
-
-  /**
-   * `ActionView::Helpers::TextHelper#highlight`.
-   *
-   * `truncate(x, y, sanitize: false)`.
-   */
-  private class Highlight extends ActionViewContextCall, RawHelperCall {
-    Highlight() {
-      this.getMethodName() = "highlight" and
-      this.getKeywordArgument("sanitize").getConstantValue().isBoolean(false)
+    /**
+     * `ActionView::Helpers::TextHelper#truncate`.
+     *
+     * `truncate(x, escape: false)`.
+     */
+    private class Truncate extends ActionViewContextCall, RawHelperCall {
+      Truncate() {
+        this.getMethodName() = "truncate" and
+        this.getKeywordArgument("escape").getConstantValue().isBoolean(false)
+      }
+
+      override Expr getRawArgument() { result = this.getArgument(0) }
     }
 
-    override Expr getRawArgument() { result = this.getArgument(0) }
-  }
-
-  /**
-   * `ActionView::Helpers::JavascriptHelper#javascript_tag`.
-   *
-   * `javascript_tag(x)`.
-   */
-  private class JavascriptTag extends ActionViewContextCall, RawHelperCall {
-    JavascriptTag() { this.getMethodName() = "javascript_tag" }
+    /**
+     * `ActionView::Helpers::TextHelper#highlight`.
+     *
+     * `truncate(x, y, sanitize: false)`.
+     */
+    private class Highlight extends ActionViewContextCall, RawHelperCall {
+      Highlight() {
+        this.getMethodName() = "highlight" and
+        this.getKeywordArgument("sanitize").getConstantValue().isBoolean(false)
+      }
+
+      override Expr getRawArgument() { result = this.getArgument(0) }
+    }
 
-    override Expr getRawArgument() { result = this.getArgument(0) }
-  }
+    /**
+     * `ActionView::Helpers::JavascriptHelper#javascript_tag`.
+     *
+     * `javascript_tag(x)`.
+     */
+    private class JavascriptTag extends ActionViewContextCall, RawHelperCall {
+      JavascriptTag() { this.getMethodName() = "javascript_tag" }
 
-  /**
-   * `ActionView::Helpers::TagHelper#tag`.
-   *
-   * `tag(x, x, y, false)`.
-   */
-  private class ContentTag extends ActionViewContextCall, RawHelperCall {
-    ContentTag() {
-      this.getMethodName() = "content_tag" and
-      this.getArgument(3).getConstantValue().isBoolean(false)
+      override Expr getRawArgument() { result = this.getArgument(0) }
     }
 
-    override Expr getRawArgument() { result = this.getArgument(1) }
-  }
+    /**
+     * `ActionView::Helpers::TagHelper#tag`.
+     *
+     * `tag(x, x, y, false)`.
+     */
+    private class ContentTag extends ActionViewContextCall, RawHelperCall {
+      ContentTag() {
+        this.getMethodName() = "content_tag" and
+        this.getArgument(3).getConstantValue().isBoolean(false)
+      }
+
+      override Expr getRawArgument() { result = this.getArgument(1) }
+    }
 
-  /**
-   * `ActionView::Helpers::TagHelper#tag`.
-   *
-   * `tag(x, x, y, false)`.
-   */
-  private class Tag extends ActionViewContextCall, RawHelperCall {
-    Tag() {
-      this.getMethodName() = "tag" and
-      this.getArgument(3).getConstantValue().isBoolean(false)
+    /**
+     * `ActionView::Helpers::TagHelper#tag`.
+     *
+     * `tag(x, x, y, false)`.
+     */
+    private class Tag extends ActionViewContextCall, RawHelperCall {
+      Tag() {
+        this.getMethodName() = "tag" and
+        this.getArgument(3).getConstantValue().isBoolean(false)
+      }
+
+      override Expr getRawArgument() { result = this.getArgument(0) }
     }
 
-    override Expr getRawArgument() { result = this.getArgument(0) }
+    /**
+     * `ActionView::Helpers::TagHelper#tag.<tag name>`.
+     *
+     * `tag.h1(x, escape: false)`.
+     */
+    private class TagMethod extends MethodCall, RawHelperCall {
+      TagMethod() {
+        inActionViewContext(this) and
+        this.getReceiver().(MethodCall).getMethodName() = "tag" and
+        this.getKeywordArgument("escape").getConstantValue().isBoolean(false)
+      }
+
+      override Expr getRawArgument() { result = this.getArgument(0) }
+    }
   }
 
   /**
-   * `ActionView::Helpers::TagHelper#tag.<tag name>`.
-   *
-   * `tag.h1(x, escape: false)`.
+   * An argument to a method call which constructs a script tag, interpreting the
+   * argument as a URL. Remote input flowing to this argument may allow loading of
+   * arbitrary javascript.
    */
-  private class TagMethod extends MethodCall, RawHelperCall {
-    TagMethod() {
-      inActionViewContext(this) and
-      this.getReceiver().(MethodCall).getMethodName() = "tag" and
-      this.getKeywordArgument("escape").getConstantValue().isBoolean(false)
+  class ArgumentInterpretedAsUrl extends DataFlow::Node {
+    ArgumentInterpretedAsUrl() {
+      exists(DataFlow::CallNode call |
+        call.getMethodName() = ["javascript_include_tag", "javascript_path", "path_to_javascript"] and
+        this = call.getArgument(0)
+        or
+        call.getMethodName() = "javascript_url" and
+        this = call.getKeywordArgument("host")
+      )
     }
-
-    override Expr getRawArgument() { result = this.getArgument(0) }
   }
 }

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

@@ -79,14 +79,21 @@ private module Shared {
    * An argument to an ActionView helper method which is not escaped,
    * considered as a flow sink.
    */
-  class RawHelperCallArgumentAsSink extends Sink, ErbOutputMethodCallArgumentNode {
+  class RawHelperCallArgumentAsSink extends Sink {
     RawHelperCallArgumentAsSink() {
-      exists(ErbOutputDirective d, ActionViewHelpers::RawHelperCall c |
+      exists(ErbOutputDirective d, ActionView::Helpers::RawHelperCall c |
         d.getTerminalStmt() = c and this.asExpr().getExpr() = c.getRawArgument()
       )
     }
   }
 
+  /**
+   * An argument that is used to construct the `src` attribute of a `<script>`
+   * tag.
+   */
+  class ArgumentInterpretedAsUrlAsSink extends Sink, ErbOutputMethodCallArgumentNode,
+    ActionView::ArgumentInterpretedAsUrl { }
+
   /**
    * A argument to a call to the `link_to` method, which does not expect
    * unsanitized user-input, considered as a flow sink.

ruby/ql/test/library-tests/frameworks/ActionView.ql

@@ -18,6 +18,6 @@ query predicate httpResponses(Http::Server::HttpResponse r, DataFlow::Node body,
   r.getBody() = body and r.getMimetype() = mimeType
 }
 
-query predicate rawHelperCalls(ActionViewHelpers::RawHelperCall c, Expr arg) {
+query predicate rawHelperCalls(ActionView::Helpers::RawHelperCall c, Expr arg) {
   arg = c.getRawArgument()
 }

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

@@ -23,6 +23,7 @@ edges
 | app/views/foo/bars/show.html.erb:54:29:54:34 | call to params :  | app/views/foo/bars/show.html.erb:54:29:54:44 | ...[...] |
 | app/views/foo/bars/show.html.erb:57:13:57:18 | call to params :  | app/views/foo/bars/show.html.erb:57:13:57:28 | ...[...] |
 | app/views/foo/bars/show.html.erb:74:19:74:24 | call to params :  | app/views/foo/bars/show.html.erb:74:19:74:34 | ...[...] |
+| app/views/foo/bars/show.html.erb:77:28:77:33 | call to params :  | app/views/foo/bars/show.html.erb:77:28:77:39 | ...[...] |
 nodes
 | app/controllers/foo/bars_controller.rb:9:12:9:17 | call to params :  | semmle.label | call to params :  |
 | app/controllers/foo/bars_controller.rb:9:12:9:29 | ...[...] :  | semmle.label | ...[...] :  |
@@ -53,6 +54,8 @@ nodes
 | app/views/foo/bars/show.html.erb:57:13:57:28 | ...[...] | semmle.label | ...[...] |
 | app/views/foo/bars/show.html.erb:74:19:74:24 | call to params :  | semmle.label | call to params :  |
 | app/views/foo/bars/show.html.erb:74:19:74:34 | ...[...] | semmle.label | ...[...] |
+| app/views/foo/bars/show.html.erb:77:28:77:33 | call to params :  | semmle.label | call to params :  |
+| app/views/foo/bars/show.html.erb:77:28:77:39 | ...[...] | semmle.label | ...[...] |
 subpaths
 #select
 | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | app/controllers/foo/bars_controller.rb:18:10:18: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 a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
@@ -68,3 +71,4 @@ subpaths
 | app/views/foo/bars/show.html.erb:54:29:54:44 | ...[...] | app/views/foo/bars/show.html.erb:54:29:54:34 | call to params :  | app/views/foo/bars/show.html.erb:54:29:54:44 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:54:29:54:34 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:57:13:57:28 | ...[...] | app/views/foo/bars/show.html.erb:57:13:57:18 | call to params :  | app/views/foo/bars/show.html.erb:57:13:57:28 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:57:13:57:18 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:74:19:74:34 | ...[...] | app/views/foo/bars/show.html.erb:74:19:74:24 | call to params :  | app/views/foo/bars/show.html.erb:74:19:74:34 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:74:19:74:24 | call to params | user-provided value |
+| app/views/foo/bars/show.html.erb:77:28:77:39 | ...[...] | app/views/foo/bars/show.html.erb:77:28:77:33 | call to params :  | app/views/foo/bars/show.html.erb:77:28:77:39 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:77:28:77:33 | call to params | user-provided value |

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

@@ -72,3 +72,6 @@
 
 <%# BAD: simple_format called with sanitize: false %>
 <%= simple_format(params[:comment], sanitize: false) %>
+
+<%# BAD: javasript_include_tag called with remote input %>
+<%= javascript_include_tag params[:url] %>