From 3c9b3705b076916c46f795c946c33ea975d2685a Mon Sep 17 00:00:00 2001
From: Zack Deveau <zack.ref@gmail.com>
Date: Fri, 12 May 2023 13:04:22 -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]
---
 .../action_controller/metal/redirecting.rb    | 21 ++++++++++++++++++-
 actionpack/test/controller/redirect_test.rb   | 17 +++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/actionpack/lib/action_controller/metal/redirecting.rb b/actionpack/lib/action_controller/metal/redirecting.rb
index 11d462855d..fdd3f9dc44 100644
--- a/actionpack/lib/action_controller/metal/redirecting.rb
+++ b/actionpack/lib/action_controller/metal/redirecting.rb
@@ -7,6 +7,10 @@ module Redirecting
     include AbstractController::Logger
     include ActionController::UrlFor
 
+    ILLEGAL_HEADER_VALUE_REGEX = /[\x00-\x08\x0A-\x1F]/.freeze
+
+    class UnsafeRedirectError < StandardError; end
+
     # Redirects the browser to the target specified in +options+. This parameter can be any one of:
     #
     # * <tt>Hash</tt> - The URL will be generated by calling url_for with the +options+.
@@ -60,7 +64,11 @@ def redirect_to(options = {}, response_options = {})
       raise AbstractController::DoubleRenderError if response_body
 
       self.status        = _extract_redirect_to_status(options, response_options)
-      self.location      = _compute_redirect_to_location(request, options)
+
+      redirect_to_location = _compute_redirect_to_location(request, options)
+      _ensure_url_is_http_header_safe(redirect_to_location)
+
+      self.location      = redirect_to_location
       self.response_body = "<html><body>You are being <a href=\"#{ERB::Util.unwrapped_html_escape(response.location)}\">redirected</a>.</body></html>"
     end
 
@@ -129,5 +137,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 e218ef35e4..c088d96413 100644
--- a/actionpack/test/controller/redirect_test.rb
+++ b/actionpack/test/controller/redirect_test.rb
@@ -153,6 +153,11 @@ def redirect_with_null_bytes
     redirect_to "\000/lol\r\nwat"
   end
 
+  def unsafe_redirect_with_illegal_http_header_value_character
+    redirect_to "javascript:alert(document.domain)\b"
+  end
+
+
   def rescue_errors(e) raise e end
 
   private
@@ -437,6 +442,18 @@ def test_redirect_to_with_block_and_accepted_options
       assert_redirected_to "http://test.host/redirect/hello_world"
     end
   end
+
+  def test_unsafe_redirect_with_illegal_http_header_value_character
+    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
 
 module ModuleTest
-- 
2.39.2

