Merge branch 'security-geo-2019-q1-recurity-assessment' into 'master'

Geo - 2019 Q1 Recurity Assessment

See merge request gitlab/gitlab-ee!838

ee/app/controllers/ee/sessions_controller.rb

@@ -14,7 +14,7 @@ module EE
       return super if signed_in?
 
       if ::Gitlab::Geo.secondary_with_primary?
-        redirect_to oauth_geo_auth_url(state: geo_login_state.encode)
+        redirect_to oauth_geo_auth_url(host: ::Gitlab::Geo.current_node.url, state: geo_login_state.encode)
       else
         super
       end
@@ -33,7 +33,7 @@ module EE
     end
 
     def geo_login_state
-      ::Gitlab::Geo::Oauth::LoginState.new(return_to: geo_return_to_after_login)
+      ::Gitlab::Geo::Oauth::LoginState.new(return_to: sanitize_redirect(geo_return_to_after_login))
     end
 
     def geo_logout_state

ee/changelogs/unreleased/security-geo-2019-q1-recurity-assessment.yml

+---
+title: Geo - Improve security while redirecting user back to the secondary after a
+  logout & re-login via the primary
+merge_request:
+author:
+type: security

ee/lib/gitlab/geo/oauth/login_state.rb

@@ -7,40 +7,57 @@ module Gitlab
         attr_reader :return_to
 
         def self.from_state(state)
-          salt, hmac, return_to = state.to_s.split(':', 3)
-          self.new(salt: salt, hmac: hmac, return_to: return_to)
+          salt, token, return_to = state.to_s.split(':', 3)
+          self.new(salt: salt, token: token, return_to: return_to)
         end
 
-        def initialize(return_to:, salt: nil, hmac: nil)
-          @return_to = return_to
+        def initialize(return_to:, salt: nil, token: nil)
+          @return_to = Gitlab::ReturnToLocation.new(return_to).full_path
           @salt = salt
-          @hmac = hmac
+          @token = token
         end
 
-        def valid?
-          return false unless salt.present? && hmac.present?
-
-          hmac == generate_hmac
+        def encode
+          "#{salt}:#{hmac_token}:#{return_to}"
         end
 
-        def encode
-          "#{salt}:#{generate_hmac}:#{return_to}"
+        def valid?
+          return false unless salt.present? && token.present?
+
+          decoded_token = JSONWebToken::HMACToken.decode(token, key).first
+          secure_compare(decoded_token.dig('data', 'return_to'))
+        rescue JWT::DecodeError
+          false
         end
 
         private
 
-        attr_reader :hmac
+        attr_reader :token
 
-        def generate_hmac
-          digest = OpenSSL::Digest::SHA256.new
-          key = Gitlab::Application.secrets.secret_key_base + salt
+        def expiration_time
+          1.minute
+        end
+
+        def hmac_token
+          hmac_token = JSONWebToken::HMACToken.new(key)
+          hmac_token.expire_time = Time.now + expiration_time
+          hmac_token[:data] = { return_to: return_to.to_s }
+          hmac_token.encoded
+        end
 
-          OpenSSL::HMAC.hexdigest(digest, key, return_to.to_s)
+        def key
+          ActiveSupport::KeyGenerator
+            .new(Gitlab::Application.secrets.secret_key_base)
+            .generate_key(salt)
         end
 
         def salt
           @salt ||= SecureRandom.hex(8)
         end
+
+        def secure_compare(value)
+          ActiveSupport::SecurityUtils.secure_compare(return_to.to_s, value)
+        end
       end
     end
   end

