From aa6c92922d25b334e5944426dccb3ca100b9a448 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 29 Jul 2013 15:13:13 +1000 Subject: [PATCH] SECURITY: correct our CSRF implementation to be much more aggressive --- app/assets/javascripts/discourse.js | 3 +++ .../javascripts/discourse/mixins/ajax.js | 19 +++++++++++++++++-- app/controllers/application_controller.rb | 12 ++++++++++++ app/controllers/session_controller.rb | 9 +++++---- .../users/omniauth_callbacks_controller.rb | 3 ++- config/routes.rb | 2 ++ lib/current_user.rb | 14 ++++++++++++++ lib/discourse.rb | 3 +++ 8 files changed, 58 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/discourse.js b/app/assets/javascripts/discourse.js index 603780a4a..3173d80c7 100644 --- a/app/assets/javascripts/discourse.js +++ b/app/assets/javascripts/discourse.js @@ -132,8 +132,11 @@ Discourse = Ember.Application.createWithMixins(Discourse.Ajax, { // Add a CSRF token to all AJAX requests var csrfToken = $('meta[name=csrf-token]').attr('content'); + $.ajaxPrefilter(function(options, originalOptions, xhr) { if (!options.crossDomain) { + // This may be delay set + csrfToken = csrfToken || $('meta[name=csrf-token]').attr('content'); xhr.setRequestHeader('X-CSRF-Token', csrfToken); } }); diff --git a/app/assets/javascripts/discourse/mixins/ajax.js b/app/assets/javascripts/discourse/mixins/ajax.js index bdb52bbc4..11799c670 100644 --- a/app/assets/javascripts/discourse/mixins/ajax.js +++ b/app/assets/javascripts/discourse/mixins/ajax.js @@ -46,7 +46,7 @@ Discourse.Ajax = Em.Mixin.create({ return Ember.RSVP.resolve(fixture); } - return Ember.Deferred.promise(function (promise) { + var performAjax = function(promise) { var oldSuccess = args.success; args.success = function(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'; $.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(''); + performAjax(promise); + }); + }); + } else { + return Ember.Deferred.promise(performAjax); + } } }); diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 0b1bdc7ca..0a2e9e133 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -14,6 +14,18 @@ class ApplicationController < ActionController::Base 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 :block_if_maintenance_mode before_filter :authorize_mini_profiler diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index d0a195fec..368778f60 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -1,10 +1,11 @@ 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 + def csrf + render json: {csrf: form_authenticity_token } + end + def create params.require(:login) params.require(:password) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 6e6081075..168e1bc95 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -15,7 +15,8 @@ class Users::OmniauthCallbacksController < ApplicationController # need to be able to call this 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 def complete diff --git a/config/routes.rb b/config/routes.rb index 3c4872e76..4647f13f6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -96,6 +96,8 @@ Discourse::Application.routes.draw do end end + get 'session/csrf' => 'session#csrf' + resources :users, except: [:show, :update] do collection do get 'check_username' diff --git a/lib/current_user.rb b/lib/current_user.rb index 4b204726c..38538031b 100644 --- a/lib/current_user.rb +++ b/lib/current_user.rb @@ -17,6 +17,12 @@ module CurrentUser 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) session[:current_user_id] = user.id unless user.auth_token && user.auth_token.length == 32 @@ -30,6 +36,13 @@ module CurrentUser cookies.permanent["_t"] = { value: user.auth_token, httponly: true } end + def is_api? + # ensure current user has been called + # otherwise + current_user + @is_api + end + def current_user return @current_user if @current_user || @not_logged_in @@ -64,6 +77,7 @@ module CurrentUser if api_key = request["api_key"] if api_username = request["api_username"] if SiteSetting.api_key_valid?(api_key) + @is_api = true @current_user = User.where(username_lower: api_username.downcase).first end end diff --git a/lib/discourse.rb b/lib/discourse.rb index 3b89797b2..f8fe64be8 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -20,6 +20,9 @@ module Discourse # When a setting is missing class SiteSettingMissing < Exception; end + # Cross site request forgery + class CSRF < Exception; end + def self.cache @cache ||= Cache.new end