From 364d6ead3d568f9b7cb427d96b9432385ec53459 Mon Sep 17 00:00:00 2001 From: Yoav Aner Date: Sun, 29 Nov 2020 09:48:20 +0000 Subject: [PATCH] prevent remember_token timing attacks * see https://github.com/thoughtbot/clearance/issues/916 * similar to https://github.com/thoughtbot/clearance/pull/909 * also see https://github.com/rack/rack/security/advisories/GHSA-hrqr-hxpp-chr3 for an example of the type of attack that could be possible with an injectable cookie value * Rails provides signed cookies https://api.rubyonrails.org/classes/ActionDispatch/Cookies.html since Rails 3 (??) which prevents tampering * using a signed cookie instead of a plain one, means the attacker cannot forge the cookie value, and therefore cannot perform timing attacks to find a valid token * another added value is that tampering with the cookie will not even hit the database * added a configuration parameter `signed_cookie` so this is optional and defaults to false for backwards compatibility (however, for better security, it might be better to issue a breaking change and default to true) * changed the add_cookies_to_headers method to use ActionDispatch / Rails' cookie-handling code to set the cookie * updated specs --- lib/clearance/configuration.rb | 9 ++++ lib/clearance/session.rb | 30 +++++++----- spec/clearance/session_spec.rb | 53 +++++++++++++-------- spec/configuration_spec.rb | 13 +++++ spec/support/clearance.rb | 11 +++++ spec/support/request_with_remember_token.rb | 14 +++--- 6 files changed, 94 insertions(+), 36 deletions(-) diff --git a/lib/clearance/configuration.rb b/lib/clearance/configuration.rb index cccefd97a..c2cfa59e8 100644 --- a/lib/clearance/configuration.rb +++ b/lib/clearance/configuration.rb @@ -88,6 +88,14 @@ class Configuration # @return [Boolean] attr_accessor :secure_cookie + # Controls whether cookies are signed. + # Defaults to `false` for backwards compatibility. + # When set, using Rails' signed cookies + # (more secure against timing/bruteforce attachs) + # see [ActionDispatch::Cookies](https://api.rubyonrails.org/classes/ActionDispatch/Cookies.html) + # @return [Boolean] + attr_accessor :signed_cookie + # The array of sign in guards to run when signing a user in. # Defaults to an empty array. Sign in guards respond to `call` and are # initialized with a session and the current stack. Each guard can decide @@ -124,6 +132,7 @@ def initialize @rotate_csrf_on_sign_in = true @routes = true @secure_cookie = false + @signed_cookie = false @sign_in_guards = [] end diff --git a/lib/clearance/session.rb b/lib/clearance/session.rb index 980b17f02..5d362edc0 100644 --- a/lib/clearance/session.rb +++ b/lib/clearance/session.rb @@ -14,15 +14,9 @@ def initialize(env) # Called by {RackSession} to add the Clearance session cookie to a response. # # @return [void] - def add_cookie_to_headers(headers) + def add_cookie_to_headers(_headers) if signed_in_with_remember_token? - Rack::Utils.set_cookie_header!( - headers, - remember_token_cookie, - cookie_options.merge( - value: current_user.remember_token, - ), - ) + set_remember_token(current_user.remember_token) end end @@ -112,9 +106,23 @@ def cookies @cookies ||= ActionDispatch::Request.new(@env).cookie_jar end + # @api private + def set_remember_token(token) + if Clearance.configuration.signed_cookie + cookies.signed[remember_token_cookie] = cookie_options(token) + else + cookies[remember_token_cookie] = cookie_options(token) + end + remember_token + end + # @api private def remember_token - cookies[remember_token_cookie] + if Clearance.configuration.signed_cookie + cookies.signed[remember_token_cookie] + else + cookies[remember_token_cookie] + end end # @api private @@ -159,7 +167,7 @@ def initialize_sign_in_guard_stack end # @api private - def cookie_options + def cookie_options(value) { domain: domain, expires: remember_token_expires, @@ -167,7 +175,7 @@ def cookie_options same_site: Clearance.configuration.same_site, path: Clearance.configuration.cookie_path, secure: Clearance.configuration.secure_cookie, - value: remember_token, + value: value, } end diff --git a/spec/clearance/session_spec.rb b/spec/clearance/session_spec.rb index c742ed51e..4ff2c50d8 100644 --- a/spec/clearance/session_spec.rb +++ b/spec/clearance/session_spec.rb @@ -37,7 +37,19 @@ session.sign_in user session.add_cookie_to_headers(headers) - expect(headers["Set-Cookie"]).to match(/custom_cookie_name=.+;/) + expect(remember_token_cookie(session, "custom_cookie_name")).to be_present + end + end + + context "with signed cookies" do + it "uses cookies.signed" do + Clearance.configuration.signed_cookie = true + + cookie_jar = {} + expect(session).to receive(:cookies).and_return(cookie_jar) + expect(cookie_jar).to receive(:signed).and_return(cookie_jar) + + session.sign_in user end end @@ -159,7 +171,7 @@ def stub_status(success) it 'sets a httponly cookie' do session.add_cookie_to_headers(headers) - expect(headers['Set-Cookie']).to match(/remember_token=.+; HttpOnly/) + expect(remember_token_cookie(session)[:httponly]).to be_truthy end end @@ -185,7 +197,7 @@ def stub_status(success) it "sets a same-site cookie" do session.add_cookie_to_headers(headers) - expect(headers["Set-Cookie"]).to match(/remember_token=.+; SameSite/) + expect(remember_token_cookie(session)[:same_site]).to eq(:lax) end end @@ -210,10 +222,7 @@ def stub_status(success) session.sign_in user session.add_cookie_to_headers headers - expect(headers).to set_cookie( - 'remember_token', - user.remember_token, 1.year.from_now - ) + expect(remember_token_cookie(session)[:expires]).to eq(1.year.from_now) end end @@ -230,13 +239,12 @@ def stub_status(success) session = Clearance::Session.new(environment) session.sign_in user session.add_cookie_to_headers headers - - expect(headers).to set_cookie( - 'remember_token', - user.remember_token, - remembered_expires - ) - + expect( + remember_token_cookie(session)[:expires] + ).to eq(remembered_expires) + expect( + remember_token_cookie(session)[:value] + ).to eq(user.remember_token) end end end @@ -251,7 +259,7 @@ def stub_status(success) it 'sets a standard cookie' do session.add_cookie_to_headers(headers) - expect(headers['Set-Cookie']).not_to match(/remember_token=.+; secure/) + expect(remember_token_cookie(session)[:secure]).to be_falsey end end @@ -264,7 +272,7 @@ def stub_status(success) it 'sets a secure cookie' do session.add_cookie_to_headers(headers) - expect(headers['Set-Cookie']).to match(/remember_token=.+; secure/) + expect(remember_token_cookie(session)[:secure]).to be_truthy end end end @@ -282,7 +290,7 @@ def stub_status(success) it "sets a standard cookie" do session.add_cookie_to_headers(headers) - expect(headers['Set-Cookie']).to match(/domain=\.example\.com; path/) + expect(remember_token_cookie(session)[:domain]).to eq(cookie_domain) end end @@ -292,7 +300,7 @@ def stub_status(success) it "sets a standard cookie" do session.add_cookie_to_headers(headers) - expect(headers['Set-Cookie']).to match(/domain=\.example\.com; path/) + expect(remember_token_cookie(session)[:domain]).to eq(".example.com") end end end @@ -328,7 +336,7 @@ def stub_status(success) it 'sets a standard cookie' do session.add_cookie_to_headers(headers) - expect(headers['Set-Cookie']).to match(/path=\/user; expires/) + expect(remember_token_cookie(session)[:path]).to eq("/user") end end end @@ -399,6 +407,13 @@ def stub_status(success) end end + # a bit of a hack to get the cookies that ActionDispatch sets inside session + def remember_token_cookie(session, cookie_name="remember_token") + cookies = session.send(:cookies) + # see https://stackoverflow.com/a/21315095 + cookies.instance_eval("@set_cookies")[cookie_name] + end + def env_with_cookies(cookies) Rack::MockRequest.env_for '/', 'HTTP_COOKIE' => serialize_cookies(cookies) end diff --git a/spec/configuration_spec.rb b/spec/configuration_spec.rb index aef06dc80..7e8027bca 100644 --- a/spec/configuration_spec.rb +++ b/spec/configuration_spec.rb @@ -66,6 +66,19 @@ end end + context "when signed_cookie is set to true" do + it "returns true" do + Clearance.configure { |config| config.signed_cookie = true } + expect(Clearance.configuration.signed_cookie).to eq true + end + end + + context "when signed_cookie is not specified" do + it "defaults to false" do + expect(Clearance.configuration.signed_cookie).to eq false + end + end + context "when no redirect URL specified" do it 'returns "/" as redirect URL' do expect(Clearance::Configuration.new.redirect_url).to eq "/" diff --git a/spec/support/clearance.rb b/spec/support/clearance.rb index 2d62115b5..b6737bb53 100644 --- a/spec/support/clearance.rb +++ b/spec/support/clearance.rb @@ -4,6 +4,17 @@ # need an empty block to initialize the configuration object end +# NOTE: to run the entire suite with signed cookies +# you can set the signed_cookie default to true +# and run all specs. +# However, to fake the actual signing process you +# can monkey-patch ActionDispatch so signed cookies +# behave like normal ones +# +# class ActionDispatch::Cookies::CookieJar +# def signed; self; end +# end + module Clearance module Test module Redirects diff --git a/spec/support/request_with_remember_token.rb b/spec/support/request_with_remember_token.rb index 56aa7d644..ba3d6b0f8 100644 --- a/spec/support/request_with_remember_token.rb +++ b/spec/support/request_with_remember_token.rb @@ -1,11 +1,13 @@ module RememberTokenHelpers def request_with_remember_token(remember_token) - cookies = { - 'action_dispatch.cookies' => { - Clearance.configuration.cookie_name => remember_token - } - } - env = { clearance: Clearance::Session.new(cookies) } + cookies = ActionDispatch::Request.new({}).cookie_jar + if Clearance.configuration.signed_cookie + cookies.signed[Clearance.configuration.cookie_name] = remember_token + else + cookies[Clearance.configuration.cookie_name] = remember_token + end + + env = { clearance: Clearance::Session.new(cookies.request.env) } Rack::Request.new env end