SECURITY: correct our CSRF implementation to be much more aggressive

This commit is contained in:
Sam 2013-07-29 15:13:13 +10:00
parent 4a20d09523
commit aa6c92922d
8 changed files with 58 additions and 7 deletions

View file

@ -132,8 +132,11 @@ Discourse = Ember.Application.createWithMixins(Discourse.Ajax, {
// Add a CSRF token to all AJAX requests // Add a CSRF token to all AJAX requests
var csrfToken = $('meta[name=csrf-token]').attr('content'); var csrfToken = $('meta[name=csrf-token]').attr('content');
$.ajaxPrefilter(function(options, originalOptions, xhr) { $.ajaxPrefilter(function(options, originalOptions, xhr) {
if (!options.crossDomain) { if (!options.crossDomain) {
// This may be delay set
csrfToken = csrfToken || $('meta[name=csrf-token]').attr('content');
xhr.setRequestHeader('X-CSRF-Token', csrfToken); xhr.setRequestHeader('X-CSRF-Token', csrfToken);
} }
}); });

View file

@ -46,7 +46,7 @@ Discourse.Ajax = Em.Mixin.create({
return Ember.RSVP.resolve(fixture); return Ember.RSVP.resolve(fixture);
} }
return Ember.Deferred.promise(function (promise) { var performAjax = function(promise) {
var oldSuccess = args.success; var oldSuccess = args.success;
args.success = function(xhr) { args.success = function(xhr) {
Ember.run(promise, promise.resolve, xhr); Ember.run(promise, promise.resolve, xhr);
@ -69,7 +69,22 @@ Discourse.Ajax = Em.Mixin.create({
if ((!args.dataType) && (args.type === 'GET')) args.dataType = 'json'; if ((!args.dataType) && (args.type === 'GET')) args.dataType = 'json';
$.ajax(Discourse.getURL(url), args); $.ajax(Discourse.getURL(url), args);
}); };
// For cached pages we strip out CSRF tokens, need to round trip to server prior to sending the
// request (bypass for GET, not needed)
var csrfToken = $('meta[name=csrf-token]').attr('content');
if(args.type && args.type !== 'GET' && !csrfToken){
return Ember.Deferred.promise(function(promise){
$.ajax(Discourse.getURL('/session/csrf'))
.success(function(result){
$('head').append('<meta name="csrf-token" content="' + result.csrf + '">');
performAjax(promise);
});
});
} else {
return Ember.Deferred.promise(performAjax);
}
} }
}); });

View file

@ -14,6 +14,18 @@ class ApplicationController < ActionController::Base
protect_from_forgery protect_from_forgery
# Default Rails 3.2 lets the request through with a blank session
# we are being more pedantic here and nulling session / current_user
# and then raising a CSRF exception
def handle_unverified_request
# NOTE: API key is secret, having it invalidates the need for a CSRF token
unless is_api?
super
clear_current_user
raise Discourse::CSRF
end
end
before_filter :inject_preview_style before_filter :inject_preview_style
before_filter :block_if_maintenance_mode before_filter :block_if_maintenance_mode
before_filter :authorize_mini_profiler before_filter :authorize_mini_profiler

View file

@ -1,10 +1,11 @@
class SessionController < ApplicationController class SessionController < ApplicationController
# we need to allow account login with bad CSRF tokens, if people are caching, the CSRF token on the
# page is going to be empty, this means that server will see an invalid CSRF and blow the session
# once that happens you can't log in with social
skip_before_filter :verify_authenticity_token, only: [:create]
skip_before_filter :redirect_to_login_if_required skip_before_filter :redirect_to_login_if_required
def csrf
render json: {csrf: form_authenticity_token }
end
def create def create
params.require(:login) params.require(:login)
params.require(:password) params.require(:password)

View file

@ -15,7 +15,8 @@ class Users::OmniauthCallbacksController < ApplicationController
# need to be able to call this # need to be able to call this
skip_before_filter :check_xhr skip_before_filter :check_xhr
# must be done, cause we may trigger a POST # this is the only spot where we allow CSRF, our openid / oauth redirect
# will not have a CSRF token, however the payload is all validated so its safe
skip_before_filter :verify_authenticity_token, only: :complete skip_before_filter :verify_authenticity_token, only: :complete
def complete def complete

View file

@ -96,6 +96,8 @@ Discourse::Application.routes.draw do
end end
end end
get 'session/csrf' => 'session#csrf'
resources :users, except: [:show, :update] do resources :users, except: [:show, :update] do
collection do collection do
get 'check_username' get 'check_username'

View file

@ -17,6 +17,12 @@ module CurrentUser
end end
end end
# can be used to pretend current user does no exist, for CSRF attacks
def clear_current_user
@current_user = nil
@not_logged_in = true
end
def log_on_user(user) def log_on_user(user)
session[:current_user_id] = user.id session[:current_user_id] = user.id
unless user.auth_token && user.auth_token.length == 32 unless user.auth_token && user.auth_token.length == 32
@ -30,6 +36,13 @@ module CurrentUser
cookies.permanent["_t"] = { value: user.auth_token, httponly: true } cookies.permanent["_t"] = { value: user.auth_token, httponly: true }
end end
def is_api?
# ensure current user has been called
# otherwise
current_user
@is_api
end
def current_user def current_user
return @current_user if @current_user || @not_logged_in return @current_user if @current_user || @not_logged_in
@ -64,6 +77,7 @@ module CurrentUser
if api_key = request["api_key"] if api_key = request["api_key"]
if api_username = request["api_username"] if api_username = request["api_username"]
if SiteSetting.api_key_valid?(api_key) if SiteSetting.api_key_valid?(api_key)
@is_api = true
@current_user = User.where(username_lower: api_username.downcase).first @current_user = User.where(username_lower: api_username.downcase).first
end end
end end

View file

@ -20,6 +20,9 @@ module Discourse
# When a setting is missing # When a setting is missing
class SiteSettingMissing < Exception; end class SiteSettingMissing < Exception; end
# Cross site request forgery
class CSRF < Exception; end
def self.cache def self.cache
@cache ||= Cache.new @cache ||= Cache.new
end end