Merge branch 'fix/id-claim-import-issue' into 'master'

Prevent claiming associated model IDs via import

On the import side, we should be careful not to use any IDs as part of the JSON file that could have been manipulated.

Part of https://gitlab.com/gitlab-org/gitlab-ce/issues/20821

Things we already do (__before__ this fix):

1. Remove all primary keys
1. **Always** reassign some of the foreign keys, such as ALL project IDs and user IDs (so it would be difficult to impersonate or try to gain access to another project)
1. Ignore/reject attributes that do not exist in the model
1. If someone reassigns a foreign key `submodel_id`, and that object has another json as the submodel, the new submodel will reassign the `submodel_id` to the newly created submodel ID.

Things we should do:

1. Remove/nullify any other foreign keys that we don't reassign (checked this, and there aren't many, fortunately. In fact, I don't think much harm can be done at all - at the moment).

See merge request !1985
Signed-off-by: Rémy Coutable <remy@rymai.me>

CHANGELOG

@@ -10,6 +10,7 @@ v 8.12.4 (unreleased)
   - Skip wiki creation when GitHub project has wiki enabled. !6665
   - Restrict failed login attempts for users with 2FA enabled. !6668
   - Fix failed project deletion when feature visibility set to private. !6688
+  - Prevent claiming associated model IDs via import.
 
 v 8.12.3
   - Update Gitlab Shell to support low IO priority for storage moves

lib/gitlab/import_export/attribute_cleaner.rb

+module Gitlab
+  module ImportExport
+    class AttributeCleaner
+      ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES
+
+      def self.clean!(relation_hash:)
+        relation_hash.reject! do |key, _value|
+          key.end_with?('_id') && !ALLOWED_REFERENCES.include?(key)
+        end
+      end
+    end
+  end
+end

lib/gitlab/import_export/project_tree_restorer.rb

@@ -110,9 +110,10 @@ module Gitlab
       def create_relation(relation, relation_hash_list)
         relation_array = [relation_hash_list].flatten.map do |relation_hash|
           Gitlab::ImportExport::RelationFactory.create(relation_sym: relation.to_sym,
-                                                       relation_hash: relation_hash.merge('project_id' => restored_project.id),
+                                                       relation_hash: relation_hash,
                                                        members_mapper: members_mapper,
-                                                       user: @user)
+                                                       user: @user,
+                                                       project_id: restored_project.id)
         end
 
         relation_hash_list.is_a?(Array) ? relation_array : relation_array.first

lib/gitlab/import_export/relation_factory.rb

@@ -13,6 +13,8 @@ module Gitlab
 
       USER_REFERENCES = %w[author_id assignee_id updated_by_id user_id].freeze
 
+      PROJECT_REFERENCES = %w[project_id source_project_id gl_project_id target_project_id].freeze
+
       BUILD_MODELS = %w[Ci::Build commit_status].freeze
 
       IMPORTED_OBJECT_MAX_RETRIES = 5.freeze
@@ -25,9 +27,9 @@ module Gitlab
         new(*args).create
       end
 
-      def initialize(relation_sym:, relation_hash:, members_mapper:, user:)
+      def initialize(relation_sym:, relation_hash:, members_mapper:, user:, project_id:)
         @relation_name = OVERRIDES[relation_sym] || relation_sym
-        @relation_hash = relation_hash.except('id', 'noteable_id')
+        @relation_hash = relation_hash.except('id', 'noteable_id').merge('project_id' => project_id)
         @members_mapper = members_mapper
         @user = user
         @imported_object_retries = 0
@@ -153,7 +155,11 @@ module Gitlab
       end
 
       def parsed_relation_hash
-        @parsed_relation_hash ||= @relation_hash.reject { |k, _v| !relation_class.attribute_method?(k) }
+        @parsed_relation_hash ||= begin
+          Gitlab::ImportExport::AttributeCleaner.clean!(relation_hash: @relation_hash)
+
+          @relation_hash.reject { |k, _v| !relation_class.attribute_method?(k) }
+        end
       end
 
       def set_st_diffs

spec/lib/gitlab/import_export/attribute_cleaner_spec.rb

