CVE-2013-3703: Permission Issues (CWE-275)
value: The controller of the Open Build Service API prior to version 2.4.4 is missing a write permission check, allowing an authenticated attacker to add or remove user roles from packages and/or project meta data.
CWE description: Weaknesses in this category are related to improper assignment or handling of permissions.
CVSS v3 base score: 6.5
Published: 2018-06-08T17:29Z
Repository: https://github.com/openSUSE/open-build-service
Language: Ruby
Files changed: src/api/app/controllers/application_controller.rb, src/api/app/models/package.rb, src/api/app/models/project.rb, src/api/test/unit/project_test.rb
Commit date: 2013-07-05 17:21:21 +0200
Commit message: [api] move write permission checks from controller to package and project model

This is just the first part doing the checks. Removing the old ones is another step.

Diff: diff --git a/src/api/app/controllers/application_controller.rb b/src/api/app/controllers/application_controller.rb
index 7682ee787b..50988d4e53 100644
--- a/src/api/app/controllers/application_controller.rb
+++ b/src/api/app/controllers/application_controller.rb
@@ -440,10 +440,18 @@ class ApplicationController < ActionController::API
     xml_text = String.new
     xml.write xml_text
     render :text => xml_text, :status => http_status
   end
 
+  rescue_from Project::WritePermissionError do |exception|
+    render_error :status => 403, :errorcode => "modify_project_no_permission", :message => exception.message
+  end
+
+  rescue_from Package::WritePermissionError do |exception|
+    render_error :status => 403, :errorcode => "modify_package_no_permission", :message => exception.message
+  end
+
   rescue_from Suse::Backend::NotFoundError, ActiveRecord::RecordNotFound do |exception|
     render_error message: exception.message, status: 404, errorcode: 'not_found'
   end
 
   def permissions
diff --git a/src/api/app/models/package.rb b/src/api/app/models/package.rb
index 77c4589193..b221a14262 100644
--- a/src/api/app/models/package.rb
+++ b/src/api/app/models/package.rb
@@ -13,10 +13,13 @@ class Package < ActiveRecord::Base
     setup "delete_error"
   end
   class SaveError < APIException
     setup "package_save_error"
   end
+  class WritePermissionError < APIException
+    setup "package_write_permission_error"
+  end
   class ReadAccessError < APIException
     setup 'unknown_package', 404, "Unknown package"
   end
   class UnknownObjectError < APIException
     setup 'unknown_package', 404, "Unknown package"
@@ -237,10 +240,18 @@ class Package < ActiveRecord::Base
   def is_locked?
     return true if flags.find_by_flag_and_status "lock", "enable"
     return self.project.is_locked?
   end
 
+  def check_write_access!
+    return if Rails.env.test? and User.current.nil? # for unit tests
+
+    unless User.current.can_modify_package? self
+      raise WritePermissionError, "No permission to modify package '#{self.name}' for user '#{User.current.login}'"
+    end
+  end
+
   # NOTE: this is no permission check, should it be added ?
   def can_be_deleted?
     # check if other packages have me as devel package
     msg = ""
     packs = []
@@ -283,18 +294,21 @@ class Package < ActiveRecord::Base
     self.set_package_kind
     self.update_activity
   end
 
   def add_package_kind( kinds )
+    check_write_access!
     private_set_package_kind( kinds, nil, true )
   end
 
   def set_package_kind( kinds = nil )
+    check_write_access!
     private_set_package_kind( kinds )
   end
 
   def set_package_kind_from_commit( commit )
+    check_write_access!
     private_set_package_kind( nil, commit )
   end
 
   def private_set_package_kind( kinds=nil, directory=nil, _noreset=nil )
     if kinds
@@ -417,10 +431,11 @@ class Package < ActiveRecord::Base
     #logger.debug "WORKED - #{pkg.inspect}"
     return pkg
   end
 
   def update_from_xml( xmlhash )
+    check_write_access!
     self.title = xmlhash.value('title')
     self.description = xmlhash.value('description')
     self.bcntsynctag = nil
     self.bcntsynctag = xmlhash.value('bcntsynctag')
 
@@ -614,10 +629,11 @@ class Package < ActiveRecord::Base
     path += "&comment=#{CGI.escape(comment)}" if comment
     Suse::Backend.put_source( path, render_attribute_axml )
   end
 
   def store(opts = {})
+    # no write access check here, since this operation may will disable this permission ...
     @commit_opts = opts
     save!
   end
 
   def reset_cache
@@ -647,10 +663,11 @@ class Package < ActiveRecord::Base
     end
     return a
   end
 
   def add_user( user, role )
+    check_write_access!
     unless role.kind_of? Role
       role = Role.get_by_title(role)
     end
 
     if role.global
@@ -667,10 +684,11 @@ class Package < ActiveRecord::Base
                                        :user => user,
                                        :role => role )
   end
 
   def add_group( group, role )
+    check_write_access!
     unless role.kind_of? Role
       role = Role.get_by_title(role)
     end
 
     if role.global
@@ -931,18 +949,21 @@ class Package < ActiveRecord::Base
   def expand_flags
     return project.expand_flags(self)
   end
 
   def remove_all_persons
+    check_write_access!
     self.package_user_role_relationships.delete_all
   end
 
   def remove_all_groups
+    check_write_access!
     self.package_group_role_relationships.delete_all
   end
 
   def remove_role(what, role)
+    check_write_access!
     if what.kind_of? Group
       rel = self.package_group_role_relationships.where(bs_group_id: what.id)
     else
       rel = self.package_user_role_relationships.where(bs_user_id: what.id)
     end
@@ -952,10 +973,11 @@ class Package < ActiveRecord::Base
       write_to_backend
     end
   end
 
   def add_role(what, role)
+    check_write_access!
     self.transaction do
       if what.kind_of? Group
         self.package_group_role_relationships.create!(role: role, group: what)
       else
         self.package_user_role_relationships.create!(role: role, user: what)
diff --git a/src/api/app/models/project.rb b/src/api/app/models/project.rb
index 38c3978dbe..33c88c775b 100644
--- a/src/api/app/models/project.rb
+++ b/src/api/app/models/project.rb
@@ -17,10 +17,13 @@ class Project < ActiveRecord::Base
     setup 'unknown_project', 404, "Unknown project"
   end
   class SaveError < APIException
     setup "project_save_error"
   end
+  class WritePermissionError < APIException
+    setup "project_write_permission_error"
+  end
   class ForbiddenError < APIException
     setup("change_project_protection_level", 403,
           "admin rights are required to raise the protection level of a project (it won't be safe anyway)")
   end
 
@@ -245,10 +248,19 @@ class Project < ActiveRecord::Base
       end
       return nil
     end
   end
 
+  def check_write_access!
+    return if Rails.env.test? and User.current.nil? # for unit tests
+
+    # the can_create_check is inconsistent with package class check_write_access! check
+    unless User.current.can_modify_project?(self) || User.current.can_create_project?(self.name)
+      raise WritePermissionError, "No permission to modify project '#{self.name}' for user '#{User.current.login}'"
+    end
+  end
+
   def find_linking_projects
       sql =<<-END_SQL
       SELECT prj.*
       FROM projects prj
       LEFT OUTER JOIN linked_projects lp ON lp.db_project_id = prj.id
@@ -287,10 +299,12 @@ class Project < ActiveRecord::Base
     end
     
   end
 
   def update_from_xml(xmlhash, force=nil)
+    check_write_access!
+
     # check for raising read access permissions, which can't get ensured atm
     unless self.new_record? || self.disabled_for?('access', nil, nil)
       if FlagHelper.xml_disabled_for?(xmlhash, 'access')
         raise ForbiddenError.new
       end
@@ -890,10 +904,12 @@ class Project < ActiveRecord::Base
   def find_parent
     self.class.find_parent_for self.name
   end
 
   def add_user( user, role )
+    check_write_access!
+
     unless role.kind_of? Role
       role = Role.get_by_title(role)
     end
     if role.global
       #only nonglobal roles may be set in a project
@@ -910,10 +926,12 @@ class Project < ActiveRecord::Base
       :user => user,
       :role => role )
   end
 
   def add_group( group, role )
+    check_write_access!
+
     unless role.kind_of? Role
       role = Role.get_by_title(role)
     end
     if role.global
       #only nonglobal roles may be set in a project
@@ -1521,10 +1539,12 @@ class Project < ActiveRecord::Base
     return 'standard' unless mytype
     return mytype.name
   end
 
   def set_project_type(project_type_name)
+    check_write_access!
+
     mytype = DbProjectType.find_by_name(project_type_name)
     return false unless mytype
     self.type_id = mytype.id
     self.save!
     return true
@@ -1533,10 +1553,12 @@ class Project < ActiveRecord::Base
   def maintenance_project
     return Project.find_by_id(maintenance_project_id)
   end
 
   def set_maintenance_project(project)
+    check_write_access!
+
     if project.class == Project
       self.maintenance_project_id = project.id
       self.save!
       return true
     elsif project.class == String
@@ -1581,23 +1603,25 @@ class Project < ActiveRecord::Base
     tocheck_repos.uniq
   end
 
   # called either directly or from delayed job
   def do_project_copy( params )
+    # set user if nil, needed for delayed job in Package model
+    User.current ||= User.find_by_login(params[:user])
+
+    check_write_access!
+
     # copy entire project in the backend
     begin
       path = "/source/#{URI.escape(self.name)}"
       path << Suse::Backend.build_query_from_hash(params, [:cmd, :user, :comment, :oproject, :withbinaries, :withhistory, :makeolder])
       Suse::Backend.post path, nil
     rescue Suse::Backend::HTTPError => e
       logger.debug "copy failed: #{e.message}"
       # we need to check results of backend in any case (also timeout error eg)
     end
 
-    # set user if nil, needed for delayed job in Package model
-    User.current ||= User.find_by_login(params[:user])
-
     # restore all package meta data objects in DB
     backend_pkgs = Collection.find :package, :match => "@project='#{self.name}'"
     backend_pkgs.each_package do |package|
       path = "/source/#{URI.escape(self.name)}/#{package.name}/_meta"
       p = self.packages.new(name: package.name)
@@ -1609,10 +1633,12 @@ class Project < ActiveRecord::Base
 
   # called either directly or from delayed job
   def do_project_release( params )
     User.current ||= User.find_by_login(params[:user])
 
+    check_write_access!
+
     packages.each do |pkg|
       pkg.project.repositories.each do |repo|
         next if params[:repository] and params[:repository] != repo.name
         repo.release_targets.each do |releasetarget|
           # release source and binaries
@@ -1645,10 +1671,12 @@ class Project < ActiveRecord::Base
     return true if self.project_user_role_relationships.where(role_id: role.id, bs_user_id: user.id).first
     return !self.project_group_role_relationships.where(role_id: role).joins(:groups_users).where(groups_users: { user_id: user.id }).first.nil?
   end
 
   def remove_role(what, role)
+    check_write_access!
+
     if what.kind_of? Group
       rel = self.project_group_role_relationships.where(bs_group_id: what.id)
     else
       rel = self.project_user_role_relationships.where(bs_user_id: what.id)
     end
@@ -1658,10 +1686,12 @@ class Project < ActiveRecord::Base
       write_to_backend
     end
   end
  
   def add_role(what, role)
+    check_write_access!
+
     self.transaction do
       if what.kind_of? Group
         self.project_group_role_relationships.create!(role: role, group: what)
       else
         self.project_user_role_relationships.create!(role: role, user: what)
@@ -1682,10 +1712,12 @@ class Project < ActiveRecord::Base
   def valid_name
     errors.add(:name, "is illegal") unless Project.valid_name?(self.name)
   end
 
   def update_patchinfo(patchinfo, opts = {})
+    check_write_access!
+
     opts[:enfore_issue_update] ||= false
 
     # collect bugnumbers from diff
     self.packages.each do |p|
       # create diff per package
@@ -1719,10 +1751,12 @@ class Project < ActiveRecord::Base
 
     return patchinfo
   end
 
   def create_patchinfo_from_request(req)
+    check_write_access!
+
     patchinfo = Package.new(:name => "patchinfo", :title => "Patchinfo", :description => "Collected packages for update")
     self.packages << patchinfo
     patchinfo.add_flag("build", "enable", nil, nil)
     patchinfo.add_flag("useforbuild", "disable", nil, nil)
     patchinfo.add_flag("publish", "enable", nil, nil) unless self.flags.find_by_flag_and_status("access", "disable")
@@ -1753,10 +1787,11 @@ class Project < ActiveRecord::Base
     patchinfo.sources_changed
   end
 
   # updates packages automatically generated in the backend after submitting a product file
   def update_product_autopackages
+    check_write_access!
 
     backend_pkgs = Collection.find :id, :what => 'package', :match => "@project='#{self.name}' and starts-with(@name,'_product:')"
     b_pkg_index = backend_pkgs.each_package.inject(Hash.new) {|hash,elem| hash[elem.name] = elem; hash}
     frontend_pkgs = self.packages.where("`packages`.name LIKE '_product:%'")
     f_pkg_index = frontend_pkgs.inject(Hash.new) {|hash,elem| hash[elem.name] = elem; hash}
diff --git a/src/api/test/unit/project_test.rb b/src/api/test/unit/project_test.rb
index 611ecf6cdb..5c240fe4f1 100644
--- a/src/api/test/unit/project_test.rb
+++ b/src/api/test/unit/project_test.rb
@@ -26,10 +26,11 @@ class ProjectTest < ActiveSupport::TestCase
 
   end
   
   
   def test_add_new_flags_from_xml
+    User.current = users( :Iggy )
     
     #precondition check
     @project.flags.delete_all
     @project.reload
     assert_equal 0, @project.flags.size
@@ -86,10 +87,12 @@ class ProjectTest < ActiveSupport::TestCase
     
   end
   
   
   def test_delete_flags_through_xml
+    User.current = users( :Iggy )
+
     #check precondition
     assert_equal 2, @project.type_flags('build').size
     assert_equal 2, @project.type_flags('publish').size
     
     #project is given as axml
@@ -105,10 +108,12 @@ class ProjectTest < ActiveSupport::TestCase
     assert_equal 0, @project.type_flags('publish').size
   end
 
     
   def test_store_axml
+    User.current = users( :Iggy )
+
     original = @project.to_axml
 
     #project is given as axml
     axml = Xmlhash.parse(
       "<project name='home:Iggy'>
@@ -129,10 +134,12 @@ class ProjectTest < ActiveSupport::TestCase
 
     @project.update_from_xml(Xmlhash.parse(original))
   end  
 
   def test_ordering
+    User.current = users( :Iggy )
+
     #project is given as axml
     axml = Xmlhash.parse(
       "<project name='home:Iggy'>
         <title>Iggy's Home Project</title>
         <description>dummy</description>
@@ -173,10 +180,11 @@ class ProjectTest < ActiveSupport::TestCase
     assert_xml_tag xml, :tag => :arch, :content => 'x86_64', :before => { :tag => :arch, :content => 'local' }
     
   end
     
   test "duplicated repos" do
+     User.current = users( :king )
      orig = @project.render_axml
 
      axml = Xmlhash.parse(
       "<project name='home:Iggy'>
         <title>Iggy's Home Project</title>
@@ -197,10 +205,11 @@ class ProjectTest < ActiveSupport::TestCase
      @project.reload
      assert_equal orig, @project.render_axml
   end
 
   test "duplicated repos with remote" do
+     User.current = users( :Iggy )
      orig = @project.render_axml
 
      xml = <<END
 <project name="home:Iggy">
   <title>Iggy"s Home Project</title>
@@ -223,10 +232,11 @@ END
      end
      @project.reload
      assert_equal orig, @project.render_axml
   end
   test "not duplicated repos with remote" do
+     User.current = users( :Iggy )
      xml = <<END
 <project name="home:Iggy">
   <title>Iggy"s Home Project</title>
   <description>dummy</description>
   <repository name="remote_2">
@@ -248,10 +258,11 @@ END
      @project.reload
      assert_equal xml, @project.render_axml
   end
 
   def test_create_maintenance_project_and_maintained_project
+    User.current = users( :king )
     maintenance_project = Project.new(:name => 'Maintenance:Project')
     assert_equal true, maintenance_project.set_project_type('maintenance')
     assert_equal 'maintenance', maintenance_project.project_type()
 
     # Create a project for which maintenance is done (i.e. a maintained project)

Vulnerable code:     # set user if nil, needed for delayed job in Package model
    User.current ||= User.find_by_login(params[:user])

Fixed code:   rescue_from Project::WritePermissionError do |exception|
    render_error :status => 403, :errorcode => "modify_project_no_permission", :message => exception.message
  end

  rescue_from Package::WritePermissionError do |exception|
    render_error :status => 403, :errorcode => "modify_package_no_permission", :message => exception.message
  end

  class WritePermissionError < APIException
    setup "package_write_permission_error"
  end
  def check_write_access!
    return if Rails.env.test? and User.current.nil? # for unit tests

    unless User.current.can_modify_package? self
      raise WritePermissionError, "No permission to modify package '#{self.name}' for user '#{User.current.login}'"
    end
  end

    check_write_access!
    check_write_access!
    check_write_access!
    check_write_access!
    # no write access check here, since this operation may will disable this permission ...
    check_write_access!
    check_write_access!
    check_write_access!
    check_write_access!
    check_write_access!
    check_write_access!
  class WritePermissionError < APIException
    setup "project_write_permission_error"
  end
  def check_write_access!
    return if Rails.env.test? and User.current.nil? # for unit tests

    # the can_create_check is inconsistent with package class check_write_access! check
    unless User.current.can_modify_project?(self) || User.current.can_create_project?(self.name)
      raise WritePermissionError, "No permission to modify project '#{self.name}' for user '#{User.current.login}'"
    end
  end

    check_write_access!

    check_write_access!

    check_write_access!

    check_write_access!

    check_write_access!

    # set user if nil, needed for delayed job in Package model
    User.current ||= User.find_by_login(params[:user])

    check_write_access!

    check_write_access!

    check_write_access!

    check_write_access!

    check_write_access!

    check_write_access!

    check_write_access!
    User.current = users( :Iggy )
    User.current = users( :Iggy )

    User.current = users( :Iggy )

    User.current = users( :Iggy )

     User.current = users( :king )
     User.current = users( :Iggy )
     User.current = users( :Iggy )
    User.current = users( :king )