Update rack and related gems to 2.0.9

NOTE: The upgrade is backwards compatible with existing sessions, but
the upgrade of redis-rack v2.1.2 changed Redis keys from
`session:gitlab:<random hex value>` to `session:gitlab:2::<hash of hex
value>`. If a session does not have a key in the new schema, it will be
created transparently. The old session key will eventually be expired
automatically.

To upgrade to rack 2.0.9, we need to do the following:

1. Fix ActiveSession to use new Rack::Session::SessionId
2. Add a monkey patch for ActionController::TestSessionPatch

Controller tests were failing without the changes in
https://github.com/rails/rails/pull/38063, which is available on the
Rails `6-0-stable` branch but not in Rails 6.0.2.2.

3. Remove CGI escaping of ActiveSession keys. This was not needed
because CGI escaping was already being done by Rails.

4. Fix deletion of Rack session keys with ActiveSession

redis-rack v2.1.2 changed the session key from one based on the public
ID to the private ID. We need to adapt ActiveSession to delete both
versions of the key to clear out old data and to make it work with the
redis-rack key name changes.

Gemfile

@@ -163,7 +163,7 @@ gem 'diffy', '~> 3.3'
 gem 'diff_match_patch', '~> 0.1.0'
 
 # Application server
-gem 'rack', '~> 2.0.7'
+gem 'rack', '~> 2.0.9'
 
 group :unicorn do
   gem 'unicorn', '~> 5.4.1'

Gemfile.lock

@@ -173,7 +173,7 @@ GEM
     concord (0.1.5)
       adamantium (~> 0.2.0)
       equalizer (~> 0.0.9)
-    concurrent-ruby (1.1.5)
+    concurrent-ruby (1.1.6)
     connection_pool (2.2.2)
     contracts (0.11.0)
     cork (0.3.0)
@@ -784,7 +784,7 @@ GEM
     public_suffix (4.0.3)
     pyu-ruby-sasl (0.0.3.3)
     raabro (1.1.6)
-    rack (2.0.7)
+    rack (2.0.9)
     rack-accept (0.4.5)
       rack (>= 0.4)
     rack-attack (6.2.0)
@@ -855,17 +855,17 @@ GEM
       json
     recursive-open-struct (1.1.0)
     redis (4.1.3)
-    redis-actionpack (5.1.0)
-      actionpack (>= 4.0, < 7)
-      redis-rack (>= 1, < 3)
+    redis-actionpack (5.2.0)
+      actionpack (>= 5, < 7)
+      redis-rack (>= 2.1.0, < 3)
       redis-store (>= 1.1.0, < 2)
     redis-activesupport (5.2.0)
       activesupport (>= 3, < 7)
       redis-store (>= 1.3, < 2)
     redis-namespace (1.6.0)
       redis (>= 3.0.4)
-    redis-rack (2.0.6)
-      rack (>= 1.5, < 3)
+    redis-rack (2.1.2)
+      rack (>= 2.0.8, < 3)
       redis-store (>= 1.2, < 2)
     redis-rails (5.0.2)
       redis-actionpack (>= 5.0, < 6)
@@ -1325,7 +1325,7 @@ DEPENDENCIES
   prometheus-client-mmap (~> 0.10.0)
   pry-byebug (~> 3.5.1)
   pry-rails (~> 0.3.9)
-  rack (~> 2.0.7)
+  rack (~> 2.0.9)
   rack-attack (~> 6.2.0)
   rack-cors (~> 1.0.6)
   rack-oauth2 (~> 1.9.3)

app/models/active_session.rb

@@ -6,31 +6,32 @@ class ActiveSession
   SESSION_BATCH_SIZE = 200
   ALLOWED_NUMBER_OF_ACTIVE_SESSIONS = 100
 
-  attr_writer :session_id
-
   attr_accessor :created_at, :updated_at,
     :ip_address, :browser, :os,
     :device_name, :device_type,
-    :is_impersonated
+    :is_impersonated, :session_id
 
   def current?(session)
     return false if session_id.nil? || session.id.nil?
 
-    session_id == session.id
+    # Rack v2.0.8+ added private_id, which uses the hash of the
+    # public_id to avoid timing attacks.
+    session_id.private_id == session.id.private_id
   end
 
   def human_device_type
     device_type&.titleize
   end
 
+  # This is not the same as Rack::Session::SessionId#public_id, but we
+  # need to preserve this for backwards compatibility.
   def public_id
-    encrypted_id = Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id)
-    CGI.escape(encrypted_id)
+    Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id.public_id)
   end
 
   def self.set(user, request)
     Gitlab::Redis::SharedState.with do |redis|
