prevent remember_token timing attacks

* see thoughtbot#916
* similar to thoughtbot#909
* also see 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

@@ -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
 

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,15 +167,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/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

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 "/"

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

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