CVE-2020-29159: Insufficient Information (NVD-CWE-noinfo)
value: An issue was discovered in Zammad before 3.5.1. The default signup Role (for newly created Users) can be a privileged Role, if configured by an admin. This behvaior was unintended.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
CVSS v3 base score: 4.9
Published: 2020-12-28T08:15Z
Repository: https://github.com/zammad/zammad
Language: Ruby
Files changed: app/models/role.rb, db/migrate/20120101000001_create_base.rb, db/migrate/20201005113317_role_signup_column.rb, db/seeds/permissions.rb, spec/db/migrate/role_signup_column_spec.rb
Commit date: 2020-11-16 09:08:51 +0100
Commit message: Maintenance: Provide allow_signup column to define the signup permissions for roles and disable new permissions by default as signup permission.

Diff: diff --git a/app/models/role.rb b/app/models/role.rb
index 97a18ef22..a50604ab2 100644
--- a/app/models/role.rb
+++ b/app/models/role.rb
@@ -220,15 +220,14 @@ returns
 
     true
   end
 
   def check_default_at_signup_permissions
-    all_permissions = Permission.all.pluck(:id)
-    admin_permissions = Permission.where('name LIKE ? OR name = ?', 'admin%', 'ticket.agent').pluck(:id) # admin.*/ticket.agent permissions
-    normal_permissions = (all_permissions - admin_permissions) | (admin_permissions - all_permissions) # all other permissions besides admin.*/ticket.agent
-    return true if default_at_signup != true # means if default_at_signup = false, no need further checks
-    return true if self.permission_ids.all? { |i| normal_permissions.include? i } # allow user to choose only normal permissions
+    return true if !default_at_signup
 
-    raise Exceptions::UnprocessableEntity, 'Cannot set default at signup when role has admin or ticket.agent permissions.'
+    forbidden_permissions = permissions.reject(&:allow_signup)
+    return true if forbidden_permissions.blank?
+
+    raise Exceptions::UnprocessableEntity, "Cannot set default at signup when role has #{forbidden_permissions.join(', ')} permissions."
   end
 
 end
diff --git a/db/migrate/20120101000001_create_base.rb b/db/migrate/20120101000001_create_base.rb
index 1cffcac92..79aaa5330 100644
--- a/db/migrate/20120101000001_create_base.rb
+++ b/db/migrate/20120101000001_create_base.rb
@@ -126,15 +126,16 @@ class CreateBase < ActiveRecord::Migration[4.2]
     add_index :roles, [:name], unique: true
     add_foreign_key :roles, :users, column: :created_by_id
     add_foreign_key :roles, :users, column: :updated_by_id
 
     create_table :permissions do |t|
-      t.string :name,                   limit: 255, null: false
-      t.string :note,                   limit: 500, null: true
-      t.string :preferences,            limit: 10_000, null: true
-      t.boolean :active,                               null: false, default: true
-      t.timestamps limit: 3, null: false
+      t.string :name,          limit: 255, null: false
+      t.string :note,          limit: 500, null: true
+      t.string :preferences,   limit: 10_000, null: true
+      t.boolean :active,       null: false, default: true
+      t.boolean :allow_signup, null: false, default: false
+      t.timestamps limit: 3,   null: false
     end
     add_index :permissions, [:name], unique: true
 
     create_table :permissions_roles, id: false do |t|
       t.belongs_to :role, index: true