-      session_id = request.session.id
+      session_id = request.session.id.public_id
       client = DeviceDetector.new(request.user_agent)
       timestamp = Time.current
 
@@ -63,32 +64,35 @@ class ActiveSession
 
   def self.list(user)
     Gitlab::Redis::SharedState.with do |redis|
-      cleaned_up_lookup_entries(redis, user).map do |entry|
-        # rubocop:disable Security/MarshalLoad
-        Marshal.load(entry)
-        # rubocop:enable Security/MarshalLoad
+      cleaned_up_lookup_entries(redis, user).map do |raw_session|
+        load_raw_session(raw_session)
       end
     end
   end
 
   def self.destroy(user, session_id)
+    return unless session_id
+
     Gitlab::Redis::SharedState.with do |redis|
       destroy_sessions(redis, user, [session_id])
     end
   end
 
   def self.destroy_with_public_id(user, public_id)
-    session_id = decrypt_public_id(public_id)
-    destroy(user, session_id) unless session_id.nil?
+    decrypted_id = decrypt_public_id(public_id)
+
+    return if decrypted_id.nil?
+
+    session_id = Rack::Session::SessionId.new(decrypted_id)
+    destroy(user, session_id)
   end
 
   def self.destroy_sessions(redis, user, session_ids)
-    key_names = session_ids.map {|session_id| key_name(user.id, session_id) }
-    session_names = session_ids.map {|session_id| "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id}" }
+    key_names = session_ids.map { |session_id| key_name(user.id, session_id.public_id) }
 
-    redis.srem(lookup_key_name(user.id), session_ids)
+    redis.srem(lookup_key_name(user.id), session_ids.map(&:public_id))
     redis.del(key_names)
-    redis.del(session_names)
+    redis.del(rack_session_keys(session_ids))
   end
 
   def self.cleanup(user)
@@ -110,28 +114,65 @@ class ActiveSession
     sessions_from_ids(session_ids_for_user(user.id))
   end
 
+  # Lists the relevant session IDs for the user.
+  #
+  # Returns an array of Rack::Session::SessionId objects
   def self.session_ids_for_user(user_id)
     Gitlab::Redis::SharedState.with do |redis|
-      redis.smembers(lookup_key_name(user_id))
+      session_ids = redis.smembers(lookup_key_name(user_id))
+      session_ids.map { |id| Rack::Session::SessionId.new(id) }
     end
   end
 
+  # Lists the ActiveSession objects for the given session IDs.
+  #
+  # session_ids - An array of Rack::Session::SessionId objects
+  #
+  # Returns an array of ActiveSession objects
   def self.sessions_from_ids(session_ids)
     return [] if session_ids.empty?
 
     Gitlab::Redis::SharedState.with do |redis|
-      session_keys = session_ids.map { |session_id| "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id}" }
+      session_keys = rack_session_keys(session_ids)
 
       session_keys.each_slice(SESSION_BATCH_SIZE).flat_map do |session_keys_batch|
         redis.mget(session_keys_batch).compact.map do |raw_session|
-          # rubocop:disable Security/MarshalLoad
-          Marshal.load(raw_session)
-          # rubocop:enable Security/MarshalLoad
+          load_raw_session(raw_session)
         end
       end
     end
   end
 
+  # Deserializes an ActiveSession object from Redis.
+  #
+  # raw_session - Raw bytes from Redis
+  #
+  # Returns an ActiveSession object
+  def self.load_raw_session(raw_session)
+    # rubocop:disable Security/MarshalLoad
+    session = Marshal.load(raw_session)
+    # rubocop:enable Security/MarshalLoad
+
+    # Older ActiveSession models serialize `session_id` as strings, To
+    # avoid breaking older sessions, we keep backwards compatibility
+    # with older Redis keys and initiate Rack::Session::SessionId here.
+    session.session_id = Rack::Session::SessionId.new(session.session_id) if session.try(:session_id).is_a?(String)
+    session
+  end
+
+  def self.rack_session_keys(session_ids)
+    session_ids.each_with_object([]) do |session_id, arr|
+      # This is a redis-rack implementation detail
+      # (https://github.com/redis-store/redis-rack/blob/master/lib/rack/session/redis.rb#L88)
+      #
+      # We need to delete session keys based on the legacy public key name
+      # and the newer private ID keys, but there's no well-defined interface
+      # so we have to do it directly.
+      arr << "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id.public_id}"
+      arr << "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id.private_id}"
+    end
+  end
+
   def self.raw_active_session_entries(redis, session_ids, user_id)
     return [] if session_ids.empty?
 
