Skip to content

Commit c500bf8

Browse files
committed
Security: Force all GET requests to be read-only
The W3C makes a clear distinction between GET and POST requests. GET requests should only cause "safe" actions, and the user should never be held accountable for making GET requests. See the following for an overview: http://www.w3.org/2001/tag/doc/whenToUseGet.html The Rails 'protect_against_forgery' function (and possibly some web browsers) rely on the distinction between GET and POST to provide protection against CSRF attacks. See: http://en.wikipedia.org/wiki/Cross-site_request_forgery http://guides.rubyonrails.org/security.html#_csrf_countermeasures Unfortunately, enforcing these rules in rather difficult, especially in a large application with lots of controllers and plugins. So this patch applies a rather heavy-handed fix: We globally block database writes during GET requests, and specifically override that policy in one or two places. All of our current overrides invoke User#reset_token!. I haven't performed a full security analysis of allowing User#reset_token! (or updates to session[:user] based on our "remember me" token) in a GET request. For now, I'm going to go ahead and allow this activity--if we actually have some sort of vulnerability here, it affects a wide range of web applications. Note that this patch may break some part of the /admin interface. I've tried posting articles and other basic stuff, but I haven't used the lesser-known corners of /admin since making these changes. Please report any problems.
1 parent ff0113a commit c500bf8

File tree

6 files changed

+102
-2
lines changed

6 files changed

+102
-2
lines changed

app/controllers/account_controller.rb

+4-1
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,10 @@ def forget
4343
def activate
4444
self.current_user = site.user_by_token(params[:id])
4545
if logged_in?
46-
current_user.reset_token!
46+
# TODO - See security comments on AuthenticatedSystem#login_from_cookie.
47+
ActiveRecord::Base.with_writable_records do
48+
current_user.reset_token!
49+
end
4750
else
4851
flash[:error] = "Invalid token. Try resending your forgotten password request."
4952
end

app/controllers/admin/assets_controller.rb

+1
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ class Admin::AssetsController < Admin::BaseController
33
skip_before_filter :login_required
44
before_filter :find_asset, :except => [:index, :new, :create, :latest, :search, :upload, :clear_bucket]
55
before_filter :login_required
6+
before_filter :protect_action, :except => [:index, :new, :latest, :search]
67

78
def index
89
search_assets 24

app/controllers/application.rb

+16
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ class ApplicationController < ActionController::Base
33

44
include Mephisto::CachingMethods
55
before_filter :set_cache_root
6+
around_filter :get_requests_are_readonly
67
helper_method :site
78
attr_reader :site
89

@@ -95,6 +96,21 @@ def set_cache_root
9596
end
9697
end
9798

99+
# Much of a web browser's built-in protection against CSRF attacks
100+
# assumes that all GET requests are "safe". Since we don't want to
101+
# rely on getting this 100% right in every controller and plugin, let's
102+
# just enforce this policy globally. You can override this using
103+
# <code>skip_filter :get_requests_are_readonly</code>.
104+
def get_requests_are_readonly
105+
if request.method == :get
106+
ActiveRecord::Base.with_readonly_records do
107+
yield
108+
end
109+
else
110+
yield
111+
end
112+
end
113+
98114
def with_site_timezone
99115
old_tz = ENV['TZ']
100116
ENV['TZ'] = site.timezone.name
+35
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
module ActiveRecord
2+
class Base
3+
# Are ActiveRecord::Base objects currently readonly?
4+
def self.all_records_are_readonly?
5+
Thread.current[:all_records_are_readonly]
6+
end
7+
8+
def readonly_with_global_flag?
9+
self.class.all_records_are_readonly? || readonly_without_global_flag?
10+
end
11+
alias_method_chain :readonly?, :global_flag
12+
13+
# Make all ActiveRecord::Base objects readonly within a block.
14+
def self.with_readonly_records # :yield:
15+
saved = all_records_are_readonly?
16+
begin
17+
Thread.current[:all_records_are_readonly] = true
18+
yield
19+
ensure
20+
Thread.current[:all_records_are_readonly] = saved
21+
end
22+
end
23+
24+
# Make all ActiveRecord::Base objects writable within a block.
25+
def self.with_writable_records # :yield:
26+
saved = all_records_are_readonly?
27+
begin
28+
Thread.current[:all_records_are_readonly] = false
29+
yield
30+
ensure
31+
Thread.current[:all_records_are_readonly] = saved
32+
end
33+
end
34+
end
35+
end

lib/authenticated_system.rb

+11-1
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,17 @@ def self.included(base)
7474
def login_from_cookie
7575
return unless cookies[:token] && !logged_in?
7676
self.current_user = site.user_by_token(cookies[:token])
77-
cookies[:token] = { :value => self.current_user.reset_token! , :expires => self.current_user.token_expires_at } if logged_in?
77+
# TODO - We allow the token to be changed on GET requests and we log
78+
# the user in. I haven't fully analyzed the consequences of allowing
79+
# session and token updates on hostile GET requests triggered by CSRF
80+
# attacks. If this helps out in some kind of attack, it would affect
81+
# almost every single web application in existence.
82+
ActiveRecord::Base.with_writable_records do
83+
cookies[:token] = {
84+
:value => self.current_user.reset_token!,
85+
:expires => self.current_user.token_expires_at
86+
} if logged_in?
87+
end
7888
true
7989
end
8090

spec/models/readonly_spec.rb

+35
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
require File.dirname(__FILE__) + '/../spec_helper'
2+
3+
# Verify that our readonly_record patches are working.
4+
describe "Any record" do
5+
before :each do
6+
@article = Article.make(:title => "Original title")
7+
end
8+
9+
it "should be writable by default" do
10+
assert !ActiveRecord::Base.all_records_are_readonly?
11+
@article.title = "Hello!"
12+
@article.save!
13+
end
14+
15+
it "should not be writable inside with_readonly_records" do
16+
assert_raise ActiveRecord::ReadOnlyRecord do
17+
ActiveRecord::Base.with_readonly_records do
18+
assert ActiveRecord::Base.all_records_are_readonly?
19+
@article.title = "Hello!"
20+
@article.save!
21+
end
22+
end
23+
end
24+
25+
it "should be writable inside with_writable_records" do
26+
ActiveRecord::Base.with_readonly_records do
27+
ActiveRecord::Base.with_writable_records do
28+
assert !ActiveRecord::Base.all_records_are_readonly?
29+
@article.title = "Hello!"
30+
@article.save!
31+
end
32+
end
33+
end
34+
end
35+

0 commit comments

Comments
 (0)