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]
rails · Jun 26, 2023 · 1c3f93d · 1c3f93d
1 parent f09dc7c
commit 1c3f93d
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 1 deletion.
diff --git 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
@@ -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