Merge pull request #16111 from erik-krogh/rb-url

RB: Improve QHelp for `rb/url-redirect`, and fix an FP.

ruby/ql/lib/codeql/ruby/ast/internal/Constant.qll

@@ -560,6 +560,10 @@ private predicate isArrayExpr(Expr e, ArrayLiteralCfgNode arr) {
   // Note(hmac): I don't think this is necessary, as `getSource` will not return
   // results if the source is a phi node.
   forex(ExprCfgNode n | n = e.getAControlFlowNode() | isArrayConstant(n, arr))
+  or
+  // if `e` is an array, then `e.freeze` is also an array
+  e.(MethodCall).getMethodName() = "freeze" and
+  isArrayExpr(e.(MethodCall).getReceiver(), arr)
 }
 
 private class TokenConstantAccess extends ConstantAccess, TTokenConstantAccess {

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

@@ -83,6 +83,12 @@ module UrlRedirect {
    */
   class StringConstCompareAsSanitizer extends Sanitizer, StringConstCompareBarrier { }
 
+  /**
+   * A string concatenation against a constant list, considered as a sanitizer-guard.
+   */
+  class StringConstArrayInclusionAsSanitizer extends Sanitizer, StringConstArrayInclusionCallBarrier
+  { }
+
   /**
    * Some methods will propagate taint to their return values.
    * Here we cover a few common ones related to `ActionController::Parameters`.

ruby/ql/src/queries/security/cwe-601/UrlRedirect.qhelp

@@ -1,44 +1,68 @@
-<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
 <qhelp>
 
+
 <overview>
-<p>
-Directly incorporating user input into a URL redirect request without validating the input
+<p>Directly incorporating user input into a URL redirect request without validating the input
 can facilitate phishing attacks. In these attacks, unsuspecting users can be redirected to a
 malicious site that looks very similar to the real site they intend to visit, but which is
-controlled by the attacker.
-</p>
-</overview>
+controlled by the attacker.</p>
 
+</overview>
 <recommendation>
-<p>
-To guard against untrusted URL redirection, it is advisable to avoid putting user input
+
+<p>To guard against untrusted URL redirection, it is advisable to avoid putting user input
 directly into a redirect URL. Instead, maintain a list of authorized
-redirects on the server; then choose from that list based on the user input provided.
+redirects on the server; then choose from that list based on the user input provided.</p>
+<p>
+If this is not possible, then the user input should be validated in some other way,
+for example, by verifying that the target URL is on the same host as the current page.
 </p>
 </recommendation>
 
+
 <example>
 <p>
 The following example shows an HTTP request parameter being used directly in a URL redirect
 without validating the input, which facilitates phishing attacks:
 </p>
 
-<sample src="examples/redirect_bad.rb"/>
+<sample src="examples/url_redirect.rb"/>
 
 <p>
-One way to remedy the problem is to validate the user input against a known fixed string
+One way to remedy the problem is to validate the user input against a set of known fixed strings
 before doing the redirection:
 </p>
 
-<sample src="examples/redirect_good.rb"/>
+<sample src="examples/url_redirect_good.rb"/>
+
+<p>
+Alternatively, we can check that the target URL does not redirect to a different host
+by checking that the URL is either relative or on a known good host:
+</p>
+
+<sample src="examples/url_redirect_domain.rb"/>
+
+<p>
+Note that as written, the above code will allow redirects to URLs on <code>example.com</code>,
+which is harmless but perhaps not intended. You can substitute your own domain (if known) for
+<code>example.com</code> to prevent this.
+</p>
+
 </example>
 
 <references>
-<li>OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html">
-  XSS Unvalidated Redirects and Forwards Cheat Sheet</a>.</li>
-<li>Rails Guides: <a href="https://guides.rubyonrails.org/security.html#redirection-and-files">
-  Redirection and Files</a>.</li>
-</references>
 
+<li>
+OWASP:
+<a href="https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html">
+Unvalidated Redirects and Forwards Cheat Sheet</a>.
+</li>
+<li>
+Rails Guides: <a href="https://guides.rubyonrails.org/security.html#redirection-and-files">Redirection and Files</a>.
+</li>
+
+</references>
 </qhelp>

ruby/ql/src/queries/security/cwe-601/examples/redirect_good.rb

@@ -1,11 +0,0 @@
-class HelloController < ActionController::Base
-  VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.html"
-
-  def hello
-    if params[:url] == VALID_REDIRECT
-      redirect_to params[:url]
-    else
-      # error
-    end
-  end
-end

ruby/ql/src/queries/security/cwe-601/examples/url_redirect_domain.rb

@@ -0,0 +1,21 @@
+require 'uri'
+
+class HelloController < ActionController::Base
+  KNOWN_HOST = "example.org"
+
+  def hello
+    begin
+      target_url = URI.parse(params[:url])
+
+      # Redirect if the URL is either relative or on a known good host
+      if !target_url.host || target_url.host == KNOWN_HOST
+        redirect_to target_url.to_s
+      else
+        redirect_to "/error.html" # Redirect to error page if the host is not known
+      end
+    rescue URI::InvalidURIError
+      # Handle the exception, for example, by redirecting to a safe page
+      redirect_to "/error.html"
+    end
+  end
+end

ruby/ql/src/queries/security/cwe-601/examples/url_redirect_good.rb

@@ -0,0 +1,16 @@
+class HelloController < ActionController::Base
+  VALID_REDIRECTS = [
+    "http://cwe.mitre.org/data/definitions/601.html",
+    "http://cwe.mitre.org/data/definitions/79.html"
+  ].freeze
+
+  def hello
+    # GOOD: the request parameter is validated against a known list of URLs
+    target_url = params[:url]
+    if VALID_REDIRECTS.include?(target_url)
+      redirect_to target_url
+    else
+      redirect_to "/error.html"
+    end
+  end
+end

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

@@ -98,3 +98,40 @@ end
 Rails.routes.draw do
   get "/r9", to: "users#route9"
 end
+
+class DomainController < ActionController::Base
+  KNOWN_HOST = "example.org"
+  
+  def hello
+    begin
+      target_url = URI.parse(params[:url])
+ 
+      # Redirect if the URL is either relative or on a known good host
+      if !target_url.host || target_url.host == KNOWN_HOST
+        redirect_to target_url.to_s
+      else
+        redirect_to "/error.html" # Redirect to error page if the host is not known
+      end
+    rescue URI::InvalidURIError
+      # Handle the exception, for example, by redirecting to a safe page
+      redirect_to "/error.html"
+    end
+  end
+end
+
+class ConstController < ActionController::Base
+  VALID_REDIRECTS = [
+    "http://cwe.mitre.org/data/definitions/601.html",
+    "http://cwe.mitre.org/data/definitions/79.html"
+  ].freeze
+  
+  def hello
+    # GOOD: the request parameter is validated against a known list of URLs
+    target_url = params[:url]
+    if VALID_REDIRECTS.include?(target_url)
+      redirect_to target_url
+    else
+      redirect_to "/error.html"
+    end
+  end
+end