@@ -146,7 +187,7 @@ class ActiveSession
     entry_keys = raw_active_session_entries(redis, session_ids, user_id)
 
     entry_keys.compact.map do |raw_session|
-      Marshal.load(raw_session) # rubocop:disable Security/MarshalLoad
+      load_raw_session(raw_session)
     end
   end
 
@@ -159,10 +200,13 @@ class ActiveSession
     sessions = active_session_entries(session_ids, user.id, redis)
     sessions.sort_by! {|session| session.updated_at }.reverse!
     destroyable_sessions = sessions.drop(ALLOWED_NUMBER_OF_ACTIVE_SESSIONS)
-    destroyable_session_ids = destroyable_sessions.map { |session| session.send :session_id } # rubocop:disable GitlabSecurity/PublicSend
+    destroyable_session_ids = destroyable_sessions.map { |session| session.session_id }
     destroy_sessions(redis, user, destroyable_session_ids) if destroyable_session_ids.any?
   end
 
+  # Cleans up the lookup set by removing any session IDs that are no longer present.
+  #
+  # Returns an array of marshalled ActiveModel objects that are still active.
   def self.cleaned_up_lookup_entries(redis, user)
     session_ids = session_ids_for_user(user.id)
     entries = raw_active_session_entries(redis, session_ids, user.id)
@@ -181,13 +225,8 @@ class ActiveSession
   end
 
   private_class_method def self.decrypt_public_id(public_id)
-    decoded_id = CGI.unescape(public_id)
-    Gitlab::CryptoHelper.aes256_gcm_decrypt(decoded_id)
+    Gitlab::CryptoHelper.aes256_gcm_decrypt(public_id)
   rescue
     nil
   end
-
-  private
-
-  attr_reader :session_id
 end

changelogs/unreleased/security-update-rack-2-0-9.yml

+---
+title: Update rack and related gems to 2.0.9 to fix security issue
+merge_request:
+author:
+type: security

spec/models/active_session_spec.rb

@@ -9,10 +9,8 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
     end
   end
 
-  let(:session) do
-    double(:session, { id: '6919a6f1bb119dd7396fadc38fd18d0d',
-                       '[]': {} })
-  end
+  let(:rack_session) { Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') }
+  let(:session) { instance_double(ActionDispatch::Request::Session, id: rack_session, '[]': {}) }
 
   let(:request) do
     double(:request, {
@@ -25,13 +23,13 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
 
   describe '#current?' do
     it 'returns true if the active session matches the current session' do
-      active_session = ActiveSession.new(session_id: '6919a6f1bb119dd7396fadc38fd18d0d')
+      active_session = ActiveSession.new(session_id: rack_session)
 
       expect(active_session.current?(session)).to be true
     end
 
     it 'returns false if the active session does not match the current session' do
-      active_session = ActiveSession.new(session_id: '59822c7d9fcdfa03725eff41782ad97d')
+      active_session = ActiveSession.new(session_id: Rack::Session::SessionId.new('59822c7d9fcdfa03725eff41782ad97d'))
 
       expect(active_session.current?(session)).to be false
     end
@@ -46,14 +44,12 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
 
   describe '#public_id' do
     it 'returns an encrypted, url-encoded session id' do
-      original_session_id = "!*'();:@&\n=+$,/?%abcd#123[4567]8"
+      original_session_id = Rack::Session::SessionId.new("!*'();:@&\n=+$,/?%abcd#123[4567]8")
       active_session = ActiveSession.new(session_id: original_session_id)
-      encrypted_encoded_id = active_session.public_id
-
-      encrypted_id = CGI.unescape(encrypted_encoded_id)
+      encrypted_id = active_session.public_id
       derived_session_id = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_id)
 
-      expect(original_session_id).to eq derived_session_id
+      expect(original_session_id.public_id).to eq derived_session_id
     end
   end
 
@@ -104,7 +100,8 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
   describe '.list_sessions' do
     it 'uses the ActiveSession lookup to return original sessions' do
       Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:gitlab:6919a6f1bb119dd7396fadc38fd18d0d", Marshal.dump({ _csrf_token: 'abcd' }))
+        # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88
+        redis.set("session:gitlab:#{rack_session.private_id}", Marshal.dump({ _csrf_token: 'abcd' }))
 
         redis.sadd(
           "session:lookup:user:gitlab:#{user.id}",
@@ -127,17 +124,18 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
         redis.sadd("session:lookup:user:gitlab:#{user.id}", session_ids)
       end
 
-      expect(ActiveSession.session_ids_for_user(user.id)).to eq(session_ids)
+      expect(ActiveSession.session_ids_for_user(user.id).map(&:to_s)).to eq(session_ids)
     end
   end
 
   describe '.sessions_from_ids' do
     it 'uses the ActiveSession lookup to return original sessions' do
       Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:gitlab:6919a6f1bb119dd7396fadc38fd18d0d", Marshal.dump({ _csrf_token: 'abcd' }))
+        # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88
+        redis.set("session:gitlab:#{rack_session.private_id}", Marshal.dump({ _csrf_token: 'abcd' }))
       end
 
-      expect(ActiveSession.sessions_from_ids(['6919a6f1bb119dd7396fadc38fd18d0d'])).to eq [{ _csrf_token: 'abcd' }]
+      expect(ActiveSession.sessions_from_ids([rack_session])).to eq [{ _csrf_token: 'abcd' }]
     end
 
     it 'avoids a redis lookup for an empty array' do
@@ -152,11 +150,12 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
       redis = double(:redis)
       expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis)
 
-      sessions = %w[session-a session-b]
+      sessions = %w[session-a session-b session-c session-d]
       mget_responses = sessions.map { |session| [Marshal.dump(session)]}
-      expect(redis).to receive(:mget).twice.and_return(*mget_responses)
+      expect(redis).to receive(:mget).exactly(4).times.and_return(*mget_responses)
 
-      expect(ActiveSession.sessions_from_ids([1, 2])).to eql(sessions)
+      session_ids = [1, 2].map { |id| Rack::Session::SessionId.new(id.to_s) }
+      expect(ActiveSession.sessions_from_ids(session_ids).map(&:to_s)).to eql(sessions)
     end
   end
 
@@ -212,6 +211,12 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
   end
 
   describe '.destroy' do
+    it 'gracefully handles a nil session ID' do
+      expect(described_class).not_to receive(:destroy_sessions)
+
+      ActiveSession.destroy(user, nil)
+    end
+
     it 'removes the entry associated with the currently killed user session' do
       Gitlab::Redis::SharedState.with do |redis|
         redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '')
@@ -244,8 +249,9 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
 
     it 'removes the devise session' do
       Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '')