+require 'spec_helper'
+
+describe Gitlab::ImportExport::AttributeCleaner, lib: true do
+  let(:unsafe_hash) do
+    {
+      'service_id' => 99,
+      'moved_to_id' => 99,
+      'namespace_id' => 99,
+      'ci_id' => 99,
+      'random_project_id' => 99,
+      'random_id' => 99,
+      'milestone_id' => 99,
+      'project_id' => 99,
+      'user_id' => 99,
+      'random_id_in_the_middle' => 99,
+      'notid' => 99
+    }
+  end
+
+  let(:post_safe_hash) do
+    {
+      'project_id' => 99,
+      'user_id' => 99,
+      'random_id_in_the_middle' => 99,
+      'notid' => 99
+    }
+  end
+
+  it 'removes unwanted attributes from the hash' do
+    described_class.clean!(relation_hash: unsafe_hash)
+
+    expect(unsafe_hash).to eq(post_safe_hash)
+  end
+end

spec/lib/gitlab/import_export/relation_factory_spec.rb

+require 'spec_helper'
+
+describe Gitlab::ImportExport::RelationFactory, lib: true do
+  let(:project) { create(:empty_project) }
+  let(:members_mapper) { double('members_mapper').as_null_object }
+  let(:user) { create(:user) }
+  let(:created_object) do
+    described_class.create(relation_sym: relation_sym,
+                           relation_hash: relation_hash,
+                           members_mapper: members_mapper,
+                           user: user,
+                           project_id: project.id)
+  end
+
+  context 'hook object' do
+    let(:relation_sym) { :hooks }
+    let(:id) { 999 }
+    let(:service_id) { 99 }
+    let(:original_project_id) { 8 }
+    let(:token) { 'secret' }
+
+    let(:relation_hash) do
+      {
+        'id' => id,
+        'url' => 'https://example.json',
+        'project_id' => original_project_id,
+        'created_at' => '2016-08-12T09:41:03.462Z',
+        'updated_at' => '2016-08-12T09:41:03.462Z',
+        'service_id' => service_id,
+        'push_events' => true,
+        'issues_events' => false,
+        'merge_requests_events' => true,
+        'tag_push_events' => false,
+        'note_events' => true,
+        'enable_ssl_verification' => true,
+        'build_events' => false,
+        'wiki_page_events' => true,
+        'token' => token
+      }
+    end
+
+    it 'does not have the original ID' do
+      expect(created_object.id).not_to eq(id)
+    end
+
+    it 'does not have the original service_id' do
+      expect(created_object.service_id).not_to eq(service_id)
+    end
+
+    it 'does not have the original project_id' do
+      expect(created_object.project_id).not_to eq(original_project_id)
+    end
+
+    it 'has the new project_id' do
+      expect(created_object.project_id).to eq(project.id)
+    end
+
+    it 'has a token' do
+      expect(created_object.token).to eq(token)
+    end
+
+    context 'original service exists' do
+      let(:service_id) { Service.create(project: project).id }
+
+      it 'does not have the original service_id' do
+        expect(created_object.service_id).not_to eq(service_id)
+      end
+    end
+  end
+
+  # Mocks an ActiveRecordish object with the dodgy columns
+  class FooModel
+    include ActiveModel::Model
+
+    def initialize(params)
+      params.each { |key, value| send("#{key}=", value) }
+    end
+
+    def values
+      instance_variables.map { |ivar| instance_variable_get(ivar) }
+    end
+  end
+
+  # `project_id`, `described_class.USER_REFERENCES`, noteable_id, target_id, and some project IDs are already
+  # re-assigned by described_class.
+  context 'Potentially hazardous foreign keys' do
+    let(:relation_sym) { :hazardous_foo_model }
+    let(:relation_hash) do
+      {
+        'service_id' => 99,
+        'moved_to_id' => 99,
+        'namespace_id' => 99,
+        'ci_id' => 99,
+        'random_project_id' => 99,
+        'random_id' => 99,
+        'milestone_id' => 99,
+        'project_id' => 99,
+        'user_id' => 99,
+      }
+    end
+
+    class HazardousFooModel < FooModel
+      attr_accessor :service_id, :moved_to_id, :namespace_id, :ci_id, :random_project_id, :random_id, :milestone_id, :project_id
+    end
+
+    it 'does not preserve any foreign key IDs' do
+      expect(created_object.values).not_to include(99)
+    end
+  end
+
+  context 'Project references' do
+    let(:relation_sym) { :project_foo_model }
+    let(:relation_hash) do
+      Gitlab::ImportExport::RelationFactory::PROJECT_REFERENCES.map { |ref| { ref => 99 } }.inject(:merge)
+    end
+
+    class ProjectFooModel < FooModel
+      attr_accessor(*Gitlab::ImportExport::RelationFactory::PROJECT_REFERENCES)
+    end
+
+    it 'does not preserve any project foreign key IDs' do
+      expect(created_object.values).not_to include(99)
+    end
+  end
+end