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
This commit is contained in:
Douwe Maan 2016-09-30 07:31:02 +00:00
commit 8a866bfce3
5 changed files with 184 additions and 5 deletions

View file

@ -0,0 +1,13 @@
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

View file

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

View file

@ -13,6 +13,8 @@ module Gitlab
USER_REFERENCES = %w[author_id assignee_id updated_by_id user_id].freeze 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 BUILD_MODELS = %w[Ci::Build commit_status].freeze
IMPORTED_OBJECT_MAX_RETRIES = 5.freeze IMPORTED_OBJECT_MAX_RETRIES = 5.freeze
@ -25,9 +27,9 @@ module Gitlab
new(*args).create new(*args).create
end 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_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 @members_mapper = members_mapper
@user = user @user = user
@imported_object_retries = 0 @imported_object_retries = 0
@ -153,7 +155,11 @@ module Gitlab
end end
def parsed_relation_hash 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 end
def set_st_diffs def set_st_diffs

View file

@ -0,0 +1,34 @@
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

View file

@ -0,0 +1,125 @@
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