Skip to content

Commit

Permalink
ActionDispatch::Executor don't fully trust body#close
Browse files Browse the repository at this point in the history
Under certain circumstances, the middleware isn't informed that the
response body has been fully closed which result in request state not
being fully reset before the next request.

[CVE-2022-23633]
  • Loading branch information
byroot authored and tenderlove committed Feb 11, 2022
1 parent 76489d8 commit 3c0420c
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 24 deletions.
2 changes: 1 addition & 1 deletion actionpack/lib/action_dispatch/middleware/executor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def initialize(app, executor)
end

def call(env)
state = @executor.run!
state = @executor.run!(reset: true)
begin
response = @app.call(env)
returned = response << ::Rack::BodyProxy.new(response.pop) { state.complete! }
Expand Down
21 changes: 21 additions & 0 deletions actionpack/test/dispatch/executor_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,27 @@ def test_callbacks_execute_in_shared_context
assert_not defined?(@in_shared_context) # it's not in the test itself
end

def test_body_abandonned
total = 0
ran = 0
completed = 0

executor.to_run { total += 1; ran += 1 }
executor.to_complete { total += 1; completed += 1}

stack = middleware(proc { [200, {}, "response"] })

requests_count = 5

requests_count.times do
stack.call({})
end

assert_equal (requests_count * 2) - 1, total
assert_equal requests_count, ran
assert_equal requests_count - 1, completed
end

private
def call_and_return_body(&block)
app = middleware(block || proc { [200, {}, "response"] })
Expand Down
42 changes: 19 additions & 23 deletions activesupport/lib/active_support/execution_wrapper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,18 +64,21 @@ def self.register_hook(hook, outer: false)
# after the work has been performed.
#
# Where possible, prefer +wrap+.
def self.run!
if active?
Null
def self.run!(reset: false)
if reset
lost_instance = IsolatedExecutionState.delete(active_key)
lost_instance&.complete!
else
new.tap do |instance|
success = nil
begin
instance.run!
success = true
ensure
instance.complete! unless success
end
return Null if active?
end

new.tap do |instance|
success = nil
begin
instance.run!
success = true
ensure
instance.complete! unless success
end
end
end
Expand Down Expand Up @@ -105,27 +108,20 @@ def self.perform # :nodoc:
end
end

class << self # :nodoc:
attr_accessor :active
end

def self.error_reporter
@error_reporter ||= ActiveSupport::ErrorReporter.new
end

def self.inherited(other) # :nodoc:
super
other.active = Concurrent::Hash.new
def self.active_key # :nodoc:
@active_key ||= :"active_execution_wrapper_#{object_id}"
end

self.active = Concurrent::Hash.new

def self.active? # :nodoc:
@active[IsolatedExecutionState.unique_id]
IsolatedExecutionState.key?(active_key)
end

def run! # :nodoc:
self.class.active[IsolatedExecutionState.unique_id] = true
IsolatedExecutionState[self.class.active_key] = self
run
end

Expand All @@ -140,7 +136,7 @@ def run # :nodoc:
def complete!
complete
ensure
self.class.active.delete(IsolatedExecutionState.unique_id)
IsolatedExecutionState.delete(self.class.active_key)
end

def complete # :nodoc:
Expand Down
8 changes: 8 additions & 0 deletions activesupport/lib/active_support/isolated_execution_state.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ def []=(key, value)
current[key] = value
end

def key?(key)
state.key?(key)
end

def delete(key)
state.delete(key)
end

def clear
current.clear
end
Expand Down

4 comments on commit 3c0420c

@kobaltz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked into it too much yet, but trying to upgrade to 7.0.2.1 and my CI/CD platform gets angry with this.

NameError: undefined local variable or method `state' for ActiveSupport::IsolatedExecutionState:Module

@tylerdiaz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the same issue as @kobaltz on our production setup

@john-diaz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked into it too much yet, but trying to upgrade to 7.0.2.1 and my CI/CD platform gets angry with this.

NameError: undefined local variable or method `state' for ActiveSupport::IsolatedExecutionState:Module

Bump, running into @kobaltz's issue in prod.

@kobaltz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does appear that 7.0.2.2 addresses this

Please sign in to comment.