ee/spec/controllers/ee/sessions_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe SessionsController do
+  include DeviseHelpers
+  include EE::GeoHelpers
+
+  describe '#new' do
+    before do
+      set_devise_mapping(context: @request)
+    end
+
+    context 'on a Geo secondary node' do
+      set(:primary_node) { create(:geo_node, :primary) }
+      set(:secondary_node) { create(:geo_node) }
+
+      before do
+        stub_current_geo_node(secondary_node)
+      end
+
+      shared_examples 'a valid oauth authentication redirect' do
+        it 'redirects to the correct oauth_geo_auth_url' do
+          get(:new)
+
+          redirect_uri = URI.parse(response.location)
+          redirect_params = CGI.parse(redirect_uri.query)
+
+          expect(response).to have_gitlab_http_status(302)
+          expect(response).to redirect_to %r(\A#{primary_node.url}oauth/geo/auth)
+          expect(redirect_params['state'].first).to end_with(':')
+        end
+      end
+
+      context 'with a tampered HOST header' do
+        before do
+          request.headers['HOST'] = 'http://this.is.not.my.host'
+        end
+
+        it_behaves_like 'a valid oauth authentication redirect'
+      end
+
+      context 'with a tampered X-Forwarded-Host header' do
+        before do
+          request.headers['X-Forwarded-Host'] = 'http://this.is.not.my.host'
+        end
+
+        it_behaves_like 'a valid oauth authentication redirect'
+      end
+
+      context 'without a tampered header' do
+        it_behaves_like 'a valid oauth authentication redirect'
+      end
+    end
+  end
+end

ee/spec/controllers/oauth/geo_auth_controller_spec.rb

@@ -27,12 +27,34 @@ describe Oauth::GeoAuthController do
       expect(response).to redirect_to(root_url)
     end
 
-    it "redirects to primary node's oauth endpoint" do
-      oauth_endpoint = Gitlab::Geo::Oauth::Session.new.authorize_url(redirect_uri: oauth_geo_callback_url, state: login_state)
+    shared_examples "a valid redirect to to primary node's oauth endpoint" do
+      it "redirects to primary node's oauth endpoint" do
+        oauth_endpoint = Gitlab::Geo::Oauth::Session.new.authorize_url(redirect_uri: oauth_geo_callback_url, state: login_state)
 
-      get :auth, params: { state: login_state }
+        get :auth, params: { state: login_state }
+
+        expect(response).to redirect_to(oauth_endpoint)
+      end
+    end
+
+    context 'without a tampered header' do
+      it_behaves_like "a valid redirect to to primary node's oauth endpoint"
+    end
+
+    context 'with a tampered HOST header' do
+      before do
+        request.headers['HOST'] = 'http://this.is.not.my.host'
+      end
+
+      it_behaves_like "a valid redirect to to primary node's oauth endpoint"
+    end
+
+    context 'with a tampered X-Forwarded-Host header' do
+      before do
+        request.headers['X-Forwarded-Host'] = 'http://this.is.not.my.host'
+      end
 
-      expect(response).to redirect_to(oauth_endpoint)
+      it_behaves_like "a valid redirect to to primary node's oauth endpoint"
     end
   end
 
@@ -55,16 +77,40 @@ describe Oauth::GeoAuthController do
         expect(response).to redirect_to(new_user_session_path)
       end
 
-      it 'redirects to redirect_url if state is valid' do
-        get :callback, params: { state: login_state }
+      context 'with a valid state' do
+        shared_examples 'a valid redirect to redirect_url' do
+          it "redirects to primary node's oauth endpoint" do
+            get :callback, params: { state: login_state }
 
-        expect(response).to redirect_to(secondary_node.url)
-      end
+            expect(response).to redirect_to('/')
+          end
+        end
 
-      it 'does not display a flash message if state is valid' do
-        get :callback, params: { state: login_state }
+        context 'without a tampered header' do
+          it_behaves_like 'a valid redirect to redirect_url'
+        end
+
+        context 'with a tampered HOST header' do
+          before do
+            request.headers['HOST'] = 'http://this.is.not.my.host'
+          end
+
+          it_behaves_like 'a valid redirect to redirect_url'
+        end
+
+        context 'with a tampered X-Forwarded-Host header' do
+          before do
+            request.headers['X-Forwarded-Host'] = 'http://this.is.not.my.host'
+          end
+
+          it_behaves_like 'a valid redirect to redirect_url'
+        end
+
+        it 'does not display a flash message' do
+          get :callback, params: { state: login_state }
 
-        expect(controller).to set_flash[:alert].to(nil)
+          expect(controller).to set_flash[:alert].to(nil)
+        end
       end
     end
 

ee/spec/lib/gitlab/geo/oauth/login_state_spec.rb

@@ -3,9 +3,14 @@
 require 'spec_helper'
 
 describe Gitlab::Geo::Oauth::LoginState do
-  let(:salt) { '100d8cbd1750a2bb' }
-  let(:hmac) { '62fdcface89baab582f33de6672f10499974c28b5cc269795c4830b8b3ab06be' }
-  let(:oauth_return_to) { 'http://fake-secondary.com:3000/project/test' }
+  let(:salt) { 'b9653b6aa2ff6b54' }
+  let(:token) { 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJkYXRhIjp7InJldHVybl90byI6Ii9wcm9qZWN0L3Rlc3Q_Zm9vPWJhciN6b28ifSwianRpIjoiODdjZDQ2M2MtOTgyNC00ZjliLWI5NDMtOGFkMjJmY2E2MmZhIiwiaWF0IjoxNTQ5ODI1MjAwLCJuYmYiOjE1NDk4MjUxOTUsImV4cCI6MTU0OTgyNTI2MH0.qZE6kuoeW6BK1URuIl8l8MiCfGjtTTXixVdMCE80gVA' }
+  let(:return_to) { 'http://fake-secondary.com:3000/project/test?foo=bar#zoo' }
+  let(:timestamp) { Time.utc(2019, 2, 10, 19, 0, 0) }
+
+  around do |example|
+    Timecop.freeze(timestamp) { example.run }
+  end
 
   before do
     allow(Gitlab::Application.secrets).to receive(:secret_key_base)
@@ -22,7 +27,7 @@ describe Gitlab::Geo::Oauth::LoginState do
     end
 
     it 'returns a valid instance when state is valid' do
-      expect(described_class.from_state("#{salt}:#{hmac}:#{oauth_return_to}")).to be_valid
+      expect(described_class.from_state("#{salt}:#{token}:#{return_to}")).to be_valid
     end
   end
 
@@ -39,32 +44,42 @@ describe Gitlab::Geo::Oauth::LoginState do
       expect(subject.valid?).to eq false
     end
 
-    it 'returns false when hmac is nil' do
-      subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: nil)
+    it 'returns false when token is nil' do
+      subject = described_class.new(return_to: return_to, salt: salt, token: nil)
 
       expect(subject.valid?).to eq false
     end
 
-    it 'returns false when hmac is empty' do
-      subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: '')
+    it 'returns false when token is empty' do
+      subject = described_class.new(return_to: return_to, salt: salt, token: '')
 
       expect(subject.valid?).to eq false
     end
 
     it 'returns false when salt not match' do
