Merge branch 'fix/private-labels-permissions' into 'master'

Fix vulnerability that leaks private labels and milestones

This fixes vulnerability that leaks information about private labels and milestones because of  insecure direct object reference in issueable create service.
This affects merge requests and issues.

See https://gitlab.com/gitlab-org/gitlab-ce/issues/15439

This MR introduces additional check that rejects labels and milestone that does not belong to the same project issue/merg request does.

`IssuableBaseService` may benefit from encapsulating filters in separate class/module, which then may improve coherency in this class.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/15439

See merge request !1954

Signed-off-by: Rémy Coutable <remy@rymai.me>

6 files changed, 97 insertions, 17 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 461a0dd2776..6d7efadefff 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.2.5
   - Fix a window.opener bug that could lead to XSS and open redirects
+  - Fix vulnerability that leaks private labels and milestones
   - Prevent privilege escalation via "impersonate" feature
   - Prevent users from deleting Webhooks via API they do not own
   - Prevent information disclosure via snippet API
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 15b3825f96a..fcec35b03b7 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -28,6 +28,10 @@ class IssuableBaseService < BaseService
   end
 
   def filter_params(issuable_ability_name = :issue)
+    filter_assignee
+    filter_milestone
+    filter_labels
+
     ability = :"admin_#{issuable_ability_name}"
 
     unless can?(current_user, ability, project)
@@ -36,4 +40,27 @@ class IssuableBaseService < BaseService
       params.delete(:assignee_id)
     end
   end
+
+  def filter_assignee
+    if params[:assignee_id] == IssuableFinder::NONE
+      params[:assignee_id] = ''
+    end
+  end
+
+  def filter_milestone
+    milestone_id = params[:milestone_id]
+    return unless milestone_id
+
+    if milestone_id == IssuableFinder::NONE ||
+        project.milestones.find_by(id: milestone_id).nil?
+      params[:milestone_id] = ''
+    end
+  end
+
+  def filter_labels
+    return if params[:label_ids].to_a.empty?
+
+    params[:label_ids] =
+      project.labels.where(id: params[:label_ids]).pluck(:id)
+  end
 end
diff --git a/spec/services/issues/bulk_update_service_spec.rb b/spec/services/issues/bulk_update_service_spec.rb
index 4c62fbafd73..93ed52c65cc 100644
--- a/spec/services/issues/bulk_update_service_spec.rb
+++ b/spec/services/issues/bulk_update_service_spec.rb
@@ -100,7 +100,7 @@ describe Issues::BulkUpdateService do
   describe :update_milestone do
 
     before do
-      @milestone = create :milestone
+      @milestone = create(:milestone, project: @project)
       @params = {
         issues_ids: [issue.id],
         milestone_id: @milestone.id
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index 7f1ebcb3198..5a811ce0ace 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -4,20 +4,60 @@ describe Issues::CreateService do
   let(:project) { create(:empty_project) }
   let(:user) { create(:user) }
 
-  describe :execute do
-    context "valid params" do
+  describe '#execute' do
+    let(:issue) { described_class.new(project, user, opts).execute }
+
+    context 'when params are valid' do
+      let(:assignee) { create(:user) }
+      let(:milestone) { create(:milestone, project: project) }
+      let(:labels) { [create(:label, title: 'foo', project: project), create(:label, title: 'bar', project: project)] }
+
       before do
         project.team << [user, :master]
-        opts = {
-          title: 'Awesome issue',
-          description: 'please fix'
-        }
+        project.team << [assignee, :master]
+      end
 
-        @issue = Issues::CreateService.new(project, user, opts).execute
+      let(:opts) do
+        { title: 'Awesome issue',
+          description: 'please fix',
+          assignee: assignee,
+          label_ids: labels.map(&:id),
+          milestone_id: milestone.id }
       end
 
-      it { expect(@issue).to be_valid }
-      it { expect(@issue.title).to eq('Awesome issue') }
+      it { expect(issue).to be_valid }
+      it { expect(issue.title).to eq('Awesome issue') }
+      it { expect(issue.assignee).to eq assignee }
+      it { expect(issue.labels).to match_array labels }
+      it { expect(issue.milestone).to eq milestone }
+
+      context 'when label belongs to different project' do
+        let(:label) { create(:label) }
+
+        let(:opts) do
+          { title: 'Title',
+            description: 'Description',
+            label_ids: [label.id] }
+        end
+
+        it 'does not assign label'do
+          expect(issue.labels).to_not include label
+        end
+      end
+
+      context 'when milestone belongs to different project' do
+        let(:milestone) { create(:milestone) }
+
+        let(:opts) do
+          { title: 'Title',
+            description: 'Description',
+            milestone_id: milestone.id }
+        end
+
+        it 'does not assign milestone' do
+          expect(issue.milestone).to_not eq milestone
+        end
+      end
     end
   end
 end
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index f55527ee9a3..55c04b6e42c 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -4,9 +4,15 @@ describe Issues::UpdateService do
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
   let(:user3) { create(:user) }
-  let(:issue) { create(:issue, title: 'Old title', assignee_id: user3.id) }
-  let(:label) { create(:label) }
-  let(:project) { issue.project }
+  let(:project) { create(:empty_project) }
+  let(:label) { create(:label, project: project) }
+  let(:label2) { create(:label) }
+
+  let(:issue) do
+    create(:issue, title: 'Old title',
+                   assignee_id: user3.id,
+                   project: project)
+  end
 
   before do
     project.team << [user, :master]
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 2ed51d223b7..08aa4495d30 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -1,12 +1,18 @@
 require 'spec_helper'
 
-describe MergeRequests::UpdateService do
+describe MergeRequests::UpdateService, services: true do
+  let(:project) { create(:project) }
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
   let(:user3) { create(:user) }
-  let(:merge_request) { create(:merge_request, :simple, title: 'Old title', assignee_id: user3.id) }
-  let(:project) { merge_request.project }
-  let(:label) { create(:label) }
+  let(:label) { create(:label, project: project) }
+  let(:label2) { create(:label) }
+
+  let(:merge_request) do
+    create(:merge_request, :simple, title: 'Old title',
+                                    assignee_id: user3.id,
+                                    source_project: project)
+  end
 
   before do
     project.team << [user, :master]