Restrict rb/reflected-xss instance variable taint edges

Author: alexrford

ql/lib/codeql/ruby/controlflow/CfgNodes.qll

@@ -355,6 +355,13 @@ module ExprNodes {
     final override VariableReadAccess getExpr() { result = ExprCfgNode.super.getExpr() }
   }
 
+  /** A control-flow node that wraps a `InstanceVariableWriteAccess` AST expression. */
+  class InstanceVariableWriteAccessCfgNode extends ExprCfgNode {
+    override InstanceVariableWriteAccess e;
+
+    final override InstanceVariableWriteAccess getExpr() { result = ExprCfgNode.super.getExpr() }
+  }
+
   /** A control-flow node that wraps a `StringInterpolationComponent` AST expression. */
   class StringInterpolationComponentCfgNode extends StmtSequenceCfgNode {
     StringInterpolationComponentCfgNode() { this.getNode() instanceof StringInterpolationComponent }

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

@@ -101,6 +101,27 @@ module ReflectedXSS {
   class StringConstArrayInclusionCallAsSanitizerGuard extends SanitizerGuard,
     StringConstArrayInclusionCall { }
 
+    /**
+     * A `VariableWriteAccessCfgNode` that is not succeeded (locally) by another
+     * write to that variable.
+     */
+    private class FinalInstanceVarWrite extends CfgNodes::ExprNodes::InstanceVariableWriteAccessCfgNode {
+      private InstanceVariable var;
+
+      FinalInstanceVarWrite() {
+        var = this.getExpr().getVariable() and
+        not exists(CfgNodes::ExprNodes::InstanceVariableWriteAccessCfgNode succWrite |
+          succWrite.getExpr().getVariable() = var |
+          succWrite = this.getASuccessor+()
+        )
+      }
+
+      InstanceVariable getVariable() { result = var }
+
+      AssignExpr getAnAssignExpr() { result.getLeftOperand() = this.getExpr() }
+    }
+
+
   /**
    * An additional step that is taint-preserving in the context of reflected XSS.
    */
@@ -136,17 +157,16 @@ module ReflectedXSS {
     or
     // instance variables in the controller
     exists(
-      ActionControllerActionMethod action, VariableReadAccess viewVarRead, AssignExpr ae,
-      InstanceVariableWriteAccess controllerVarWrite
+      ActionControllerActionMethod action, VariableReadAccess viewVarRead,
+      AssignExpr ae, FinalInstanceVarWrite controllerVarWrite
     |
       viewVarRead = node2.asExpr().(CfgNodes::ExprNodes::VariableReadAccessCfgNode).getExpr() and
       action.getDefaultTemplateFile() = viewVarRead.getLocation().getFile() and
       // match read to write on variable name
       viewVarRead.getVariable().getName() = controllerVarWrite.getVariable().getName() and
-      // TODO: include only final assignment along a path
       // propagate taint from assignment RHS expr to variable read access in view
+      ae = controllerVarWrite.getAnAssignExpr() and
       node1.asExpr().getExpr() = ae.getRightOperand() and
-      ae.getLeftOperand() = controllerVarWrite and
       ae.getParent+() = action
     )
     or

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

@@ -6,8 +6,6 @@ edges
 | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | app/controllers/foo/bars_controller.rb:19:22:19:23 | dt :  |
 | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | app/controllers/foo/bars_controller.rb:23:53:23:54 | dt :  |
 | app/controllers/foo/bars_controller.rb:19:22:19:23 | dt :  | app/views/foo/bars/show.html.erb:41:3:41:16 | @instance_text |
-| app/controllers/foo/bars_controller.rb:20:17:20:22 | call to params :  | app/controllers/foo/bars_controller.rb:20:17:20:29 | ...[...] :  |
-| app/controllers/foo/bars_controller.rb:20:17:20:29 | ...[...] :  | app/views/foo/bars/show.html.erb:63:5:63:13 | @safe_foo |
 | app/controllers/foo/bars_controller.rb:23:53:23:54 | dt :  | app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text |
 | app/controllers/foo/bars_controller.rb:23:53:23:54 | dt :  | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] |
 | app/controllers/foo/bars_controller.rb:23:53:23:54 | dt :  | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] |
@@ -25,8 +23,6 @@ nodes
 | app/controllers/foo/bars_controller.rb:17:21:17:36 | ...[...] :  | semmle.label | ...[...] :  |
 | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | semmle.label | call to params :  |
 | app/controllers/foo/bars_controller.rb:19:22:19:23 | dt :  | semmle.label | dt :  |
-| app/controllers/foo/bars_controller.rb:20:17:20:22 | call to params :  | semmle.label | call to params :  |
-| app/controllers/foo/bars_controller.rb:20:17:20:29 | ...[...] :  | semmle.label | ...[...] :  |
 | app/controllers/foo/bars_controller.rb:23:53:23:54 | dt :  | semmle.label | dt :  |
 | 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 | ...[...] |
@@ -43,7 +39,6 @@ nodes
 | app/views/foo/bars/show.html.erb:54:29:54:44 | ...[...] | semmle.label | ...[...] |
 | app/views/foo/bars/show.html.erb:57:13:57:18 | call to params :  | semmle.label | call to params :  |
 | app/views/foo/bars/show.html.erb:57:13:57:28 | ...[...] | semmle.label | ...[...] |
-| app/views/foo/bars/show.html.erb:63:5:63:13 | @safe_foo | semmle.label | @safe_foo |
 #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 $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | a 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 $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | a user-provided value |
@@ -56,4 +51,3 @@ nodes
 | app/views/foo/bars/show.html.erb:47:5:47:13 | call to user_name | app/controllers/foo/bars_controller.rb:9:12:9:17 | call to params :  | app/views/foo/bars/show.html.erb:47:5:47:13 | call to user_name | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:9:12:9:17 | call to params | a user-provided value |
 | 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 $@. | app/views/foo/bars/show.html.erb:54:29:54:34 | call to params | a 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 $@. | app/views/foo/bars/show.html.erb:57:13:57:18 | call to params | a user-provided value |
-| app/views/foo/bars/show.html.erb:63:5:63:13 | @safe_foo | app/controllers/foo/bars_controller.rb:20:17:20:22 | call to params :  | app/views/foo/bars/show.html.erb:63:5:63:13 | @safe_foo | Cross-site scripting vulnerability due to $@. | app/controllers/foo/bars_controller.rb:20:17:20:22 | call to params | a user-provided value |

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

@@ -59,7 +59,6 @@
 <% end %>
 
 <%# GOOD: @safe_foo is a hardcoded string here at runtime %>
-<%# TODO: we mistakenly flag this up due to overly broad flow through instance variables %>
 <%= @safe_foo.html_safe %>
 
 <%# GOOD: @html_escaped is manually escaped in the controller %>