CVE-2020-29160: Missing Authorization (CWE-862)
value: An issue was discovered in Zammad before 3.5.1. A REST API call allows an attacker to change Ticket Article data in a way that defeats auditing.
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: 7.5
Published: 2020-12-28T08:15Z
Repository: https://github.com/zammad/zammad
Language: Ruby
Files changed: app/controllers/ticket_articles_controller.rb, spec/requests/ticket/article_spec.rb, spec/requests/ticket_spec.rb
Commit date: 2020-11-16 09:08:52 +0100
Commit message: Maintenance: Sync Article REST API capabilities with the ones of the frontend.

Diff: diff --git a/app/controllers/ticket_articles_controller.rb b/app/controllers/ticket_articles_controller.rb
index 642d11771..ad2a16536 100644
--- a/app/controllers/ticket_articles_controller.rb
+++ b/app/controllers/ticket_articles_controller.rb
@@ -99,15 +99,22 @@ class TicketArticlesController < ApplicationController
   # PUT /articles/1
   def update
     article = Ticket::Article.find(params[:id])
     authorize!(article)
 
-    clean_params = Ticket::Article.association_name_to_id_convert(params)
-    clean_params = Ticket::Article.param_cleanup(clean_params, true)
-
-    # only apply preferences changes (keep not updated keys/values)
-    clean_params = article.param_preferences_merge(clean_params)
+    # only update internal and highlight info
+    clean_params = {}
+    if !params[:internal].nil?
+      clean_params[:internal] = params[:internal]
+    end
+    if params.dig(:preferences, :highlight).present?
+      clean_params = article.param_preferences_merge(clean_params.merge(
+                                                       preferences: {
+                                                         highlight: params[:preferences][:highlight].to_s
+                                                       }
+                                                     ))
+    end
 
     article.update!(clean_params)
 
     if response_expand?
       result = article.attributes_with_association_names
diff --git a/spec/requests/ticket/article_spec.rb b/spec/requests/ticket/article_spec.rb
index 531d5c9b4..8f98c4552 100644
--- a/spec/requests/ticket/article_spec.rb
+++ b/spec/requests/ticket/article_spec.rb
@@ -116,41 +116,49 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO
       params = {
         ticket_id:    json_response['ticket_id'],
         content_type: 'text/plain',
         body:         'some body',
         type:         'note',
+        internal:     false,
         preferences:  {
           some_key1: 123,
+          highlight: '123',
         },
       }
       post '/api/v1/ticket_articles', params: params, as: :json
       expect(response).to have_http_status(:created)
       expect(json_response).to be_a_kind_of(Hash)
       expect(json_response['subject']).to be_nil
       expect(json_response['body']).to eq('some body')
+      expect(json_response['internal']).to eq(false)
       expect(json_response['content_type']).to eq('text/plain')
       expect(json_response['updated_by_id']).to eq(agent.id)
       expect(json_response['created_by_id']).to eq(agent.id)
       expect(json_response['preferences']['some_key1']).to eq(123)
+      expect(json_response['preferences']['highlight']).to eq('123')
       expect(ticket.articles.count).to eq(5)
 
       params = {
         body:        'some body 2',
+        internal:    true,
         preferences: {
           some_key2: 'abc',
+          highlight: '234',
         },
       }
       put "/api/v1/ticket_articles/#{json_response['id']}", params: params, as: :json
       expect(response).to have_http_status(:ok)
       expect(json_response).to be_a_kind_of(Hash)
       expect(json_response['subject']).to be_nil
-      expect(json_response['body']).to eq('some body 2')
+      expect(json_response['body']).not_to eq('some body 2')
+      expect(json_response['internal']).to eq(true)
       expect(json_response['content_type']).to eq('text/plain')
       expect(json_response['updated_by_id']).to eq(agent.id)
       expect(json_response['created_by_id']).to eq(agent.id)
       expect(json_response['preferences']['some_key1']).to eq(123)
-      expect(json_response['preferences']['some_key2']).to eq('abc')
+      expect(json_response['preferences']['some_key2']).not_to eq('abc')
+      expect(json_response['preferences']['highlight']).to eq('234')
 
     end
 
     it 'does ticket create with customer and articles' do
       params = {
diff --git a/spec/requests/ticket_spec.rb b/spec/requests/ticket_spec.rb
index ff36205ca..209ce996f 100644
--- a/spec/requests/ticket_spec.rb
+++ b/spec/requests/ticket_spec.rb
@@ -900,11 +900,11 @@ RSpec.describe 'Ticket', type: :request do
       put "/api/v1/ticket_articles/#{json_response['id']}", params: params, as: :json
       expect(response).to have_http_status(:ok)
       expect(json_response).to be_a_kind_of(Hash)
       expect(json_response['ticket_id']).to eq(ticket.id)
       expect(json_response['from']).to eq(%("Tickets Agent via #{ticket_group.email_address.realname}" <#{ticket_group.email_address.email}>))
-      expect(json_response['subject']).to eq('new subject')
+      expect(json_response['subject']).not_to eq('new subject')
       expect(json_response['body']).to eq('some body')
       expect(json_response['content_type']).to eq('text/plain')
       expect(json_response['internal']).to eq(true)
       expect(json_response['created_by_id']).to eq(agent.id)
       expect(json_response['sender_id']).to eq(Ticket::Article::Sender.lookup(name: 'Agent').id)
@@ -989,11 +989,11 @@ RSpec.describe 'Ticket', type: :request do
       put "/api/v1/ticket_articles/#{json_response['id']}", params: params, as: :json
       expect(response).to have_http_status(:ok)
       expect(json_response).to be_a_kind_of(Hash)
       expect(json_response['ticket_id']).to eq(ticket.id)
       expect(json_response['from']).to eq('Tickets Admin')
-      expect(json_response['subject']).to eq('new subject')
+      expect(json_response['subject']).not_to eq('new subject')
       expect(json_response['body']).to eq('some body')
       expect(json_response['content_type']).to eq('text/plain')
       expect(json_response['internal']).to eq(true)
       expect(json_response['created_by_id']).to eq(admin.id)
       expect(json_response['sender_id']).to eq(Ticket::Article::Sender.lookup(name: 'Agent').id)

Vulnerable code:     clean_params = Ticket::Article.association_name_to_id_convert(params)
    clean_params = Ticket::Article.param_cleanup(clean_params, true)

    # only apply preferences changes (keep not updated keys/values)
    clean_params = article.param_preferences_merge(clean_params)
      expect(json_response['body']).to eq('some body 2')
      expect(json_response['preferences']['some_key2']).to eq('abc')
      expect(json_response['subject']).to eq('new subject')
      expect(json_response['subject']).to eq('new subject')
Fixed code:     # only update internal and highlight info
    clean_params = {}
    if !params[:internal].nil?
      clean_params[:internal] = params[:internal]
    end
    if params.dig(:preferences, :highlight).present?
      clean_params = article.param_preferences_merge(clean_params.merge(
                                                       preferences: {
                                                         highlight: params[:preferences][:highlight].to_s
                                                       }
                                                     ))
    end
        internal:     false,
          highlight: '123',
      expect(json_response['internal']).to eq(false)
      expect(json_response['preferences']['highlight']).to eq('123')
        internal:    true,
          highlight: '234',
      expect(json_response['body']).not_to eq('some body 2')
      expect(json_response['internal']).to eq(true)
      expect(json_response['preferences']['some_key2']).not_to eq('abc')
      expect(json_response['preferences']['highlight']).to eq('234')
      expect(json_response['subject']).not_to eq('new subject')
      expect(json_response['subject']).not_to eq('new subject')