From 6d3e49f128c2db4cb157e058effe07781b0a66e4 Mon Sep 17 00:00:00 2001
From: Zack Deveau <zack.ref@gmail.com>
Date: Thu, 11 May 2023 16:55:01 -0400
Subject: [PATCH] Added check for illegal HTTP header value in redirect_to

The set of legal characters for an HTTP header value is described
in https://datatracker.ietf.org/doc/html/rfc7230\#section-3.2.6.

This commit adds a check to redirect_to that ensures the
provided URL does not contain any of the illegal characters.

Downstream consumers of the resulting Location response header
may remove the header if it does not comply with the RFC.
This can result in a cross site scripting (XSS) vector by
allowing for the redirection page to sit idle waiting
for user interaction with the provided malicious link.

[CVE-2023-28362]

format
---
 .../action_controller/metal/redirecting.rb    | 19 ++++++++++++++++++-
 actionpack/test/controller/redirect_test.rb   | 17 +++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/actionpack/lib/action_controller/metal/redirecting.rb b/actionpack/lib/action_controller/metal/redirecting.rb
index 0409ba7026..830b94c092 100644
--- a/actionpack/lib/action_controller/metal/redirecting.rb
+++ b/actionpack/lib/action_controller/metal/redirecting.rb
@@ -4,6 +4,8 @@ module ActionController
   module Redirecting
     extend ActiveSupport::Concern
 
+    ILLEGAL_HEADER_VALUE_REGEX = /[\x00-\x08\x0A-\x1F]/.freeze
+
     include AbstractController::Logger
     include ActionController::UrlFor
 
@@ -86,7 +88,11 @@ def redirect_to(options = {}, response_options = {})
       allow_other_host = response_options.delete(:allow_other_host) { _allow_other_host }
 
       self.status        = _extract_redirect_to_status(options, response_options)
-      self.location      = _enforce_open_redirect_protection(_compute_redirect_to_location(request, options), allow_other_host: allow_other_host)
+
+      redirect_to_location = _compute_redirect_to_location(request, options)
+      _ensure_url_is_http_header_safe(redirect_to_location)
+
+      self.location      = _enforce_open_redirect_protection(redirect_to_location, allow_other_host: allow_other_host)
       self.response_body = "<html><body>You are being <a href=\"#{ERB::Util.unwrapped_html_escape(response.location)}\">redirected</a>.</body></html>"
     end
 
@@ -204,5 +210,16 @@ def _url_host_allowed?(url)
       rescue ArgumentError, URI::Error
         false
       end
+
+      def _ensure_url_is_http_header_safe(url)
+        # Attempt to comply with the set of valid token characters
+        # defined for an HTTP header value in
+        # https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6
+        if url.match(ILLEGAL_HEADER_VALUE_REGEX)
+          msg = "The redirect URL #{url} contains one or more illegal HTTP header field character. " \
+            "Set of legal characters defined in https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6"
+          raise UnsafeRedirectError, msg
+        end
+      end
   end
 end
diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb
index 91a8f8512b..40bd8d68da 100644
--- a/actionpack/test/controller/redirect_test.rb
+++ b/actionpack/test/controller/redirect_test.rb
@@ -104,6 +104,10 @@ def unsafe_redirect_protocol_relative_triple_slash
     redirect_to "///www.rubyonrails.org/"
   end
 
+  def unsafe_redirect_with_illegal_http_header_value_character
+    redirect_to "javascript:alert(document.domain)\b", allow_other_host: true
+  end
+
   def only_path_redirect
     redirect_to action: "other_host", only_path: true
   end
@@ -556,6 +560,19 @@ def test_unsafe_redirect_with_protocol_relative_triple_slash_url
     end
   end
 
+  def test_unsafe_redirect_with_illegal_http_header_value_character
+    with_raise_on_open_redirects do
+      error = assert_raise(ActionController::Redirecting::UnsafeRedirectError) do
+        get :unsafe_redirect_with_illegal_http_header_value_character
+      end
+
+      msg = "The redirect URL javascript:alert(document.domain)\b contains one or more illegal HTTP header field character. " \
+        "Set of legal characters defined in https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6"
+
+      assert_equal msg, error.message
+    end
+  end
+
   def test_only_path_redirect
     with_raise_on_open_redirects do
       get :only_path_redirect
-- 
2.39.2

