summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMayra Cabrera <mcabrera@gitlab.com>2018-05-21 21:05:50 +0000
committerMayra Cabrera <mcabrera@gitlab.com>2018-05-25 10:44:29 -0500
commit5ebe2ebdc1f6b16dfba63cc78870359c49c6bf0d (patch)
tree6ee99edbeadfd9ccc4947444df1f1eb23788fa0b
parent27afc05f7d781487f7c05f963e56be3b414a1e8d (diff)
downloadgitlab-ce-5ebe2ebdc1f6b16dfba63cc78870359c49c6bf0d.tar.gz
Merge branch 'security-fj-import-export-assignment-10-8' into 'security-10-8'
[10.8] Arbitrary assignment of Project fields using "Import project" from a Gitlab tar.gz See merge request gitlab/gitlabhq!2390
-rw-r--r--changelogs/unreleased/security-fj-import-export-assignment.yml5
-rw-r--r--lib/gitlab/import_export/attribute_cleaner.rb11
-rw-r--r--lib/gitlab/import_export/attributes_finder.rb4
-rw-r--r--lib/gitlab/import_export/import_export.yml2
-rw-r--r--lib/gitlab/import_export/project_tree_restorer.rb23
-rw-r--r--lib/gitlab/import_export/reader.rb2
-rw-r--r--lib/gitlab/import_export/relation_factory.rb10
-rw-r--r--spec/lib/gitlab/import_export/attribute_cleaner_spec.rb29
-rw-r--r--spec/lib/gitlab/import_export/project.json2
-rw-r--r--spec/lib/gitlab/import_export/project.light.json2
-rw-r--r--spec/lib/gitlab/import_export/project_tree_restorer_spec.rb9
-rw-r--r--spec/lib/gitlab/import_export/relation_factory_spec.rb12
12 files changed, 93 insertions, 18 deletions
diff --git a/changelogs/unreleased/security-fj-import-export-assignment.yml b/changelogs/unreleased/security-fj-import-export-assignment.yml
new file mode 100644
index 00000000000..4bfd71d431a
--- /dev/null
+++ b/changelogs/unreleased/security-fj-import-export-assignment.yml
@@ -0,0 +1,5 @@
+---
+title: Fixed bug that allowed importing arbitrary project attributes
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb
index 34169319b26..7c9fc5c15bb 100644
--- a/lib/gitlab/import_export/attribute_cleaner.rb
+++ b/lib/gitlab/import_export/attribute_cleaner.rb
@@ -7,14 +7,15 @@ module Gitlab
new(*args).clean
end
- def initialize(relation_hash:, relation_class:)
+ def initialize(relation_hash:, relation_class:, excluded_keys: [])
@relation_hash = relation_hash
@relation_class = relation_class
+ @excluded_keys = excluded_keys
end
def clean
@relation_hash.reject do |key, _value|
- prohibited_key?(key) || !@relation_class.attribute_method?(key)
+ prohibited_key?(key) || !@relation_class.attribute_method?(key) || excluded_key?(key)
end.except('id')
end
@@ -23,6 +24,12 @@ module Gitlab
def prohibited_key?(key)
key.end_with?('_id') && !ALLOWED_REFERENCES.include?(key)
end
+
+ def excluded_key?(key)
+ return false if @excluded_keys.empty?
+
+ @excluded_keys.include?(key)
+ end
end
end
end
diff --git a/lib/gitlab/import_export/attributes_finder.rb b/lib/gitlab/import_export/attributes_finder.rb
index 56042ddecbf..0c8fda07294 100644
--- a/lib/gitlab/import_export/attributes_finder.rb
+++ b/lib/gitlab/import_export/attributes_finder.rb
@@ -32,6 +32,10 @@ module Gitlab
@methods[key].nil? ? {} : { methods: @methods[key] }
end
+ def find_excluded_keys(klass_name)
+ @excluded_attributes[klass_name.to_sym]&.map(&:to_s) || []
+ end
+
private
def find_attributes_only(value)
diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml
index 21ac7f7e0b6..36c7534cd7a 100644
--- a/lib/gitlab/import_export/import_export.yml
+++ b/lib/gitlab/import_export/import_export.yml
@@ -98,8 +98,6 @@ excluded_attributes:
- :import_jid
- :created_at
- :updated_at
- - :import_jid
- - :import_jid
- :id
- :star_count
- :last_activity_at
diff --git a/lib/gitlab/import_export/project_tree_restorer.rb b/lib/gitlab/import_export/project_tree_restorer.rb
index d5590dde40f..4eb67fbe11e 100644
--- a/lib/gitlab/import_export/project_tree_restorer.rb
+++ b/lib/gitlab/import_export/project_tree_restorer.rb
@@ -88,16 +88,18 @@ module Gitlab
end
def project_params
- @project_params ||= json_params.merge(override_params)
+ @project_params ||= begin
+ attrs = json_params.merge(override_params)
+
+ # Cleaning all imported and overridden params
+ Gitlab::ImportExport::AttributeCleaner.clean(relation_hash: attrs,
+ relation_class: Project,
+ excluded_keys: excluded_keys_for_relation(:project))
+ end
end
def override_params
- return {} unless params = @project.import_data&.data&.fetch('override_params', nil)
-
- @override_params ||= params.select do |key, _value|
- Project.column_names.include?(key.to_s) &&
- !reader.project_tree[:except].include?(key.to_sym)
- end
+ @override_params ||= @project.import_data&.data&.fetch('override_params', nil) || {}
end
def json_params
@@ -171,7 +173,8 @@ module Gitlab
relation_hash: parsed_relation_hash(relation_hash, relation.to_sym),
members_mapper: members_mapper,
user: @user,
- project: @restored_project)
+ project: @restored_project,
+ excluded_keys: excluded_keys_for_relation(relation))
end.compact
relation_hash_list.is_a?(Array) ? relation_array : relation_array.first
@@ -192,6 +195,10 @@ module Gitlab
def reader
@reader ||= Gitlab::ImportExport::Reader.new(shared: @shared)
end
+
+ def excluded_keys_for_relation(relation)
+ @reader.attributes_finder.find_excluded_keys(relation)
+ end
end
end
end
diff --git a/lib/gitlab/import_export/reader.rb b/lib/gitlab/import_export/reader.rb
index eb7f5120592..e621c40fc7a 100644
--- a/lib/gitlab/import_export/reader.rb
+++ b/lib/gitlab/import_export/reader.rb
@@ -1,7 +1,7 @@
module Gitlab
module ImportExport
class Reader
- attr_reader :tree
+ attr_reader :tree, :attributes_finder
def initialize(shared:)
@shared = shared
diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb
index e3e9f156fb4..301719406e4 100644
--- a/lib/gitlab/import_export/relation_factory.rb
+++ b/lib/gitlab/import_export/relation_factory.rb
@@ -36,13 +36,21 @@ module Gitlab
new(*args).create
end
- def initialize(relation_sym:, relation_hash:, members_mapper:, user:, project:)
+ def initialize(relation_sym:, relation_hash:, members_mapper:, user:, project:, excluded_keys: [])
@relation_name = OVERRIDES[relation_sym] || relation_sym
@relation_hash = relation_hash.except('noteable_id')
@members_mapper = members_mapper
@user = user
@project = project
@imported_object_retries = 0
+
+ # Remove excluded keys from relation_hash
+ # We don't do this in the parsed_relation_hash because of the 'transformed attributes'
+ # For example, MergeRequestDiffFiles exports its diff attribute as utf8_diff. Then,
+ # in the create method that attribute is renamed to diff. And because diff is an excluded key,
+ # if we clean the excluded keys in the parsed_relation_hash, it will be removed
+ # from the object attributes and the export will fail.
+ @relation_hash.except!(*excluded_keys)
end
# Creates an object from an actual model with name "relation_sym" with params from
diff --git a/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb b/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb
index cd5a1b2982b..536cc359d39 100644
--- a/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb
+++ b/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb
@@ -15,7 +15,10 @@ describe Gitlab::ImportExport::AttributeCleaner do
'project_id' => 99,
'user_id' => 99,
'random_id_in_the_middle' => 99,
- 'notid' => 99
+ 'notid' => 99,
+ 'import_source' => 'whatever',
+ 'import_type' => 'whatever',
+ 'non_existent_attr' => 'whatever'
}
end
@@ -28,10 +31,30 @@ describe Gitlab::ImportExport::AttributeCleaner do
}
end
+ let(:excluded_keys) { %w[import_source import_type] }
+
+ subject { described_class.clean(relation_hash: unsafe_hash, relation_class: relation_class, excluded_keys: excluded_keys) }
+
+ before do
+ allow(relation_class).to receive(:attribute_method?).and_return(true)
+ allow(relation_class).to receive(:attribute_method?).with('non_existent_attr').and_return(false)
+ end
+
it 'removes unwanted attributes from the hash' do
- # allow(relation_class).to receive(:attribute_method?).and_return(true)
+ expect(subject).to eq(post_safe_hash)
+ end
+
+ it 'removes attributes not present in relation_class' do
+ expect(subject.keys).not_to include 'non_existent_attr'
+ end
+
+ it 'removes excluded keys from the hash' do
+ expect(subject.keys).not_to include excluded_keys
+ end
+
+ it 'does not remove excluded key if not listed' do
parsed_hash = described_class.clean(relation_hash: unsafe_hash, relation_class: relation_class)
- expect(parsed_hash).to eq(post_safe_hash)
+ expect(parsed_hash.keys).to eq post_safe_hash.keys + excluded_keys
end
end
diff --git a/spec/lib/gitlab/import_export/project.json b/spec/lib/gitlab/import_export/project.json
index 6d63749296e..4d0cc1f196e 100644
--- a/spec/lib/gitlab/import_export/project.json
+++ b/spec/lib/gitlab/import_export/project.json
@@ -1,5 +1,7 @@
{
"description": "Nisi et repellendus ut enim quo accusamus vel magnam.",
+ "import_type": "gitlab_project",
+ "creator_id": 123,
"visibility_level": 10,
"archived": false,
"labels": [
diff --git a/spec/lib/gitlab/import_export/project.light.json b/spec/lib/gitlab/import_export/project.light.json
index 5dbf0ed289b..c13cf4a0507 100644
--- a/spec/lib/gitlab/import_export/project.light.json
+++ b/spec/lib/gitlab/import_export/project.light.json
@@ -1,5 +1,7 @@
{
"description": "Nisi et repellendus ut enim quo accusamus vel magnam.",
+ "import_type": "gitlab_project",
+ "creator_id": 123,
"visibility_level": 10,
"archived": false,
"milestones": [
diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
index 13a8c9adcee..68ddc947e02 100644
--- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
@@ -23,6 +23,10 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch)
project_tree_restorer = described_class.new(user: @user, shared: @shared, project: @project)
+
+ expect(Gitlab::ImportExport::RelationFactory).to receive(:create).with(hash_including(excluded_keys: ['whatever'])).and_call_original.at_least(:once)
+ allow(project_tree_restorer).to receive(:excluded_keys_for_relation).and_return(['whatever'])
+
@restored_project_json = project_tree_restorer.restore
end
end
@@ -248,6 +252,11 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
expect(labels.where(type: "ProjectLabel").count).to eq(results.fetch(:first_issue_labels, 0))
expect(labels.where(type: "ProjectLabel").where.not(group_id: nil).count).to eq(0)
end
+
+ it 'does not set params that are excluded from import_export settings' do
+ expect(project.import_type).to be_nil
+ expect(project.creator_id).not_to eq 123
+ end
end
shared_examples 'restores group correctly' do |**results|
diff --git a/spec/lib/gitlab/import_export/relation_factory_spec.rb b/spec/lib/gitlab/import_export/relation_factory_spec.rb
index 5c61a5a2044..5f0dfd64b15 100644
--- a/spec/lib/gitlab/import_export/relation_factory_spec.rb
+++ b/spec/lib/gitlab/import_export/relation_factory_spec.rb
@@ -4,12 +4,14 @@ describe Gitlab::ImportExport::RelationFactory do
let(:project) { create(:project) }
let(:members_mapper) { double('members_mapper').as_null_object }
let(:user) { create(:admin) }
+ let(:excluded_keys) { [] }
let(:created_object) do
described_class.create(relation_sym: relation_sym,
relation_hash: relation_hash,
members_mapper: members_mapper,
user: user,
- project: project)
+ project: project,
+ excluded_keys: excluded_keys)
end
context 'hook object' do
@@ -67,6 +69,14 @@ describe Gitlab::ImportExport::RelationFactory do
expect(created_object.service_id).not_to eq(service_id)
end
end
+
+ context 'excluded attributes' do
+ let(:excluded_keys) { %w[url] }
+
+ it 'are removed from the imported object' do
+ expect(created_object.url).to be_nil
+ end
+ end
end
# Mocks an ActiveRecordish object with the dodgy columns