CVE-2020-29158: Missing Authorization (CWE-862)
value: An issue was discovered in Zammad before 3.5.1. An Agent with Customer permissions in a Group can bypass intended access control on internal Articles via the Ticket detail view.
CWE description: The product does not perform an authorization check when an actor attempts to access a resource or perform an action.
CVSS v3 base score: 4.3
Published: 2020-12-28T08:15Z
Repository: https://github.com/zammad/zammad
Language: Ruby
Files changed: app/policies/ticket/article_policy.rb, app/policies/ticket_policy.rb, spec/factories/role.rb, spec/policies/ticket/article_policy_spec.rb
Commit date: 2020-11-16 09:08:52 +0100
Commit message: Maintenance: Improved article view for agent customer.

Diff: diff --git a/app/policies/ticket/article_policy.rb b/app/policies/ticket/article_policy.rb
index 31309643b..e01c85a36 100644
--- a/app/policies/ticket/article_policy.rb
+++ b/app/policies/ticket/article_policy.rb
@@ -53,11 +53,11 @@ class Ticket::ArticlePolicy < ApplicationPolicy
   def deletable_timeframe
     deletable_timeframe_setting.seconds
   end
 
   def access?(query)
-    return false if record.internal == true && !user.permissions?('ticket.agent')
-
     ticket = Ticket.lookup(id: record.ticket_id)
+    return false if record.internal == true && !TicketPolicy.new(user, ticket).agent_read_access?
+
     Pundit.authorize(user, ticket, query)
   end
 end
diff --git a/app/policies/ticket_policy.rb b/app/policies/ticket_policy.rb
index 87ab085fd..fd56de931 100644
--- a/app/policies/ticket_policy.rb
+++ b/app/policies/ticket_policy.rb
@@ -39,10 +39,14 @@ class TicketPolicy < ApplicationPolicy
     return true if record.state.name != 'closed' # check if the ticket state is already closed
 
     raise Exceptions::UnprocessableEntity, 'Cannot follow-up on a closed ticket. Please create a new ticket.'
   end
 
+  def agent_read_access?
+    agent_access?('read')
+  end
+
   private
 
   def access?(access)
     return true if agent_access?(access)
 
diff --git a/spec/factories/role.rb b/spec/factories/role.rb
index 6b2842562..55e6ae265 100644
--- a/spec/factories/role.rb
+++ b/spec/factories/role.rb
@@ -6,10 +6,14 @@ FactoryBot.define do
 
     factory :agent_role do
       permissions { Permission.where(name: 'ticket.agent') }
     end
 
+    trait :customer do
+      permissions { Permission.where(name: 'ticket.customer') }
+    end
+
     trait :admin do
       permissions { Permission.where(name: 'admin') }
     end
   end
 end
diff --git a/spec/policies/ticket/article_policy_spec.rb b/spec/policies/ticket/article_policy_spec.rb
index e469dec36..6b8d1be54 100644
--- a/spec/policies/ticket/article_policy_spec.rb
+++ b/spec/policies/ticket/article_policy_spec.rb
@@ -27,10 +27,19 @@ describe Ticket::ArticlePolicy do
       let(:user) { create(:agent_and_customer, groups: [group]) }
 
       it { is_expected.to permit_actions(%i[show]) }
     end
 
+    context 'when agent and customer but no agent group access' do
+      let(:user) do
+        customer_role = create(:role, :customer)
+        create(:agent_and_customer, roles: [customer_role])
+      end
+
+      it { is_expected.not_to permit_actions(%i[show]) }
+    end
+
     context 'when customer' do
       let(:user) { ticket_customer }
 
       it { is_expected.not_to permit_actions(%i[show]) }
     end

Vulnerable code:     return false if record.internal == true && !user.permissions?('ticket.agent')

Fixed code:     return false if record.internal == true && !TicketPolicy.new(user, ticket).agent_read_access?

  def agent_read_access?
    agent_access?('read')
  end

    trait :customer do
      permissions { Permission.where(name: 'ticket.customer') }
    end

    context 'when agent and customer but no agent group access' do
      let(:user) do
        customer_role = create(:role, :customer)
        create(:agent_and_customer, roles: [customer_role])
      end

      it { is_expected.not_to permit_actions(%i[show]) }
    end