simplify the modeling of html_safe. Any call to html_safe is now considered an XSS sink

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()
     }
   }
 

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