Commit f349d939 authored by Grzegorz Bizon's avatar Grzegorz Bizon Committed by Rémy Coutable

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: default avatarRémy Coutable <remy@rymai.me>
parent 7fba61da
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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
......@@ -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]
......
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]
......
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