-      subject = described_class.new(return_to: oauth_return_to, salt: 'salt', hmac: hmac)
+      subject = described_class.new(return_to: return_to, salt: 'invalid-salt', token: token)
 
       expect(subject.valid?).to eq(false)
     end
 
-    it 'returns false when hmac does not match' do
-      subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: 'hmac')
+    it 'returns false when token does not match' do
+      subject = described_class.new(return_to: return_to, salt: salt, token: 'invalid-token')
 
       expect(subject.valid?).to eq(false)
     end
 
-    it 'returns true when hmac matches' do
-      subject = described_class.new(return_to: oauth_return_to, salt: salt, hmac: hmac)
+    it "returns false when token's expired" do
+      subject = described_class.new(return_to: return_to, salt: salt, token: token)
+
+      # Needs to be at least 120 seconds, because the default expiry is
+      # 60 seconds with an additional 60 second leeway.
+      Timecop.freeze(timestamp + 125) do
+        expect(subject.valid?).to eq(false)
+      end
+    end
+
+    it 'returns true when token matches' do
+      subject = described_class.new(return_to: return_to, salt: salt, token: token)
 
       expect(subject.valid?).to eq(true)
     end
@@ -77,14 +92,34 @@ describe Gitlab::Geo::Oauth::LoginState do
       expect { subject.encode }.not_to raise_error
     end
 
-    it 'returns a string with salt, hmac, and return_to colon separated' do
-      subject = described_class.new(return_to: oauth_return_to)
+    it 'returns a string with salt, token, and return_to colon separated' do
+      subject = described_class.new(return_to: return_to)
 
-      salt, hmac, return_to = subject.encode.split(':', 3)
+      salt, token, return_to = subject.encode.split(':', 3)
 
       expect(salt).not_to be_blank
-      expect(hmac).not_to be_blank
-      expect(return_to).to eq oauth_return_to
+      expect(token).not_to be_blank
+      expect(return_to).to eq return_to
+    end
+  end
+
+  describe '#return_to' do
+    it 'returns nil when return_to is nil' do
+      subject = described_class.new(return_to: nil)
+
+      expect(subject.return_to).to be_nil
+    end
+
+    it 'returns an empty string when return_to is empty' do
+      subject = described_class.new(return_to: '')
+
+      expect(subject.return_to).to eq('')
+    end
+
+    it 'returns the full path of the return_to URL' do
+      subject = described_class.new(return_to: return_to)
+
+      expect(subject.return_to).to eq('/project/test?foo=bar#zoo')
     end
   end
 end