Merge pull request github#10840 from erik-krogh/html_safe

RB: simplify html_safe modeling

Author: erik-krogh

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

@@ -311,14 +311,6 @@ private class ActionControllerRenderToCall extends ActionControllerContextCall,
   ActionControllerRenderToCall() { this.getMethodName() = ["render_to_body", "render_to_string"] }
 }
 
-/** A call to `html_safe` from within a controller. */
-private class ActionControllerHtmlSafeCall extends HtmlSafeCallImpl {
-  ActionControllerHtmlSafeCall() {
-    this.getMethodName() = "html_safe" and
-    this.getEnclosingModule() instanceof ActionControllerControllerClass
-  }
-}
-
 /** A call to `html_escape` from within a controller. */
 private class ActionControllerHtmlEscapeCall extends HtmlEscapeCallImpl {
   ActionControllerHtmlEscapeCall() {

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

@@ -39,11 +39,6 @@ predicate inActionViewContext(AstNode n) {
   n.getLocation().getFile() instanceof ErbFile
 }
 
-/** A call to `html_safe` from within a template. */
-private class ActionViewHtmlSafeCall extends HtmlSafeCallImpl {
-  ActionViewHtmlSafeCall() { this.getMethodName() = "html_safe" and inActionViewContext(this) }
-}
-
 /**
  * A call to a Rails method that escapes HTML.
  */

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

@@ -16,10 +16,13 @@ private import codeql.ruby.security.OpenSSL
  */
 module Rails {
   /**
+   * DEPRECATED: Any call to `html_safe` is considered an XSS sink.
    * A method call on a string to mark it as HTML safe for Rails. Strings marked
    * as such will not be automatically escaped when inserted into HTML.
    */
-  class HtmlSafeCall extends MethodCall instanceof HtmlSafeCallImpl { }
+  deprecated class HtmlSafeCall extends MethodCall {
+    HtmlSafeCall() { this.getMethodName() = "html_safe" }
+  }
 
   /** A call to a Rails method to escape HTML. */
   class HtmlEscapeCall extends MethodCall instanceof HtmlEscapeCallImpl { }

ruby/ql/lib/codeql/ruby/frameworks/internal/Rails.qll

@@ -1,7 +1,5 @@
 private import codeql.ruby.AST
 
-abstract class HtmlSafeCallImpl extends MethodCall { }
-
 abstract class HtmlEscapeCallImpl extends MethodCall { }
 
 abstract class RenderCallImpl extends MethodCall { }

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

@@ -62,10 +62,7 @@ private module Shared {
    */
   class HtmlSafeCallAsSink extends Sink {
     HtmlSafeCallAsSink() {
-      exists(Rails::HtmlSafeCall c, ErbOutputDirective d |
-        this.asExpr().getExpr() = c.getReceiver() and
-        c = d.getTerminalStmt()
-      )
+      this = any(DataFlow::CallNode call | call.getMethodName() = "html_safe").getReceiver()
     }
   }
 
@@ -342,14 +339,13 @@ private module OrmTracking {
 
     override predicate isSource(DataFlow2::Node source) { source instanceof OrmInstantiation }
 
-    // Select any call node and narrow down later
-    override predicate isSink(DataFlow2::Node sink) { sink instanceof DataFlow2::CallNode }
+    // Select any call receiver and narrow down later
+    override predicate isSink(DataFlow2::Node sink) {
+      sink = any(DataFlow2::CallNode c).getReceiver()
+    }
 
     override predicate isAdditionalFlowStep(DataFlow2::Node node1, DataFlow2::Node node2) {
       Shared::isAdditionalXssFlowStep(node1, node2)
-      or
-      // Propagate flow through arbitrary method calls
-      node2.(DataFlow2::CallNode).getReceiver() = node1
     }
   }
 }
@@ -382,10 +378,9 @@ module StoredXss {
 
   private class OrmFieldAsSource extends Source instanceof DataFlow2::CallNode {
     OrmFieldAsSource() {
-      exists(OrmTracking::Configuration subConfig, DataFlow2::CallNode subSrc, MethodCall call |
-        subConfig.hasFlow(subSrc, this) and
-        call = this.asExpr().getExpr() and
-        subSrc.(OrmInstantiation).methodCallMayAccessField(call.getMethodName())
+      exists(OrmTracking::Configuration subConfig, DataFlow2::CallNode subSrc |
+        subConfig.hasFlow(subSrc, this.getReceiver()) and
+        subSrc.(OrmInstantiation).methodCallMayAccessField(this.getMethodName())
       )
     }
   }

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

@@ -1,6 +1,3 @@
-htmlSafeCalls
-| app/views/foo/bars/show.html.erb:23:3:23:25 | call to html_safe |
-| app/views/foo/bars/show.html.erb:27:3:27:25 | call to html_safe |
 rawCalls
 | app/views/foo/bars/_widget.html.erb:1:5:1:21 | call to raw |
 | app/views/foo/bars/_widget.html.erb:2:5:2:20 | call to raw |

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

@@ -4,8 +4,6 @@ private import codeql.ruby.frameworks.ActionView
 private import codeql.ruby.frameworks.Rails
 private import codeql.ruby.Concepts
 
-query predicate htmlSafeCalls(Rails::HtmlSafeCall c) { any() }
-
 query predicate rawCalls(RawCall c) { any() }
 
 query predicate renderCalls(Rails::RenderCall c) { any() }

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

@@ -19,6 +19,8 @@ edges
 | app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] |
 | app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text |
 | app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:44:76:44:87 | call to display_text :  |
+| app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params :  | app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] :  |
+| app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] :  | app/controllers/foo/bars_controller.rb:31:5:31:7 | str |
 | app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... :  | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text |
 | app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... :  | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] |
 | app/views/foo/bars/show.html.erb:44:76:44:87 | call to display_text :  | app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... :  |
@@ -41,6 +43,9 @@ nodes
 | app/controllers/foo/bars_controller.rb:24:39:24:59 | ... = ... | semmle.label | ... = ... |
 | app/controllers/foo/bars_controller.rb:24:39:24:59 | ...[...] :  | semmle.label | ...[...] :  |
 | app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | semmle.label | dt :  |
+| app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params :  | semmle.label | call to params :  |
+| app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] :  | semmle.label | ...[...] :  |
+| app/controllers/foo/bars_controller.rb:31:5:31:7 | str | semmle.label | str |
 | 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:36 | ...[...] | semmle.label | ...[...] |
 | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | semmle.label | @user_website |
@@ -64,6 +69,7 @@ nodes
 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 |
+| app/controllers/foo/bars_controller.rb:31:5:31:7 | str | app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params :  | app/controllers/foo/bars_controller.rb:31:5:31:7 | str | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params | user-provided value |
 | 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 |
 | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | app/controllers/foo/bars_controller.rb:17:21:17:26 | call to params :  | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:17:21:17:26 | call to params | user-provided value |

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

@@ -25,4 +25,9 @@ def show
     response.header["x-customer-header"] = params[:bar] # OK - header not relevant to XSS
     render "foo/bars/show", locals: { display_text: dt, safe_text: "hello" }
   end
+
+  def make_safe_html
+    str = params[:user_name]
+    str.html_safe
+  end
 end

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

@@ -81,3 +81,6 @@
 
 <%# BAD: Indirect to a database value without escaping %>
 <%= @other_user_raw_name.html_safe %>
+
+<%# GOOD: The `foo.bar.baz` is not recognized as a source %>
+<%= @other_user_raw_name.foo.bar.baz.html_safe %>