Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Do not allow reading from the socket if the HTTP spec indicates that …

…there is not supposed to be a request body, unless the app hijacks the socket.

This fixes a potential Denial of Service caused by WebSocket support.
  • Loading branch information...
commit 1abbaff3daf27f0ce9422a74bb404bac7cd73d53 1 parent dd1feb6
@FooBarWidget FooBarWidget authored
View
7 lib/phusion_passenger/rack/thread_handler_extension.rb
@@ -66,7 +66,12 @@ def process_request(env, connection, socket_wrapper, full_http_response)
env[RACK_URL_SCHEME] = HTTP
end
env[RACK_HIJACK_P] = true
- env[RACK_HIJACK] = lambda { env[RACK_HIJACK_IO] ||= connection }
+ env[RACK_HIJACK] = lambda do
+ env[RACK_HIJACK_IO] ||= begin
+ connection.stop_simulating_eof!
+ connection
+ end
+ end
begin
status, headers, body = @app.call(env)
View
13 lib/phusion_passenger/request_handler/thread_handler.rb
@@ -42,6 +42,8 @@ class ThreadHandler
PING = 'PING'.freeze
OOBW = 'OOBW'.freeze
PASSENGER_CONNECT_PASSWORD = 'PASSENGER_CONNECT_PASSWORD'.freeze
+ CONTENT_LENGTH = 'CONTENT_LENGTH'.freeze
+ TRANSFER_ENCODING = 'TRANSFER_ENCODING'.freeze
MAX_HEADER_SIZE = 128 * 1024
@@ -123,7 +125,7 @@ def accept_and_process_next_request(socket_wrapper, channel, buffer)
trace(3, "Accepted new request on socket #{@socket_name}")
channel.io = connection
if headers = parse_request(connection, channel, buffer)
- prepare_request(headers)
+ prepare_request(connection, headers)
begin
if headers[REQUEST_METHOD] == PING
process_ping(headers, connection)
@@ -237,7 +239,7 @@ def parse_http_request(connection, channel, buffer)
header, value = line.split(/\s*:\s*/, 2)
header.upcase! # "Foo-Bar" => "FOO-BAR"
header.gsub!("-", "_") # => "FOO_BAR"
- if header == "CONTENT_LENGTH" || header == "CONTENT_TYPE"
+ if header == CONTENT_LENGTH || header == "CONTENT_TYPE"
headers[header] = value
else
headers["HTTP_#{header}"] = value
@@ -269,7 +271,12 @@ def process_oobw(env, connection)
# raise NotImplementedError, "Override with your own implementation!"
# end
- def prepare_request(headers)
+ def prepare_request(connection, headers)
+ if (!headers.has_key?(CONTENT_LENGTH) && !headers.has_key?(TRANSFER_ENCODING)) ||
+ headers[CONTENT_LENGTH] == 0
+ connection.simulate_eof!
+ end
+
if @analytics_logger && headers[PASSENGER_TXN_ID]
txn_id = headers[PASSENGER_TXN_ID]
union_station_key = headers[PASSENGER_UNION_STATION_KEY]
View
50 lib/phusion_passenger/utils/unseekable_socket.rb
@@ -26,14 +26,14 @@
module PhusionPassenger
module Utils
-# Some frameworks (e.g. Merb) call _seek_ and _rewind_ on the input stream
+# Some frameworks (e.g. Merb) call `seek` and `rewind` on the input stream
# if it responds to these methods. In case of Phusion Passenger, the input
-# stream is a socket, and altough socket objects respond to _seek_ and
-# _rewind_, calling these methods will raise an exception. We don't want
+# stream is a socket, and altough socket objects respond to `seek` and
+# `rewind`, calling these methods will raise an exception. We don't want
# this to happen so in AbstractRequestHandler we wrap the client socket
# into an UnseekableSocket wrapper, which doesn't respond to these methods.
#
-# We used to dynamically undef _seek_ and _rewind_ on sockets, but this
+# We used to dynamically undef `seek` and `rewind` on sockets, but this
# blows the Ruby interpreter's method cache and made things slower.
# Wrapping a socket is faster despite extra method calls.
#
@@ -87,11 +87,19 @@ def flush
def binmode
end
+ # This makes select() work.
def to_io
- # This makes select() work.
@socket
end
+ def simulate_eof!
+ @simulate_eof = true
+ end
+
+ def stop_simulating_eof!
+ @simulate_eof = false
+ end
+
def fileno
@socket.fileno
end
@@ -133,36 +141,54 @@ def puts(*args)
end
def gets
+ return nil if @simulate_eof
@socket.gets
rescue => e
raise annotate(e)
end
def read(*args)
+ if @simulate_eof
+ length, buffer = args
+ if buffer
+ buffer.replace(binary_string(""))
+ else
+ buffer = binary_string("")
+ end
+ if length
+ return nil
+ else
+ return buffer
+ end
+ end
@socket.read(*args)
rescue => e
raise annotate(e)
end
def readpartial(*args)
+ raise EOFError, "end of file reached" if @simulate_eof
@socket.readpartial(*args)
rescue => e
raise annotate(e)
end
def readline
+ raise EOFError, "end of file reached" if @simulate_eof
@socket.readline
rescue => e
raise annotate(e)
end
def each(&block)
+ return if @simulate_eof
@socket.each(&block)
rescue => e
raise annotate(e)
end
def eof?
+ return true if @simulate_eof
@socket.eof?
rescue => e
raise annotate(e)
@@ -201,6 +227,20 @@ def annotate(exception)
exception.instance_variable_set(:"@from_unseekable_socket", @socket.object_id)
return exception
end
+
+ def raise_error_because_activity_disallowed!
+ raise IOError, "It is not possible to read or write from the client socket because the current."
+ end
+
+ if ''.respond_to?(:force_encoding)
+ def binary_string(str)
+ return ''.force_encoding('binary')
+ end
+ else
+ def binary_string(str)
+ return ''
+ end
+ end
end
end # module Utils
View
146 test/ruby/request_handler_spec.rb
@@ -327,7 +327,151 @@ def body.each
lambda_called.should == true
hijack_callback_called.should == true
end
-
+
+ describe "on GET requests that may have a body" do
+ before :each do
+ @options["thread_handler"] = Class.new(RequestHandler::ThreadHandler) do
+ include Rack::ThreadHandlerExtension
+ end
+ end
+
+ it "allows reading from the client socket" do
+ lambda_called = false
+
+ @options["app"] = lambda do |env|
+ lambda_called = true
+ env['rack.input'].read(3).should == "abc"
+ [200, {}, ["ok"]]
+ end
+
+ @request_handler = RequestHandler.new(@owner_pipe[1], @options)
+ @request_handler.start_main_loop_thread
+ client = connect
+ begin
+ send_binary_request(client,
+ "REQUEST_METHOD" => "GET",
+ "PATH_INFO" => "/",
+ "CONTENT_LENGTH" => "3")
+ client.write("abc")
+ client.close_write
+ client.read.should ==
+ "Status: 200\r\n" +
+ "\r\n" +
+ "ok"
+ ensure
+ client.close
+ end
+
+ lambda_called.should be_true
+ end
+ end
+
+ describe "on GET requests that are not supposed to have a body" do
+ before :each do
+ @options["thread_handler"] = Class.new(RequestHandler::ThreadHandler) do
+ include Rack::ThreadHandlerExtension
+ end
+ end
+
+ it "disallows reading from the client socket" do
+ lambda_called = false
+
+ @options["app"] = lambda do |env|
+ lambda_called = true
+ env['rack.input'].read(1).should be_nil
+ env['rack.input'].gets.should be_nil
+ [200, {}, ["ok"]]
+ end
+
+ @request_handler = RequestHandler.new(@owner_pipe[1], @options)
+ @request_handler.start_main_loop_thread
+ client = connect
+ begin
+ send_binary_request(client,
+ "REQUEST_METHOD" => "GET",
+ "PATH_INFO" => "/")
+ client.close_write
+ client.read.should ==
+ "Status: 200\r\n" +
+ "\r\n" +
+ "ok"
+ ensure
+ client.close
+ end
+
+ lambda_called.should be_true
+ end
+
+ it "allows reading from the client socket once the socket has been fully hijacked" do
+ lambda_called = false
+
+ @options["app"] = lambda do |env|
+ lambda_called = true
+ env['rack.hijack'].call
+ io = env['rack.hijack_io']
+ begin
+ io.read.should == "hi"
+ io.write("ok")
+ ensure
+ io.close
+ end
+ end
+
+ @request_handler = RequestHandler.new(@owner_pipe[1], @options)
+ @request_handler.start_main_loop_thread
+ client = connect
+ begin
+ send_binary_request(client,
+ "REQUEST_METHOD" => "GET",
+ "PATH_INFO" => "/")
+ client.write("hi")
+ client.close_write
+ client.read.should == "ok"
+ ensure
+ client.close
+ end
+
+ lambda_called.should be_true
+ end
+
+ it "allows reading from the client socket once the socket has been partially hijacked" do
+ lambda_called = false
+
+ @options["app"] = lambda do |env|
+ block = lambda do |io|
+ lambda_called = true
+ begin
+ io.read.should == "hi"
+ io.write("ok")
+ ensure
+ io.close
+ end
+ end
+ headers = { 'rack.hijack' => block }
+ [200, headers, []]
+ end
+
+ @request_handler = RequestHandler.new(@owner_pipe[1], @options)
+ @request_handler.start_main_loop_thread
+ client = connect
+ begin
+ send_binary_request(client,
+ "REQUEST_METHOD" => "GET",
+ "PATH_INFO" => "/")
+ client.write("hi")
+ client.close_write
+ client.read.should ==
+ "Status: 200\r\n" +
+ "\r\n" +
+ "ok"
+ ensure
+ client.close
+ end
+
+ lambda_called.should be_true
+ end
+ end
+
describe "if analytics logger is given" do
def preinitialize
if @agent_pid
Please sign in to comment.
Something went wrong with that request. Please try again.