-        redis.set("session:gitlab:6919a6f1bb119dd7396fadc38fd18d0d", '')
+        redis.set("session:user:gitlab:#{user.id}:#{rack_session.public_id}", '')
+        # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88
+        redis.set("session:gitlab:#{rack_session.private_id}", '')
       end
 
       ActiveSession.destroy(user, request.session.id)
@@ -322,7 +328,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
           (1..max_number_of_sessions_plus_two).each do |number|
             redis.set(
               "session:user:gitlab:#{user.id}:#{number}",
-              Marshal.dump(ActiveSession.new(session_id: "#{number}", updated_at: number.days.ago))
+              Marshal.dump(ActiveSession.new(session_id: number.to_s, updated_at: number.days.ago))
             )
             redis.sadd(
               "session:lookup:user:gitlab:#{user.id}",

spec/support/rails/test_case_patch.rb

+# frozen_string_literal: true
+#
+# This file pulls in the changes in https://github.com/rails/rails/pull/38063
+# to fix controller specs updated with the latest Rack versions.
+#
+# This file should be removed after that change ships. It is not
+# present in Rails 6.0.2.2.
+module ActionController
+  class TestRequest < ActionDispatch::TestRequest #:nodoc:
+    def self.new_session
+      TestSessionPatched.new
+    end
+  end
+
+  # Methods #destroy and #load! are overridden to avoid calling methods on the
+  # @store object, which does not exist for the TestSession class.
+  class TestSessionPatched < Rack::Session::Abstract::PersistedSecure::SecureSessionHash #:nodoc:
+    DEFAULT_OPTIONS = Rack::Session::Abstract::Persisted::DEFAULT_OPTIONS
+
+    def initialize(session = {})
+      super(nil, nil)
+      @id = Rack::Session::SessionId.new(SecureRandom.hex(16))
+      @data = stringify_keys(session)
+      @loaded = true
+    end
+
+    def exists?
+      true
+    end
+
+    def keys
+      @data.keys
+    end
+
+    def values
+      @data.values
+    end
+
+    def destroy
+      clear
+    end
+
+    def fetch(key, *args, &block)
+      @data.fetch(key.to_s, *args, &block)
+    end
+
+    private
+
+    def load!
+      @id
+    end
+  end
+end