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>

CHANGELOG

@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 v 8.3.9
   - Fix a window.opener bug that could lead to XSS and open redirects
   - Prevent XSS via custom issue tracker URL
+  - 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

app/services/issuable_base_service.rb

@@ -34,8 +34,9 @@ class IssuableBaseService < BaseService
   end
 
   def filter_params(issuable_ability_name = :issue)
-    params[:assignee_id]  = "" if params[:assignee_id] == IssuableFinder::NONE
-    params[:milestone_id] = "" if params[:milestone_id] == IssuableFinder::NONE
+    filter_assignee
+    filter_milestone
+    filter_labels
 
     ability = :"admin_#{issuable_ability_name}"
 
@@ -46,6 +47,29 @@ class IssuableBaseService < BaseService
     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
+
   def update(issuable)
     change_state(issuable)
     filter_params

spec/services/issues/bulk_update_service_spec.rb

@@ -100,7 +100,7 @@ describe Issues::BulkUpdateService, services: true do
   describe :update_milestone do
 
     before do
-      @milestone = create :milestone
+      @milestone = create(:milestone, project: @project)
       @params = {
         issues_ids: [issue.id],
         milestone_id: @milestone.id

spec/services/issues/create_service_spec.rb

@@ -4,20 +4,60 @@ describe Issues::CreateService, services: true 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

spec/services/issues/update_service_spec.rb

@@ -4,9 +4,15 @@ describe Issues::UpdateService, services: true 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]

spec/services/merge_requests/update_service_spec.rb

 require 'spec_helper'
 
 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]