diff --git a/Gemfile b/Gemfile
index 906709656f392a71c033e39aac67dba50873e090..e1a7b2adf8668e705ade61727914807546e249bc 100644
--- a/Gemfile
+++ b/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'
diff --git a/Gemfile.lock b/Gemfile.lock
index c65e2252ce654e3cb95d33c25d8514dfe078bc0e..d705bf61e47a634a2fe31bf5af37913fac33abd4 100644
--- a/Gemfile.lock
+++ b/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)
@@ -788,7 +788,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)
@@ -859,17 +859,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)
@@ -1331,7 +1331,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)
diff --git a/app/models/active_session.rb b/app/models/active_session.rb
index f37da1b7f59bafbf2c8671454cdb22ae7a8a4f42..050155398ab39b37e8001bc77c137b5ff0586a08 100644
--- a/app/models/active_session.rb
+++ b/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 @@ def self.set(user, request)
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 @@ def self.list_sessions(user)
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 @@ def self.active_session_entries(session_ids, user_id, redis)
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 @@ def self.clean_up_old_sessions(redis, user)
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 @@ def self.cleaned_up_lookup_entries(redis, user)
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
diff --git a/changelogs/unreleased/security-update-rack-2-0-9.yml b/changelogs/unreleased/security-update-rack-2-0-9.yml
new file mode 100644
index 0000000000000000000000000000000000000000..2dec5d2782ec76289f99fc1a7b92c7ec560d0dfa
--- /dev/null
+++ b/changelogs/unreleased/security-update-rack-2-0-9.yml
@@ -0,0 +1,5 @@
+---
+title: Update rack and related gems to 2.0.9 to fix security issue
+merge_request:
+author:
+type: security
diff --git a/spec/models/active_session_spec.rb b/spec/models/active_session_spec.rb
index 27a80f93566783e0242cbe2c33c500348e50b63c..6a97d91b3ca1006446702bed6c84cadb5cc72771 100644
--- a/spec/models/active_session_spec.rb
+++ b/spec/models/active_session_spec.rb
@@ -9,10 +9,8 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
(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}",
diff --git a/spec/support/rails/test_case_patch.rb b/spec/support/rails/test_case_patch.rb
new file mode 100644
index 0000000000000000000000000000000000000000..161e1ef2a4c980e64ebd77746b0838d8c321b001
--- /dev/null
+++ b/spec/support/rails/test_case_patch.rb
@@ -0,0 +1,53 @@
+# 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