CVE-2014-8540: Permissions, Privileges, and Access Controls (CWE-264)
value: The groups API in GitLab 6.x and 7.x before 7.4.3 allows remote authenticated guest users to modify ownership of arbitrary groups by leveraging improper permission checks.
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: 6.5
Published: 2018-01-05T16:29Z
Repository: https://gitlab.com/gitlab-org/gitlab-ce
Language: Ruby
Files changed: lib/api/group_members.rb, spec/requests/api/group_members_spec.rb
Commit date: 2014-10-29 16:00:55 +0200
Commit message: Dont allow guests..developers to manage group members

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

Diff: diff --git a/lib/api/group_members.rb b/lib/api/group_members.rb
index 24c141e9b..d596517c8 100644
--- a/lib/api/group_members.rb
+++ b/lib/api/group_members.rb
@@ -37,18 +37,22 @@ def validate_access_level?(level)
       #   user_id (required) - the users id
       #   access_level (required) - Project access level
       # Example Request:
       #  POST /groups/:id/members
       post ":id/members" do
+        group = find_group(params[:id])
+        authorize! :manage_group, group
         required_attributes! [:user_id, :access_level]
+
         unless validate_access_level?(params[:access_level])
           render_api_error!("Wrong access level", 422)
         end
-        group = find_group(params[:id])
+
         if group.group_members.find_by(user_id: params[:user_id])
           render_api_error!("Already exists", 409)
         end
+
         group.add_users([params[:user_id]], params[:access_level])
         member = group.group_members.find_by(user_id: params[:user_id])
         present member.user, with: Entities::GroupMember, group: group
       end
 
@@ -60,11 +64,13 @@ def validate_access_level?(level)
       #
       # Example Request:
       #   DELETE /groups/:id/members/:user_id
       delete ":id/members/:user_id" do
         group = find_group(params[:id])
-        member =  group.group_members.find_by(user_id: params[:user_id])
+        authorize! :manage_group, group
+        member = group.group_members.find_by(user_id: params[:user_id])
+
         if member.nil?
           render_api_error!("404 Not Found - user_id:#{params[:user_id]} not a member of group #{group.name}",404)
         else
           member.destroy
         end
diff --git a/spec/requests/api/group_members_spec.rb b/spec/requests/api/group_members_spec.rb
index b266f56a9..4957186f6 100644
--- a/spec/requests/api/group_members_spec.rb
+++ b/spec/requests/api/group_members_spec.rb
@@ -113,18 +113,24 @@
       end
     end
 
     context "when a member of the group" do
       it "should delete guest's membership of group" do
-        count_before=group_with_members.group_members.count
-        delete api("/groups/#{group_with_members.id}/members/#{guest.id}", owner)
+        expect {
+          delete api("/groups/#{group_with_members.id}/members/#{guest.id}", owner)
+        }.to change { group_with_members.members.count }.by(-1)
+
         response.status.should == 200
-        group_with_members.group_members.count.should == count_before - 1
       end
 
       it "should return a 404 error when user id is not known" do
         delete api("/groups/#{group_with_members.id}/members/1328", owner)
         response.status.should == 404
       end
+
+      it "should not allow guest to modify group members" do
+        delete api("/groups/#{group_with_members.id}/members/#{master.id}", guest)
+        response.status.should == 403
+      end
     end
   end
 end

Vulnerable code:         group = find_group(params[:id])
        member =  group.group_members.find_by(user_id: params[:user_id])
        count_before=group_with_members.group_members.count
        delete api("/groups/#{group_with_members.id}/members/#{guest.id}", owner)
        group_with_members.group_members.count.should == count_before - 1
Fixed code:         group = find_group(params[:id])
        authorize! :manage_group, group



        authorize! :manage_group, group
        member = group.group_members.find_by(user_id: params[:user_id])

        expect {
          delete api("/groups/#{group_with_members.id}/members/#{guest.id}", owner)
        }.to change { group_with_members.members.count }.by(-1)


      it "should not allow guest to modify group members" do
        delete api("/groups/#{group_with_members.id}/members/#{master.id}", guest)
        response.status.should == 403
      end