From fbda0f4344b0e18dd060122e9df390fc7803befb Mon Sep 17 00:00:00 2001 From: Jacob Fugal Date: Fri, 14 Nov 2014 09:45:04 -0700 Subject: [PATCH] use SSL-only flag on CSRF cookie fixes CNVS-16884 test-plan: - in HTTP-session environment (`secure: false` or unset in config/session_store.yml), CSRF cookie should be sent without secure flag - in HTTPS-session environment (`secure: true` in config/session_store.yml), CSRF cookie should be sent with secure flag. - on a non-files host, javascript should be able to read the CSRF cookie: value of "document.cookie" in javascript console should include "_csrf_token" - on a files host, javascript should not be able to read the CSRF cookie: value of "document.cookie" in javascript console should not include "_csrf_token" Change-Id: Ifd57c973478a6d07497a404dcf1a9b9caa9014af Reviewed-on: https://gerrit.instructure.com/44451 Reviewed-by: Cody Cutrer Reviewed-by: Rob Orton QA-Review: August Thornton Tested-by: Jenkins Product-Review: Cody Cutrer --- app/controllers/application_controller.rb | 2 +- .../masking_secrets.rb | 24 +++-- .../spec/masking_secrets_spec.rb | 96 ++++++++++++------- lib/authentication_methods.rb | 11 ++- spec/lib/authentication_methods_spec.rb | 57 ++++++++++- 5 files changed, 143 insertions(+), 47 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 84741da9f18..653e2c97846 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1115,7 +1115,7 @@ class ApplicationController < ActionController::Base end def form_authenticity_token - CanvasBreachMitigation::MaskingSecrets.masked_authenticity_token(cookies) + masked_authenticity_token end API_REQUEST_REGEX = %r{\A/api/v\d} diff --git a/gems/canvas_breach_mitigation/lib/canvas_breach_mitigation/masking_secrets.rb b/gems/canvas_breach_mitigation/lib/canvas_breach_mitigation/masking_secrets.rb index ff15de5f2f9..8615dbb162d 100644 --- a/gems/canvas_breach_mitigation/lib/canvas_breach_mitigation/masking_secrets.rb +++ b/gems/canvas_breach_mitigation/lib/canvas_breach_mitigation/masking_secrets.rb @@ -6,13 +6,16 @@ module CanvasBreachMitigation # Sets the token value for the current session and returns it in # a masked form that's safe to send to the client. See section # 3.4 of "BREACH: Reviving the CRIME attack". - def masked_authenticity_token(cookies) - one_time_pad = SecureRandom.random_bytes(AUTHENTICITY_TOKEN_LENGTH) + def masked_authenticity_token(cookies, options={}) + # remask token + encoded_masked_token = masked_token(unmasked_token(cookies['_csrf_token'])) - encrypted_csrf_token = xor_byte_strings(one_time_pad, unmasked_token(cookies['_csrf_token'])) - masked_token = one_time_pad + encrypted_csrf_token - encoded_masked_token = Base64.strict_encode64(masked_token) - cookies['_csrf_token'] = encoded_masked_token + cookie = { value: encoded_masked_token } + [:domain, :httponly, :secure].each do |key| + next unless options.has_key?(key) + cookie[key] = options[key] + end + cookies['_csrf_token'] = cookie encoded_masked_token end @@ -27,6 +30,13 @@ module CanvasBreachMitigation unmasked_token(cookies['_csrf_token']) == unmasked_token(encoded_masked_token) end + def masked_token(token) + one_time_pad = SecureRandom.random_bytes(AUTHENTICITY_TOKEN_LENGTH) + encrypted_csrf_token = xor_byte_strings(one_time_pad, token) + masked_token = one_time_pad + encrypted_csrf_token + Base64.strict_encode64(masked_token) + end + private def unmasked_token(encoded_masked_token) @@ -46,4 +56,4 @@ module CanvasBreachMitigation end end end -end \ No newline at end of file +end diff --git a/gems/canvas_breach_mitigation/spec/masking_secrets_spec.rb b/gems/canvas_breach_mitigation/spec/masking_secrets_spec.rb index c27e5f72fad..2b8e9163686 100644 --- a/gems/canvas_breach_mitigation/spec/masking_secrets_spec.rb +++ b/gems/canvas_breach_mitigation/spec/masking_secrets_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" describe CanvasBreachMitigation::MaskingSecrets do before do - Rails = double("Rails").as_null_object unless defined? Rails + Rails = mock("Rails") unless defined? Rails end let(:masking_secrets) { CanvasBreachMitigation::MaskingSecrets } @@ -11,59 +11,85 @@ describe CanvasBreachMitigation::MaskingSecrets do it "puts :_csrf_token into the supplied object" do hash = {} masking_secrets.masked_authenticity_token(hash) - hash['_csrf_token'].should_not be_nil + expect(hash['_csrf_token']).not_to be nil end it "returns a byte string" do - masking_secrets.masked_authenticity_token({}).should_not be nil + expect(masking_secrets.masked_authenticity_token({})).not_to be nil end end describe ".valid_authenticity_token?" do - let(:cookies) do - # Seed a "cookie jar" with a :_csrf_token - Hash.new.tap do |cookies| - masking_secrets.masked_authenticity_token(cookies) - end - end - - let(:session) do - Hash.new - end - - it "returns true for a valid unmasked token" do - valid_unmasked = cookies['_csrf_token'] - masking_secrets.valid_authenticity_token?(session, cookies, valid_unmasked).should == true - end - - it "returns false for an invalid unmasked token" do - masking_secrets.valid_authenticity_token?(session, cookies, SecureRandom.base64(32)).should == false - end + let(:unmasked_token) { SecureRandom.base64(32) } + let(:cookies) { {'_csrf_token' => masking_secrets.masked_token(unmasked_token)} } + let(:session) { {} } it "returns true for a valid masked token" do - valid_masked = masking_secrets.masked_authenticity_token(cookies) - masking_secrets.valid_authenticity_token?(session, cookies, valid_masked).should == true + valid_masked = masking_secrets.masked_token(unmasked_token) + expect(masking_secrets.valid_authenticity_token?(session, cookies, valid_masked)).to be true end it "returns false for an invalid masked token" do - masking_secrets.valid_authenticity_token?(session, cookies, SecureRandom.base64(64)).should == false + expect(masking_secrets.valid_authenticity_token?(session, cookies, SecureRandom.base64(64))).to be false end it "returns false for a token of the wrong length" do - masking_secrets.valid_authenticity_token?(session, cookies, SecureRandom.base64(2)).should == false + expect(masking_secrets.valid_authenticity_token?(session, cookies, SecureRandom.base64(2))).to be false end it "returns true if the session is still set instead of the cookies" do cookies.delete('_csrf_token') - session[:_csrf_token] = SecureRandom.base64(32) - one_time_pad = SecureRandom.random_bytes(32) - - encrypted_csrf_token = masking_secrets.send(:xor_byte_strings, one_time_pad, Base64.strict_decode64(session[:_csrf_token])) - masked_token = one_time_pad + encrypted_csrf_token - encoded_masked_token = Base64.strict_encode64(masked_token) - - masking_secrets.valid_authenticity_token?(session, cookies, encoded_masked_token).should == true + encoded_masked_token = masking_secrets.masked_token(Base64.strict_decode64(session[:_csrf_token])) + expect(masking_secrets.valid_authenticity_token?(session, cookies, encoded_masked_token)).to be true end end -end \ No newline at end of file + + describe ".masked_authenticity_token" do + let(:cookies) { {} } + + it "initializes the _csrf_token cookie" do + token = masking_secrets.masked_authenticity_token(cookies) + expect(cookies['_csrf_token'][:value]).to eq token + end + + it "initializes the _csrf_token cookie without explicit domain by default" do + masking_secrets.masked_authenticity_token(cookies) + expect(cookies['_csrf_token']).not_to be_has_key(:domain) + end + + it "initializes the _csrf_token cookie without explicit httponly by default" do + masking_secrets.masked_authenticity_token(cookies) + expect(cookies['_csrf_token']).not_to be_has_key(:httponly) + end + + it "initializes the _csrf_token cookie without explicit secure by default" do + masking_secrets.masked_authenticity_token(cookies) + expect(cookies['_csrf_token']).not_to be_has_key(:secure) + end + + it "copies domain option to _csrf_token cookie" do + masking_secrets.masked_authenticity_token(cookies, domain: "domain") + expect(cookies['_csrf_token'][:domain]).to eq "domain" + end + + it "copies httponly option to _csrf_token cookie" do + masking_secrets.masked_authenticity_token(cookies, httponly: true) + expect(cookies['_csrf_token'][:httponly]).to be true + end + + it "copies secure option to _csrf_token cookie" do + masking_secrets.masked_authenticity_token(cookies, secure: true) + expect(cookies['_csrf_token'][:secure]).to be true + end + + it "remasks an existing _csrf_token cookie" do + original_token = masking_secrets.masked_token(SecureRandom.base64(32)) + cookies['_csrf_token'] = original_token + token = masking_secrets.masked_authenticity_token(cookies) + expect(token).not_to eq original_token + expect(cookies['_csrf_token'][:value]).to eq token + expect(masking_secrets.valid_authenticity_token?({}, {'_csrf_token' => token}, original_token)).to be true + end + end +end diff --git a/lib/authentication_methods.rb b/lib/authentication_methods.rb index ce474853597..bdaa8cd7a4a 100644 --- a/lib/authentication_methods.rb +++ b/lib/authentication_methods.rb @@ -83,9 +83,18 @@ module AuthenticationMethods end end + def masked_authenticity_token + session_options = CanvasRails::Application.config.session_options + options = session_options.slice(:domain, :secure) + options[:httponly] = HostUrl.is_file_host?(request.host_with_port) + CanvasBreachMitigation::MaskingSecrets.masked_authenticity_token(cookies, options) + end + private :masked_authenticity_token + def load_user @current_user = @current_pseudonym = nil - CanvasBreachMitigation::MaskingSecrets.masked_authenticity_token(cookies) # ensure that the cookie is set + + masked_authenticity_token # ensure that the cookie is set load_pseudonym_from_access_token diff --git a/spec/lib/authentication_methods_spec.rb b/spec/lib/authentication_methods_spec.rb index 33de7fe5caf..56d1b949df4 100644 --- a/spec/lib/authentication_methods_spec.rb +++ b/spec/lib/authentication_methods_spec.rb @@ -49,7 +49,8 @@ describe AuthenticationMethods do cas_ticket = CanvasUuid::Uuid.generate_securish_uuid request = stub(:env => {'encrypted_cookie_store.session_refreshed_at' => 5.minutes.ago}, - :format => stub(:json? => false)) + :format => stub(:json? => false), + :host_with_port => "") controller = RSpec::MockController.new(domain_root_account, request) controller.expects(:destroy_session).once controller.expects(:redirect_to_login).once @@ -137,7 +138,8 @@ describe AuthenticationMethods do describe "#load_user" do before do @request = stub(:env => {'encrypted_cookie_store.session_refreshed_at' => 5.minutes.ago}, - :format => stub(:json? => false)) + :format => stub(:json? => false), + :host_with_port => "") @controller = RSpec::MockController.new(nil, @request) @controller.stubs(:load_pseudonym_from_access_token) @controller.stubs(:api_request?).returns(false) @@ -174,6 +176,53 @@ describe AuthenticationMethods do expect(@controller.instance_variable_get(:@current_user)).to eq @user expect(@controller.instance_variable_get(:@current_pseudonym)).to eq @pseudonym end + + it "should set the CSRF cookie" do + @controller.send(:load_user) + expect(@controller.cookies['_csrf_token']).not_to be nil + end + end + end + + describe "#masked_authenticity_token" do + before do + @request = stub(host_with_port: "") + @controller = RSpec::MockController.new(nil, @request) + @session_options = {} + CanvasRails::Application.config.expects(:session_options).at_least_once.returns(@session_options) + end + + it "should not set SSL-only explicitly if session_options doesn't specify" do + @controller.send(:masked_authenticity_token) + expect(@controller.cookies['_csrf_token']).not_to be_has_key(:secure) + end + + it "should set SSL-only if session_options specifies" do + @session_options[:secure] = true + @controller.send(:masked_authenticity_token) + expect(@controller.cookies['_csrf_token'][:secure]).to be true + end + + it "should set httponly explicitly false on a non-files host" do + @controller.send(:masked_authenticity_token) + expect(@controller.cookies['_csrf_token'][:httponly]).to be false + end + + it "should set httponly explicitly true on a files host" do + HostUrl.expects(:is_file_host?).once.with(@request.host_with_port).returns(true) + @controller.send(:masked_authenticity_token) + expect(@controller.cookies['_csrf_token'][:httponly]).to be true + end + + it "should not set a cookie domain explicitly if session_options doesn't specify" do + @controller.send(:masked_authenticity_token) + expect(@controller.cookies['_csrf_token']).not_to be_has_key(:domain) + end + + it "should set a cookie domain explicitly if session_options specifies" do + @session_options[:domain] = "cookie domain" + @controller.send(:masked_authenticity_token) + expect(@controller.cookies['_csrf_token'][:domain]).to eq @session_options[:domain] end end end @@ -205,6 +254,8 @@ class RSpec::MockController options[:target] end - def cookies; {}; end + def cookies + @cookies ||= {} + end end