Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Strip null bytes from Location header #5456

Merged
merged 1 commit into from

3 participants

@brianmario

This strips null bytes off of the Location header in addition to \r\n which are already being stripped.

This is in response to a recent nasty security vulnerability in nginx. I figure it's probably easier for people to get this patch in their Rails applications than it is for them to get nginx upgraded in their infrastructure - which may or may not even be managed by them.

I'd like to get this into all supported Rails versions, not sure if this patch will apply cleanly to 3.1 or 3.0 - let me know if not and I'll send some others.

@brianmario brianmario strip null bytes from Location header as well
add tests for stripping \r\n chars since that's already happening
cfcdd33
@tenderlove tenderlove merged commit a7dee1a into rails:master
@carlosantoniodasilva carlosantoniodasilva commented on the diff
actionpack/test/controller/redirect_test.rb
@@ -120,6 +128,18 @@ def test_simple_redirect
assert_equal "http://test.host/redirect/hello_world", redirect_to_url
end
+ def test_redirect_with_header_break
+ get :redirect_with_header_break
+ assert_response :redirect
+ assert_equal "http://test.host/lolwat", redirect_to_url
+ end
+
+ def test_redirect_with_null_bytes
+ get :redirect_with_header_break

Hey, I think this should read redirect_with_null_bytes instead, not?

gah, yep :(

@tenderlove Owner

another PR plz? :heart:

on it

@tenderlove Owner

:heart::heart::heart::heart::heart::heart::heart::heart:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 15, 2012
  1. @brianmario

    strip null bytes from Location header as well

    brianmario authored
    add tests for stripping \r\n chars since that's already happening
This page is out of date. Refresh to see the latest.
View
2  actionpack/lib/action_controller/metal/redirecting.rb
@@ -93,7 +93,7 @@ def _compute_redirect_to_location(options)
_compute_redirect_to_location options.call
else
url_for(options)
- end.gsub(/[\r\n]/, '')
+ end.gsub(/[\0\r\n]/, '')
end
end
end
View
2  actionpack/lib/action_dispatch/testing/assertions/response.rb
@@ -83,7 +83,7 @@ def normalize_argument_to_redirection(fragment)
refer
else
@controller.url_for(fragment)
- end.gsub(/[\r\n]/, '')
+ end.gsub(/[\0\r\n]/, '')
end
end
end
View
20 actionpack/test/controller/redirect_test.rb
@@ -103,6 +103,14 @@ def redirect_to_with_block_and_options
redirect_to proc { {:action => "hello_world"} }
end
+ def redirect_with_header_break
+ redirect_to "/lol\r\nwat"
+ end
+
+ def redirect_with_null_bytes
+ redirect_to "\000/lol\r\nwat"
+ end
+
def rescue_errors(e) raise e end
protected
@@ -120,6 +128,18 @@ def test_simple_redirect
assert_equal "http://test.host/redirect/hello_world", redirect_to_url
end
+ def test_redirect_with_header_break
+ get :redirect_with_header_break
+ assert_response :redirect
+ assert_equal "http://test.host/lolwat", redirect_to_url
+ end
+
+ def test_redirect_with_null_bytes
+ get :redirect_with_header_break

Hey, I think this should read redirect_with_null_bytes instead, not?

gah, yep :(

@tenderlove Owner

another PR plz? :heart:

on it

@tenderlove Owner

:heart::heart::heart::heart::heart::heart::heart::heart:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ assert_response :redirect
+ assert_equal "http://test.host/lolwat", redirect_to_url
+ end
+
def test_redirect_with_no_status
get :simple_redirect
assert_response 302
Something went wrong with that request. Please try again.