diff --git a/db/migrate/20201005113317_role_signup_column.rb b/db/migrate/20201005113317_role_signup_column.rb
new file mode 100644
index 000000000..adb4cd395
--- /dev/null
+++ b/db/migrate/20201005113317_role_signup_column.rb
@@ -0,0 +1,31 @@
+class RoleSignupColumn < ActiveRecord::Migration[5.2]
+  def change
+
+    # return if it's a new setup
+    return if !Setting.exists?(name: 'system_init_done')
+
+    add_column :permissions, :allow_signup, :boolean, null: false, default: false
+
+    signup_permissions = [
+      'user_preferences',
+      'user_preferences.password',
+      'user_preferences.notifications',
+      'user_preferences.access_token',
+      'user_preferences.language',
+      'user_preferences.linked_accounts',
+      'user_preferences.device',
+      'user_preferences.avatar',
+      'user_preferences.calendar',
+      'user_preferences.out_of_office',
+      'ticket.customer',
+    ]
+
+    Permission.where(name: signup_permissions).update(allow_signup: true)
+
+    Role.where(default_at_signup: true).find_each do |role|
+      role.permissions.where.not(name: signup_permissions).find_each do |permission|
+        role.permission_revoke(permission.name)
+      end
+    end
+  end
+end
diff --git a/db/seeds/permissions.rb b/db/seeds/permissions.rb
index 70dda8358..0d81da3bc 100644
--- a/db/seeds/permissions.rb
+++ b/db/seeds/permissions.rb
@@ -261,79 +261,89 @@ Permission.create_if_not_exists(
   preferences: {
     translations: ['Sessions']
   },
 )
 Permission.create_if_not_exists(
-  name:        'user_preferences',
-  note:        'User Preferences',
-  preferences: {},
+  name:         'user_preferences',
+  note:         'User Preferences',
+  preferences:  {},
+  allow_signup: true,
 )
 Permission.create_if_not_exists(
-  name:        'user_preferences.password',
-  note:        'Change %s',
-  preferences: {
+  name:         'user_preferences.password',
+  note:         'Change %s',
+  preferences:  {
     translations: ['Password']
   },
+  allow_signup: true,
 )
 Permission.create_if_not_exists(
-  name:        'user_preferences.notifications',
-  note:        'Manage %s',
-  preferences: {
+  name:         'user_preferences.notifications',
+  note:         'Manage %s',
+  preferences:  {
     translations: ['Notifications'],
     required:     ['ticket.agent'],
   },
+  allow_signup: true,
 )
 Permission.create_if_not_exists(
-  name:        'user_preferences.access_token',
-  note:        'Manage %s',
-  preferences: {
+  name:         'user_preferences.access_token',
+  note:         'Manage %s',
+  preferences:  {
     translations: ['Token Access']
   },
+  allow_signup: true,
 )
 Permission.create_if_not_exists(
-  name:        'user_preferences.language',
-  note:        'Change %s',
-  preferences: {
+  name:         'user_preferences.language',
+  note:         'Change %s',
+  preferences:  {
     translations: ['Language']
   },
+  allow_signup: true,
 )
 Permission.create_if_not_exists(
-  name:        'user_preferences.linked_accounts',
-  note:        'Manage %s',
-  preferences: {
+  name:         'user_preferences.linked_accounts',
+  note:         'Manage %s',
+  preferences:  {
     translations: ['Linked Accounts']
   },
+  allow_signup: true,
 )
 Permission.create_if_not_exists(
-  name:        'user_preferences.device',
-  note:        'Manage %s',
-  preferences: {
+  name:         'user_preferences.device',
+  note:         'Manage %s',
+  preferences:  {
     translations: ['Devices']
   },
+  allow_signup: true,
 )
 Permission.create_if_not_exists(
-  name:        'user_preferences.avatar',
-  note:        'Manage %s',
-  preferences: {
+  name:         'user_preferences.avatar',
+  note:         'Manage %s',
+  preferences:  {
     translations: ['Avatar']
   },
+  allow_signup: true,
 )
 Permission.create_if_not_exists(
-  name:        'user_preferences.calendar',
-  note:        'Access to %s',
-  preferences: {
+  name:         'user_preferences.calendar',
+  note:         'Access to %s',
+  preferences:  {
     translations: ['Calendars'],
     required:     ['ticket.agent'],
   },
+  allow_signup: true,
 )
 Permission.create_if_not_exists(
-  name:        'user_preferences.out_of_office',
-  note:        'Change %s',
-  preferences: {
+  name:         'user_preferences.out_of_office',
+  note:         'Change %s',
+  preferences:  {
     translations: ['Out of Office'],
     required:     ['ticket.agent'],
   },
+  allow_signup: true,
 )
 
 Permission.create_if_not_exists(
   name:        'report',
   note:        'Report Interface',
@@ -352,13 +362,14 @@ Permission.create_if_not_exists(
   preferences: {
     plugin: ['groups']
   },
 )
 Permission.create_if_not_exists(
-  name:        'ticket.customer',
-  note:        'Access to Customer Tickets based on current_user and organization',
-  preferences: {},
+  name:         'ticket.customer',
+  note:         'Access to Customer Tickets based on current_user and organization',
+  preferences:  {},
+  allow_signup: true,
 )
 Permission.create_if_not_exists(
   name:        'chat',
   note:        'Access to %s',
   preferences: {
diff --git a/spec/db/migrate/role_signup_column_spec.rb b/spec/db/migrate/role_signup_column_spec.rb
new file mode 100644
index 000000000..810a05e70
--- /dev/null
+++ b/spec/db/migrate/role_signup_column_spec.rb
@@ -0,0 +1,30 @@
+require 'rails_helper'
+
+RSpec.describe RoleSignupColumn, type: :db_migration, db_strategy: :reset do
+  context 'when a role contains signup permissions' do
+    let!(:role) do
+      role = create(:role)
+      role.permission_grant('user_preferences.password')
+      role.permission_grant('ticket.agent')
+      role.update_column(:default_at_signup, true)
+      role
+    end
+
+    before do
+      without_column(:permissions, column: :allow_signup)
+      migrate
+    end
+
+    it 'has password permission' do
+      expect(role.reload.permissions.map(&:name)).to include('user_preferences.password')
+    end
+
+    it 'has no agent permission' do
+      expect(role.reload.permissions.map(&:name)).not_to include('ticket.agent')
+    end
+
+    it 'has permission with allow_signup set correctly' do
+      expect(Permission.find_by(name: 'user_preferences.password').allow_signup).to be true
+    end
+  end
+end

Vulnerable code:     all_permissions = Permission.all.pluck(:id)
    admin_permissions = Permission.where('name LIKE ? OR name = ?', 'admin%', 'ticket.agent').pluck(:id) # admin.*/ticket.agent permissions
    normal_permissions = (all_permissions - admin_permissions) | (admin_permissions - all_permissions) # all other permissions besides admin.*/ticket.agent
    return true if default_at_signup != true # means if default_at_signup = false, no need further checks
    return true if self.permission_ids.all? { |i| normal_permissions.include? i } # allow user to choose only normal permissions
    raise Exceptions::UnprocessableEntity, 'Cannot set default at signup when role has admin or ticket.agent permissions.'
      t.string :name,                   limit: 255, null: false
      t.string :note,                   limit: 500, null: true
      t.string :preferences,            limit: 10_000, null: true
      t.boolean :active,                               null: false, default: true
      t.timestamps limit: 3, null: false
  name:        'user_preferences',
  note:        'User Preferences',
  preferences: {},
  name:        'user_preferences.password',
  note:        'Change %s',
  preferences: {
  name:        'user_preferences.notifications',
  note:        'Manage %s',
  preferences: {
  name:        'user_preferences.access_token',
  note:        'Manage %s',
  preferences: {
  name:        'user_preferences.language',
  note:        'Change %s',
  preferences: {
  name:        'user_preferences.linked_accounts',
  note:        'Manage %s',
  preferences: {
  name:        'user_preferences.device',
  note:        'Manage %s',
  preferences: {
  name:        'user_preferences.avatar',
  note:        'Manage %s',
  preferences: {
  name:        'user_preferences.calendar',
  note:        'Access to %s',
  preferences: {
  name:        'user_preferences.out_of_office',
  note:        'Change %s',
  preferences: {
  name:        'ticket.customer',
  note:        'Access to Customer Tickets based on current_user and organization',
  preferences: {},
Fixed code:     return true if !default_at_signup
    forbidden_permissions = permissions.reject(&:allow_signup)
    return true if forbidden_permissions.blank?

    raise Exceptions::UnprocessableEntity, "Cannot set default at signup when role has #{forbidden_permissions.join(', ')} permissions."
      t.string :name,          limit: 255, null: false
      t.string :note,          limit: 500, null: true
      t.string :preferences,   limit: 10_000, null: true
      t.boolean :active,       null: false, default: true
      t.boolean :allow_signup, null: false, default: false
      t.timestamps limit: 3,   null: false
class RoleSignupColumn < ActiveRecord::Migration[5.2]
  def change

    # return if it's a new setup
    return if !Setting.exists?(name: 'system_init_done')

    add_column :permissions, :allow_signup, :boolean, null: false, default: false

    signup_permissions = [
      'user_preferences',
      'user_preferences.password',
      'user_preferences.notifications',
      'user_preferences.access_token',
      'user_preferences.language',
      'user_preferences.linked_accounts',
      'user_preferences.device',
      'user_preferences.avatar',
      'user_preferences.calendar',
      'user_preferences.out_of_office',
      'ticket.customer',
    ]

    Permission.where(name: signup_permissions).update(allow_signup: true)

    Role.where(default_at_signup: true).find_each do |role|
      role.permissions.where.not(name: signup_permissions).find_each do |permission|
        role.permission_revoke(permission.name)
      end
    end
  end
end
  name:         'user_preferences',
  note:         'User Preferences',
  preferences:  {},
  allow_signup: true,
  name:         'user_preferences.password',
  note:         'Change %s',
  preferences:  {
  allow_signup: true,
  name:         'user_preferences.notifications',
  note:         'Manage %s',
  preferences:  {
  allow_signup: true,
  name:         'user_preferences.access_token',
  note:         'Manage %s',
  preferences:  {
  allow_signup: true,
  name:         'user_preferences.language',
  note:         'Change %s',
  preferences:  {
  allow_signup: true,
  name:         'user_preferences.linked_accounts',
  note:         'Manage %s',
  preferences:  {
  allow_signup: true,
  name:         'user_preferences.device',
  note:         'Manage %s',
  preferences:  {
  allow_signup: true,
  name:         'user_preferences.avatar',
  note:         'Manage %s',
  preferences:  {
  allow_signup: true,
  name:         'user_preferences.calendar',
  note:         'Access to %s',
  preferences:  {
  allow_signup: true,
  name:         'user_preferences.out_of_office',
  note:         'Change %s',
  preferences:  {
  allow_signup: true,
  name:         'ticket.customer',
  note:         'Access to Customer Tickets based on current_user and organization',
  preferences:  {},
  allow_signup: true,
require 'rails_helper'

RSpec.describe RoleSignupColumn, type: :db_migration, db_strategy: :reset do
  context 'when a role contains signup permissions' do
    let!(:role) do
      role = create(:role)
      role.permission_grant('user_preferences.password')
      role.permission_grant('ticket.agent')
      role.update_column(:default_at_signup, true)
      role
    end

    before do
      without_column(:permissions, column: :allow_signup)
      migrate
    end

    it 'has password permission' do
      expect(role.reload.permissions.map(&:name)).to include('user_preferences.password')
    end

    it 'has no agent permission' do
      expect(role.reload.permissions.map(&:name)).not_to include('ticket.agent')
    end

    it 'has permission with allow_signup set correctly' do
      expect(Permission.find_by(name: 'user_preferences.password').allow_signup).to be true
    end
  end
end