Commit 988dad46 authored by Grzegorz Bizon's avatar Grzegorz Bizon

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

Fix vulnerability that leaks private labels and milestones

## Summary

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

## Fix

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

## Further work

`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
parents aea97991 55df95c3
...@@ -4,6 +4,7 @@ v 8.8.0 (unreleased) ...@@ -4,6 +4,7 @@ v 8.8.0 (unreleased)
v 8.7.0 (unreleased) v 8.7.0 (unreleased)
- Gitlab::GitAccess and Gitlab::GitAccessWiki are now instrumented - Gitlab::GitAccess and Gitlab::GitAccessWiki are now instrumented
- Fix vulnerability that made it possible to gain access to private labels and milestones
- The number of InfluxDB points stored per UDP packet can now be configured - The number of InfluxDB points stored per UDP packet can now be configured
- Fix error when cross-project label reference used with non-existent project - Fix error when cross-project label reference used with non-existent project
- Transactions for /internal/allowed now have an "action" tag set - Transactions for /internal/allowed now have an "action" tag set
......
...@@ -37,8 +37,9 @@ class IssuableBaseService < BaseService ...@@ -37,8 +37,9 @@ class IssuableBaseService < BaseService
end end
def filter_params(issuable_ability_name = :issue) def filter_params(issuable_ability_name = :issue)
params[:assignee_id] = "" if params[:assignee_id] == IssuableFinder::NONE filter_assignee
params[:milestone_id] = "" if params[:milestone_id] == IssuableFinder::NONE filter_milestone
filter_labels
ability = :"admin_#{issuable_ability_name}" ability = :"admin_#{issuable_ability_name}"
...@@ -49,6 +50,29 @@ class IssuableBaseService < BaseService ...@@ -49,6 +50,29 @@ class IssuableBaseService < BaseService
end end
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) def update(issuable)
change_state(issuable) change_state(issuable)
filter_params filter_params
......
...@@ -100,7 +100,7 @@ describe Issues::BulkUpdateService, services: true do ...@@ -100,7 +100,7 @@ describe Issues::BulkUpdateService, services: true do
describe :update_milestone do describe :update_milestone do
before do before do
@milestone = create :milestone @milestone = create(:milestone, project: @project)
@params = { @params = {
issues_ids: [issue.id], issues_ids: [issue.id],
milestone_id: @milestone.id milestone_id: @milestone.id
......
...@@ -3,40 +3,75 @@ require 'spec_helper' ...@@ -3,40 +3,75 @@ require 'spec_helper'
describe Issues::CreateService, services: true do describe Issues::CreateService, services: true do
let(:project) { create(:empty_project) } let(:project) { create(:empty_project) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:assignee) { create(:user) }
describe :execute do describe '#execute' do
context 'valid params' 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_pair(:label, project: project) }
before do before do
project.team << [user, :master] project.team << [user, :master]
project.team << [assignee, :master] project.team << [assignee, :master]
end
opts = { let(:opts) do
title: 'Awesome issue', { title: 'Awesome issue',
description: 'please fix', description: 'please fix',
assignee: assignee assignee: assignee,
} label_ids: labels.map(&:id),
milestone_id: milestone.id }
@issue = Issues::CreateService.new(project, user, opts).execute
end end
it { expect(@issue).to be_valid } it { expect(issue).to be_valid }
it { expect(@issue.title).to eq('Awesome issue') } it { expect(issue.title).to eq('Awesome issue') }
it { expect(@issue.assignee).to eq assignee } it { expect(issue.assignee).to eq assignee }
it { expect(issue.labels).to match_array labels }
it { expect(issue.milestone).to eq milestone }
it 'creates a pending todo for new assignee' do it 'creates a pending todo for new assignee' do
attributes = { attributes = {
project: project, project: project,
author: user, author: user,
user: assignee, user: assignee,
target_id: @issue.id, target_id: issue.id,
target_type: @issue.class.name, target_type: issue.class.name,
action: Todo::ASSIGNED, action: Todo::ASSIGNED,
state: :pending state: :pending
} }
expect(Todo.where(attributes).count).to eq 1 expect(Todo.where(attributes).count).to eq 1
end end
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 end
end end
...@@ -4,10 +4,15 @@ describe Issues::UpdateService, services: true do ...@@ -4,10 +4,15 @@ describe Issues::UpdateService, services: true do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:user2) { create(:user) } let(:user2) { create(:user) }
let(:user3) { create(:user) } let(:user3) { create(:user) }
let(:issue) { create(:issue, title: 'Old title', assignee_id: user3.id) } let(:project) { create(:empty_project) }
let(:label) { create(:label) } let(:label) { create(:label, project: project) }
let(:label2) { create(:label) } let(:label2) { create(:label) }
let(:project) { issue.project }
let(:issue) do
create(:issue, title: 'Old title',
assignee_id: user3.id,
project: project)
end
before do before do
project.team << [user, :master] project.team << [user, :master]
......
require 'spec_helper' require 'spec_helper'
describe MergeRequests::UpdateService, services: true do describe MergeRequests::UpdateService, services: true do
let(:project) { create(:project) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:user2) { create(:user) } let(:user2) { create(:user) }
let(:user3) { create(:user) } let(:user3) { create(:user) }
let(:merge_request) { create(:merge_request, :simple, title: 'Old title', assignee_id: user3.id) } let(:label) { create(:label, project: project) }
let(:project) { merge_request.project }
let(:label) { create(:label) }
let(:label2) { create(:label) } 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 before do
project.team << [user, :master] project.team << [user, :master]
project.team << [user2, :developer] project.team << [user2, :developer]
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment