Ruby: Improve UrlRedirect query using Rails routes

Handlers for non-GET requests aren't vulnerable to URL redirect attacks,
because browsers won't initiate non-GET requests when you click a link.

We can use Rails routing information, if present, to filter out any
handlers for non-GET requests.

Author: hmac

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

@@ -10,6 +10,7 @@ private import codeql.ruby.Concepts
 private import codeql.ruby.dataflow.RemoteFlowSources
 private import codeql.ruby.dataflow.BarrierGuards
 private import codeql.ruby.dataflow.Sanitizers
+private import codeql.ruby.frameworks.ActionController
 
 /**
  * Provides default sources, sinks and sanitizers for detecting
@@ -54,15 +55,25 @@ module UrlRedirect {
    */
   class RedirectLocationAsSink extends Sink {
     RedirectLocationAsSink() {
-      exists(HTTP::Server::HttpRedirectResponse e |
+      exists(HTTP::Server::HttpRedirectResponse e, MethodBase method |
         this = e.getRedirectLocation() and
-        // As a rough heuristic, assume that methods with these names are handlers for POST/PUT/PATCH/DELETE requests,
-        // which are not as vulnerable to URL redirection because browsers will not initiate them from clicking a link.
-        not this.asExpr()
-            .getExpr()
-            .getEnclosingMethod()
-            .getName()
-            .regexpMatch(".*(create|update|destroy).*")
+        // We only want handlers for GET requests.
+        // Handlers for other HTTP methods are not as vulnerable to URL
+        // redirection as browsers will not initiate them from clicking a link.
+        method = this.asExpr().getExpr().getEnclosingMethod() and
+        (
+          // If there's a Rails GET route to this handler, we can be certain that it is a candiate.
+          method.(ActionControllerActionMethod).getARoute().getHTTPMethod() = "get"
+          or
+          // Otherwise, we have to rely on a heuristic to filter out invulnerable handlers.
+          // We exclude any handlers with names containing create/update/destroy, as these are not likely to handle GET requests.
+          not exists(method.(ActionControllerActionMethod).getARoute()) and
+          not this.asExpr()
+              .getExpr()
+              .getEnclosingMethod()
+              .getName()
+              .regexpMatch(".*(create|update|destroy).*")
+        )
       )
     }
   }

ruby/ql/test/query-tests/security/cwe-601/UrlRedirect.expected

@@ -3,10 +3,11 @@ edges
 | UrlRedirect.rb:14:17:14:22 | call to params :  | UrlRedirect.rb:14:17:14:43 | call to fetch |
 | UrlRedirect.rb:19:17:19:22 | call to params :  | UrlRedirect.rb:19:17:19:37 | call to to_unsafe_hash |
 | UrlRedirect.rb:24:31:24:36 | call to params :  | UrlRedirect.rb:24:17:24:37 | call to filter_params |
-| UrlRedirect.rb:24:31:24:36 | call to params :  | UrlRedirect.rb:56:21:56:32 | input_params :  |
+| UrlRedirect.rb:24:31:24:36 | call to params :  | UrlRedirect.rb:63:21:63:32 | input_params :  |
 | UrlRedirect.rb:34:20:34:25 | call to params :  | UrlRedirect.rb:34:20:34:31 | ...[...] :  |
 | UrlRedirect.rb:34:20:34:31 | ...[...] :  | UrlRedirect.rb:34:17:34:37 | "#{...}/foo" |
-| UrlRedirect.rb:56:21:56:32 | input_params :  | UrlRedirect.rb:57:5:57:29 | call to permit :  |
+| UrlRedirect.rb:58:17:58:22 | call to params :  | UrlRedirect.rb:58:17:58:28 | ...[...] |
+| UrlRedirect.rb:63:21:63:32 | input_params :  | UrlRedirect.rb:64:5:64:29 | call to permit :  |
 nodes
 | UrlRedirect.rb:4:17:4:22 | call to params | semmle.label | call to params |
 | UrlRedirect.rb:9:17:9:22 | call to params :  | semmle.label | call to params :  |
@@ -20,14 +21,17 @@ nodes
 | UrlRedirect.rb:34:17:34:37 | "#{...}/foo" | semmle.label | "#{...}/foo" |
 | UrlRedirect.rb:34:20:34:25 | call to params :  | semmle.label | call to params :  |
 | UrlRedirect.rb:34:20:34:31 | ...[...] :  | semmle.label | ...[...] :  |
-| UrlRedirect.rb:56:21:56:32 | input_params :  | semmle.label | input_params :  |
-| UrlRedirect.rb:57:5:57:29 | call to permit :  | semmle.label | call to permit :  |
+| UrlRedirect.rb:58:17:58:22 | call to params :  | semmle.label | call to params :  |
+| UrlRedirect.rb:58:17:58:28 | ...[...] | semmle.label | ...[...] |
+| UrlRedirect.rb:63:21:63:32 | input_params :  | semmle.label | input_params :  |
+| UrlRedirect.rb:64:5:64:29 | call to permit :  | semmle.label | call to permit :  |
 subpaths
-| UrlRedirect.rb:24:31:24:36 | call to params :  | UrlRedirect.rb:56:21:56:32 | input_params :  | UrlRedirect.rb:57:5:57:29 | call to permit :  | UrlRedirect.rb:24:17:24:37 | call to filter_params |
+| UrlRedirect.rb:24:31:24:36 | call to params :  | UrlRedirect.rb:63:21:63:32 | input_params :  | UrlRedirect.rb:64:5:64:29 | call to permit :  | UrlRedirect.rb:24:17:24:37 | call to filter_params |
 #select
 | UrlRedirect.rb:4:17:4:22 | call to params | UrlRedirect.rb:4:17:4:22 | call to params | UrlRedirect.rb:4:17:4:22 | call to params | Untrusted URL redirection due to $@. | UrlRedirect.rb:4:17:4:22 | call to params | a user-provided value |
 | UrlRedirect.rb:9:17:9:28 | ...[...] | UrlRedirect.rb:9:17:9:22 | call to params :  | UrlRedirect.rb:9:17:9:28 | ...[...] | Untrusted URL redirection due to $@. | UrlRedirect.rb:9:17:9:22 | call to params | a user-provided value |
 | UrlRedirect.rb:14:17:14:43 | call to fetch | UrlRedirect.rb:14:17:14:22 | call to params :  | UrlRedirect.rb:14:17:14:43 | call to fetch | Untrusted URL redirection due to $@. | UrlRedirect.rb:14:17:14:22 | call to params | a user-provided value |
 | UrlRedirect.rb:19:17:19:37 | call to to_unsafe_hash | UrlRedirect.rb:19:17:19:22 | call to params :  | UrlRedirect.rb:19:17:19:37 | call to to_unsafe_hash | Untrusted URL redirection due to $@. | UrlRedirect.rb:19:17:19:22 | call to params | a user-provided value |
 | UrlRedirect.rb:24:17:24:37 | call to filter_params | UrlRedirect.rb:24:31:24:36 | call to params :  | UrlRedirect.rb:24:17:24:37 | call to filter_params | Untrusted URL redirection due to $@. | UrlRedirect.rb:24:31:24:36 | call to params | a user-provided value |
 | UrlRedirect.rb:34:17:34:37 | "#{...}/foo" | UrlRedirect.rb:34:20:34:25 | call to params :  | UrlRedirect.rb:34:17:34:37 | "#{...}/foo" | Untrusted URL redirection due to $@. | UrlRedirect.rb:34:20:34:25 | call to params | a user-provided value |
+| UrlRedirect.rb:58:17:58:28 | ...[...] | UrlRedirect.rb:58:17:58:22 | call to params :  | UrlRedirect.rb:58:17:58:28 | ...[...] | Untrusted URL redirection due to $@. | UrlRedirect.rb:58:17:58:22 | call to params | a user-provided value |

ruby/ql/test/query-tests/security/cwe-601/UrlRedirect.rb

@@ -45,9 +45,16 @@ def route8
   end
 
   # GOOD
-  # Technically vulnerable, but we assume this is a handler for a POST request,
+  # Technically vulnerable, this is a handler for a POST request,
   # so can't be triggered by following a link.
-  def create
+  def create1
+    redirect_to params[:key]
+  end
+
+  # BAD
+  # The same as `create1` but this is reachable via a GET request, as configured
+  # by the routes at the top of this file.
+  def route9
     redirect_to params[:key]
   end
 
@@ -57,3 +64,7 @@ def filter_params(input_params)
     input_params.permit(:key)
   end
 end
+
+Rails.routes.draw do
+  get "/r9", to: "users#route9"
+end