Prevent remember_token timing attacks (#917)

This commit introduces signed cookies into Clearance using the signed cookies
functionality provided by
[ActionDispatch](https://api.rubyonrails.org/classes/ActionDispatch/Cookies.html).
By using a signed cookie an attacker cannot forge the cookie value, and
therefore cannot perform timing attacks to find a valid token. See [this Rack
security
advisory](GHSA-hrqr-hxpp-chr3)
for an example of the type of attack that could be possible with an injectable
cookie value.

This change adds an optional configuration parameter `signed_cookie` which
defaults to false for backwards compatibility and does not use a signed cookie.
The other two options are `true` to use a signed cookie and `:migrate` which
converts unsigned cookies to signed ones and provides a safe transition path.
You can set this via `Clearance.configure` in an initializer:

```ruby
# ./config/initializers/clearance.rb

Clearance.configure do |config|
  # ...

  config.signed_cookie = :migrate

  # ...
end
```

This change also switched to using Rail's `ActionDispatch` for cookie handling
rather than `Rack::Utils`.

See related issues and pull requests:

* #916
* Similar to #909

Co-authored-by: Yoav Aner <yoav@gingerlime.com>
Co-authored-by: Eebs Kobeissi <ebrahim.kobeissi@gmail.com>

Author: 3 people

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, uses Rails' signed cookies
+    # (more secure against timing/brute-force attacks)
+    # see [ActionDispatch::Cookies](https://api.rubyonrails.org/classes/ActionDispatch/Cookies.html)
+    # @return [Boolean|:migrate]
+    attr_reader :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,9 +132,20 @@ def initialize
       @rotate_csrf_on_sign_in = true
       @routes = true
       @secure_cookie = false
+      @signed_cookie = false
       @sign_in_guards = []
     end
 
+    def signed_cookie=(value)
+      if [true, false, :migrate].include? value
+        @signed_cookie = value
+      else
+        raise "Clearance's signed_cookie configuration value is invalid. " \
+              "Valid values are true, false, or :migrate. " \
+              "Set this option via Clearance.configure in an initializer"
+      end
+    end
+
     # The class representing the configured user model.
     # In the default configuration, this is the `User` class.
     # @return [Class]

lib/clearance/rack_session.rb

@@ -23,7 +23,7 @@ def call(env)
       response = @app.call(env)
 
       if session.authentication_successful?
-        session.add_cookie_to_headers response[1]
+        session.add_cookie_to_headers
       end
 
       response

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
       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,27 @@ def cookies
       @cookies ||= ActionDispatch::Request.new(@env).cookie_jar
     end
 
+    # @api private
+    def set_remember_token(token)
+      case Clearance.configuration.signed_cookie
+      when true, :migrate
+        cookies.signed[remember_token_cookie] = cookie_options(token)
+      when false
+        cookies[remember_token_cookie] = cookie_options(token)
+      end
+      remember_token
+    end
+
     # @api private
     def remember_token
-      cookies[remember_token_cookie]
+      case Clearance.configuration.signed_cookie
+      when true
+        cookies.signed[remember_token_cookie]
+      when :migrate
+        cookies.signed[remember_token_cookie] || cookies[remember_token_cookie]
+      when false
+        cookies[remember_token_cookie]
+      end
     end
 
     # @api private
@@ -159,15 +171,15 @@ def initialize_sign_in_guard_stack
     end
 
     # @api private
-    def cookie_options
+    def cookie_options(value)
       {
         domain: domain,
         expires: remember_token_expires,
         httponly: Clearance.configuration.httponly,
         same_site: Clearance.configuration.same_site,
         path: Clearance.configuration.cookie_path,
         secure: Clearance.configuration.secure_cookie,
-        value: remember_token,
+        value: value,
       }
     end
 

spec/clearance/rack_session_spec.rb

@@ -20,7 +20,6 @@
     response = Rack::MockResponse.new(*app.call(env))
 
     expect(response.body).to eq expected_session
-    expect(expected_session).to have_received(:add_cookie_to_headers).
-      with(hash_including(headers))
+    expect(expected_session).to have_received(:add_cookie_to_headers)
   end
 end

spec/clearance/session_spec.rb

@@ -4,7 +4,6 @@
   before { Timecop.freeze }
   after { Timecop.return }
 
-  let(:headers) { {} }
   let(:session) { Clearance::Session.new(env_without_remember_token) }
   let(:user) { create(:user) }
 
@@ -35,9 +34,63 @@
       Clearance.configuration.cookie_name = "custom_cookie_name"
 
       session.sign_in user
-      session.add_cookie_to_headers(headers)
+      session.add_cookie_to_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 == false" 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
+
+  context "with signed cookies == true" 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
+
+  context "with signed cookies == :migrate" do
+    before do
+      Clearance.configuration.signed_cookie = :migrate
+    end
+
+    context "signed cookie exists" do
+      it "uses cookies.signed[remember_token]" do
+        cookie_jar = { "remember_token" => "signed cookie" }
+        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
+
+    context "signed cookie does not exist yet" do
+      it "uses cookies[remember_token] instead" do
+        cookie_jar = { "remember_token" => "signed cookie" }
+        # first call will try to get the signed cookie
+        expect(session).to receive(:cookies).and_return(cookie_jar)
+        # ... but signed_cookie doesn't exist
+        expect(cookie_jar).to receive(:signed).and_return({})
+        # then it attempts to retrieve the unsigned cookie
+        expect(session).to receive(:cookies).and_return(cookie_jar)
+
+        session.sign_in user
+      end
     end
   end
 
@@ -157,9 +210,9 @@ def stub_status(success)
     end
 
     it 'sets a httponly cookie' do
-      session.add_cookie_to_headers(headers)
+      session.add_cookie_to_headers
 
-      expect(headers['Set-Cookie']).to match(/remember_token=.+; HttpOnly/)
+      expect(remember_token_cookie(session)[:httponly]).to be_truthy
     end
   end
 
@@ -170,9 +223,9 @@ def stub_status(success)
     end
 
     it 'sets a standard cookie' do
-      session.add_cookie_to_headers(headers)
+      session.add_cookie_to_headers
 
-      expect(headers['Set-Cookie']).not_to match(/remember_token=.+; HttpOnly/)
+      expect(remember_token_cookie(session)[:httponly]).to be_falsey
     end
   end
 
@@ -183,9 +236,9 @@ def stub_status(success)
     end
 
     it "sets a same-site cookie" do
-      session.add_cookie_to_headers(headers)
+      session.add_cookie_to_headers
 
-      expect(headers["Set-Cookie"]).to match(/remember_token=.+; SameSite/)
+      expect(remember_token_cookie(session)[:same_site]).to eq(:lax)
     end
   end
 
@@ -195,25 +248,21 @@ def stub_status(success)
     end
 
     it "sets a standard cookie" do
-      session.add_cookie_to_headers(headers)
+      session.add_cookie_to_headers
 
-      expect(headers["Set-Cookie"]).to_not match(/remember_token=.+; SameSite/)
+      expect(remember_token_cookie(session)[:same_site]).to be_nil
     end
   end
 
   describe 'remember token cookie expiration' do
     context 'default configuration' do
       it 'is set to 1 year from now' do
         user = double("User", remember_token: "123abc")
-        headers = {}
         session = Clearance::Session.new(env_without_remember_token)
         session.sign_in user
-        session.add_cookie_to_headers headers
+        session.add_cookie_to_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
 
@@ -225,18 +274,14 @@ def stub_status(success)
         end
         with_custom_expiration expires_at do
           user = double("User", remember_token: "123abc")
-          headers = {}
           environment = env_with_cookies(remember_me: 'true')
           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
-          )
-
+          session.add_cookie_to_headers
+          expect(remember_token_cookie(session)[:expires]).to \
+            eq(remembered_expires)
+          expect(remember_token_cookie(session)[:value]).to \
+            eq(user.remember_token)
         end
       end
     end
@@ -249,9 +294,9 @@ def stub_status(success)
       end
 
       it 'sets a standard cookie' do
-        session.add_cookie_to_headers(headers)
+        session.add_cookie_to_headers
 
-        expect(headers['Set-Cookie']).not_to match(/remember_token=.+; secure/)
+        expect(remember_token_cookie(session)[:secure]).to be_falsey
       end
     end
 
@@ -262,9 +307,9 @@ def stub_status(success)
       end
 
       it 'sets a secure cookie' do
-        session.add_cookie_to_headers(headers)
+        session.add_cookie_to_headers
 
-        expect(headers['Set-Cookie']).to match(/remember_token=.+; secure/)
+        expect(remember_token_cookie(session)[:secure]).to be_truthy
       end
     end
   end
@@ -280,19 +325,19 @@ def stub_status(success)
         let(:cookie_domain) { ".example.com" }
 
         it "sets a standard cookie" do
-          session.add_cookie_to_headers(headers)
+          session.add_cookie_to_headers
 
-          expect(headers['Set-Cookie']).to match(/domain=\.example\.com; path/)
+          expect(remember_token_cookie(session)[:domain]).to eq(cookie_domain)
         end
       end
 
       context "with lambda" do
         let(:cookie_domain) { lambda { |_r| ".example.com" } }
 
         it "sets a standard cookie" do
-          session.add_cookie_to_headers(headers)
+          session.add_cookie_to_headers
 
-          expect(headers['Set-Cookie']).to match(/domain=\.example\.com; path/)
+          expect(remember_token_cookie(session)[:domain]).to eq(".example.com")
         end
       end
     end
@@ -301,9 +346,9 @@ def stub_status(success)
       before { session.sign_in(user) }
 
       it 'sets a standard cookie' do
-        session.add_cookie_to_headers(headers)
+        session.add_cookie_to_headers
 
-        expect(headers["Set-Cookie"]).not_to match(/domain=.+; path/)
+        expect(remember_token_cookie(session)[:domain]).to be_nil
       end
     end
   end
@@ -313,9 +358,9 @@ def stub_status(success)
       before { session.sign_in(user) }
 
       it 'sets a standard cookie' do
-        session.add_cookie_to_headers(headers)
+        session.add_cookie_to_headers
 
-        expect(headers["Set-Cookie"]).to_not match(/domain=.+; path/)
+        expect(remember_token_cookie(session)[:domain]).to be_nil
       end
     end
 
@@ -326,18 +371,17 @@ def stub_status(success)
       end
 
       it 'sets a standard cookie' do
-        session.add_cookie_to_headers(headers)
+        session.add_cookie_to_headers
 
-        expect(headers['Set-Cookie']).to match(/path=\/user; expires/)
+        expect(remember_token_cookie(session)[:path]).to eq("/user")
       end
     end
   end
 
   it 'does not set a remember token when signed out' do
-    headers = {}
     session = Clearance::Session.new(env_without_remember_token)
-    session.add_cookie_to_headers headers
-    expect(headers["Set-Cookie"]).to be nil
+    session.add_cookie_to_headers
+    expect(remember_token_cookie(session)).to be_nil
   end
 
   describe "#sign_out" do
@@ -399,6 +443,16 @@ 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
+    set_cookies = cookies.instance_eval <<-RUBY, __FILE__, __LINE__ + 1
+      @set_cookies
+    RUBY
+    set_cookies[cookie_name]
+  end
+
   def env_with_cookies(cookies)
     Rack::MockRequest.env_for '/', 'HTTP_COOKIE' => serialize_cookies(cookies)
   end