CVE-2016-9469: Permissions, Privileges, and Access Controls (CWE-264)
value: Multiple versions of GitLab expose a dangerous method to any authenticated user that could lead to the deletion of all Issue and MergeRequest objects on a GitLab instance. For GitLab instances with publicly available projects this vulnerability could be exploited by an unauthenticated user. A fix was included in versions 8.14.3, 8.13.8, and 8.12.11, which were released on December 5th 2016 at 3:59 PST. The GitLab versions vulnerable to this are 8.13.0, 8.13.0-ee, 8.13.1, 8.13.1-ee, 8.13.2, 8.13.2-ee, 8.13.3, 8.13.3-ee, 8.13.4, 8.13.4-ee, 8.13.5, 8.13.5-ee, 8.13.6, 8.13.6-ee, 8.13.7, 8.14.0, 8.14.0-ee, 8.14.1, 8.14.2, and 8.14.2-ee.
CWE description: Weaknesses in this category are related to the management of permissions, privileges, and other security features that are used to perform access control.
CVSS v3 base score: 8.2
Published: 2017-03-28T02:59Z
Repository: https://gitlab.com/gitlab-org/gitlab-ce
Language: Ruby
Files changed: app/finders/issuable_finder.rb, changelogs/unreleased/validate-state-param-when-filtering-issuables.yml, spec/finders/issues_finder_spec.rb
Commit date: 2016-12-06 12:26:48 +1100
Commit message: Merge branch 'issue_25064' into 'security'

Ensure state param has a valid value when filtering issuables.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/25064

This fix makes sure we only call safe methods on issuable when filtering by state.

See merge request !2038
Diff: diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 001c83ccb4..9560e9d518 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -5,11 +5,11 @@
 # Arguments:
 #   klass - actual class like Issue or MergeRequest
 #   current_user - which user use
 #   params:
 #     scope: 'created-by-me' or 'assigned-to-me' or 'all'
-#     state: 'open' or 'closed' or 'all'
+#     state: 'opened' or 'closed' or 'all'
 #     group_id: integer
 #     project_id: integer
 #     milestone_title: string
 #     assignee_id: integer
 #     search: string
@@ -205,14 +205,17 @@ def by_scope(items)
       items
     end
   end
 
   def by_state(items)
-    params[:state] ||= 'all'
-
-    if items.respond_to?(params[:state])
-      items.public_send(params[:state])
+    case params[:state].to_s
+    when 'closed'
+      items.closed
+    when 'merged'
+      items.respond_to?(:merged) ? items.merged : items.closed
+    when 'opened'
+      items.opened
     else
       items
     end
   end
 
diff --git a/changelogs/unreleased/validate-state-param-when-filtering-issuables.yml b/changelogs/unreleased/validate-state-param-when-filtering-issuables.yml
new file mode 100644
index 0000000000..3fb025806b
--- /dev/null
+++ b/changelogs/unreleased/validate-state-param-when-filtering-issuables.yml
@@ -0,0 +1,4 @@
+---
+title: Validate state param when filtering issuables
+merge_request:
+author:
diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb
index 40bccb8e50..7f69e888f3 100644
--- a/spec/finders/issues_finder_spec.rb
+++ b/spec/finders/issues_finder_spec.rb
@@ -8,10 +8,11 @@
   let(:milestone) { create(:milestone, project: project1) }
   let(:label) { create(:label, project: project2) }
   let(:issue1) { create(:issue, author: user, assignee: user, project: project1, milestone: milestone, title: 'gitlab') }
   let(:issue2) { create(:issue, author: user, assignee: user, project: project2, description: 'gitlab') }
   let(:issue3) { create(:issue, author: user2, assignee: user2, project: project2) }
+  let(:closed_issue) { create(:issue, author: user2, assignee: user2, project: project2, state: 'closed') }
   let!(:label_link) { create(:label_link, label: label, target: issue2) }
 
   before do
     project1.team << [user, :master]
     project2.team << [user, :developer]
@@ -23,11 +24,11 @@
   end
 
   describe '#execute' do
     let(:search_user) { user }
     let(:params) { {} }
-    let(:issues) { IssuesFinder.new(search_user, params.merge(scope: scope, state: 'opened')).execute }
+    let(:issues) { IssuesFinder.new(search_user, params.reverse_merge(scope: scope, state: 'opened')).execute }
 
     context 'scope: all' do
       let(:scope) { 'all' }
 
       it 'returns all issues' do
@@ -141,10 +142,44 @@
         it 'returns issue with iid match' do
           expect(issues).to contain_exactly(issue3)
         end
       end
 
+      context 'filtering by state' do
+        context 'with opened' do
+          let(:params) { { state: 'opened' } }
+
+          it 'returns only opened issues' do
+            expect(issues).to contain_exactly(issue1, issue2, issue3)
+          end
+        end
+
+        context 'with closed' do
+          let(:params) { { state: 'closed' } }
+
+          it 'returns only closed issues' do
+            expect(issues).to contain_exactly(closed_issue)
+          end
+        end
+
+        context 'with all' do
+          let(:params) { { state: 'all' } }
+
+          it 'returns all issues' do
+            expect(issues).to contain_exactly(issue1, issue2, issue3, closed_issue)
+          end
+        end
+
+        context 'with invalid state' do
+          let(:params) { { state: 'invalid_state' } }
+
+          it 'returns all issues' do
+            expect(issues).to contain_exactly(issue1, issue2, issue3, closed_issue)
+          end
+        end
+      end
+
       context 'when the user is unauthorized' do
         let(:search_user) { nil }
 
         it 'returns no results' do
           expect(issues).to be_empty

Vulnerable code: #     state: 'open' or 'closed' or 'all'
    params[:state] ||= 'all'

    if items.respond_to?(params[:state])
      items.public_send(params[:state])
    let(:issues) { IssuesFinder.new(search_user, params.merge(scope: scope, state: 'opened')).execute }
Fixed code: #     state: 'opened' or 'closed' or 'all'
    case params[:state].to_s
    when 'closed'
      items.closed
    when 'merged'
      items.respond_to?(:merged) ? items.merged : items.closed
    when 'opened'
      items.opened
---
title: Validate state param when filtering issuables
merge_request:
author:
  let(:closed_issue) { create(:issue, author: user2, assignee: user2, project: project2, state: 'closed') }
    let(:issues) { IssuesFinder.new(search_user, params.reverse_merge(scope: scope, state: 'opened')).execute }
      context 'filtering by state' do
        context 'with opened' do
          let(:params) { { state: 'opened' } }

          it 'returns only opened issues' do
            expect(issues).to contain_exactly(issue1, issue2, issue3)
          end
        end

        context 'with closed' do
          let(:params) { { state: 'closed' } }

          it 'returns only closed issues' do
            expect(issues).to contain_exactly(closed_issue)
          end
        end

        context 'with all' do
          let(:params) { { state: 'all' } }

          it 'returns all issues' do
            expect(issues).to contain_exactly(issue1, issue2, issue3, closed_issue)
          end
        end

        context 'with invalid state' do
          let(:params) { { state: 'invalid_state' } }

          it 'returns all issues' do
            expect(issues).to contain_exactly(issue1, issue2, issue3, closed_issue)
          end
        end
      end