production caching may leak data between requests #1

Closed
coldnebo opened this Issue Jun 14, 2012 · 2 comments

Projects

None yet

1 participant

@coldnebo
Owner

When used in production contexts, controller classes live longer than a single request. (i.e. config.cache_classes = true)

If predicates are defined and called during the scope of a single request object, then subsequent requests will simply redefine the predicate closure on the controller class and all is good.

However, if, for some reason, a predicate is defined during one request and then inadvertently used by another request without being redefined, then the predicate closure can leak data across requests. If the data is sensitive (i.e. user information) this can be a potentially serious security issue that must be guarded against.

Thanks to Randy Souza for reporting this issue to me!

@coldnebo coldnebo was assigned Jun 14, 2012
@coldnebo
Owner

I toyed with the idea of using a dynamically added after_filter to remove the method definition, but that seems very complex -- also, it may not cover the case where a before_filter does a redirect, which can break the filter chain. So for this reason, I decided to store the request object id as part of the closure to ensure that it can only be called from the defining request context. This avoids having to mess with/rely on filter chains and gives the user more meaningful error information (namely what and where they should fix their predicate definitions).

@coldnebo
Owner

Ok, I also tested this manually with the latest rails doing the following:

# starting with a fresh rvm gemset...
rails new conditest
cd conditest
rails generate controller Main index hack

# then in app/controllers/main_controller.rb
class MainController < ApplicationController
  include Condi

  def index
    my_terrible_secret = true
    predicate(:show_secret?) { my_terrible_secret }
  end

  def hack
    my_terrible_secret = false
    render :action => 'index'
  end
end

# in app/views/main/index.html.erb
<h1>Main#index</h1>
<p>Find me in app/views/main/index.html.erb</p>
<% if show_secret? %>
<p>THIS IS THE SECRET BLOCK</p>
<% end %>

# and in config/environments/development.rb
config.cache_classes = true

Now, start the server and hit /main/index followed by /main/hack. Any order of operations should provide expected results. You shouldn't see the secret data incorrectly.

Here's how it looks, first we try the hack method and get an error since the predicate wasn't defined in that path:

Started GET "/main/hack" for 127.0.0.1 at 2012-06-14 01:22:47 -0400
Connecting to database specified by database.yml
Processing by MainController#hack as HTML
  Rendered main/index.html.erb within layouts/application (11.0ms)
Completed 500 Internal Server Error in 36ms

ActionView::Template::Error (undefined method `show_secret?' for #<#<Class:0x00000002ed6a00>:0x00000002068cc0>):
    1: <h1>Main#index</h1>
    2: <p>Find me in app/views/main/index.html.erb</p>
    3: <% if show_secret? %>
    4: <p>THIS IS THE SECRET BLOCK</p>
    5: <% end %>
  app/views/main/index.html.erb:3:in `_app_views_main_index_html_erb__2737243105131892858_14154300'
  app/controllers/main_controller.rb:11:in `hack'

  Rendered /local/rvm/gems/ruby-1.9.3-p194@conditest/gems/actionpack-3.2.6/lib/action_dispatch/middleware/templates/rescues/_trace.erb (4.1ms)
  Rendered /local/rvm/gems/ruby-1.9.3-p194@conditest/gems/actionpack-3.2.6/lib/action_dispatch/middleware/templates/rescues/_request_and_response.erb (0.8ms)
  Rendered /local/rvm/gems/ruby-1.9.3-p194@conditest/gems/actionpack-3.2.6/lib/action_dispatch/middleware/templates/rescues/template_error.erb within rescues/layout (9.6ms)
[2012-06-14 01:22:48] WARN  Could not determine content-length of response body. Set content-length of the response or set Response#chunked = true

Next, we get the index, which defines the predicate and then uses it in the index.erb view. All works well and we see the SECRET!

Started GET "/main/index" for 127.0.0.1 at 2012-06-14 01:22:52 -0400
Processing by MainController#index as HTML
  Rendered main/index.html.erb within layouts/application (0.0ms)
Compiled mainer.css  (21ms)  (pid 5561)
Compiled application.css  (40ms)  (pid 5561)
Compiled mainer.js  (561ms)  (pid 5561)
Compiled application.js  (591ms)  (pid 5561)
Completed 200 OK in 765ms (Views: 764.6ms | ActiveRecord: 0.0ms)

Now, we try to go back to the hack. Since the classes are cached, the previous 'index' request has already created a predicate... but this request scope is different! It fails the comparison check and raises a warning message, preventing us from accessing the previous request closure data.

Started GET "/main/hack" for 127.0.0.1 at 2012-06-14 01:22:57 -0400
Processing by MainController#hack as HTML
  Rendered main/index.html.erb within layouts/application (1.1ms)
Completed 500 Internal Server Error in 3ms

ActionView::Template::Error (predicate 'show_secret?' cannot be called outside of the request it was defined in (26849920). please redefine the predicate in this request (26850900).):
    1: <h1>Main#index</h1>
    2: <p>Find me in app/views/main/index.html.erb</p>
    3: <% if show_secret? %>
    4: <p>THIS IS THE SECRET BLOCK</p>
    5: <% end %>
  app/views/main/index.html.erb:3:in `_app_views_main_index_html_erb__2737243105131892858_14154300'
  app/controllers/main_controller.rb:11:in `hack'

  Rendered /local/rvm/gems/ruby-1.9.3-p194@conditest/gems/actionpack-3.2.6/lib/action_dispatch/middleware/templates/rescues/_trace.erb (4.7ms)
  Rendered /local/rvm/gems/ruby-1.9.3-p194@conditest/gems/actionpack-3.2.6/lib/action_dispatch/middleware/templates/rescues/_request_and_response.erb (2.7ms)
  Rendered /local/rvm/gems/ruby-1.9.3-p194@conditest/gems/actionpack-3.2.6/lib/action_dispatch/middleware/templates/rescues/template_error.erb within rescues/layout (12.6ms)
@coldnebo coldnebo added a commit that closed this issue Jun 14, 2012
@coldnebo fixes #1 e520edf
@coldnebo coldnebo closed this in e520edf Jun 14, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment