Maintenance: Improve package installation.

app/assets/javascripts/app/views/package.jst.eco

@@ -3,6 +3,11 @@
 </div>
 
 <div class="page-content">
+  <p>
+    <%- @T('The installation of packages comes with security implications, because arbitrary code will be executed in the context of the Zammad application.') %>
+    <br>
+    <%- @T('Only packages from known, trusted and verfied sources should be installed.') %>
+  </p>
   <p>
     <%- @T('After installing, updating or uninstalling packages the following commands need to be executed on the server:') %>
     <ul>
@@ -48,4 +53,4 @@
     <% end %>
     </tbody>
   </table>
-</div>
+</div>

app/models/package.rb

@@ -263,32 +263,37 @@ subsequently in a separate step.
       )
     end
 
-    # store package
-    if !data[:reinstall]
-      package_db = Package.create(meta)
-      Store.add(
-        object:        'Package',
-        o_id:          package_db.id,
-        data:          package.to_json,
-        filename:      "#{meta[:name]}-#{meta[:version]}.zpm",
-        preferences:   {},
-        created_by_id: UserInfo.current_user_id || 1,
-      )
-    end
+    Transaction.execute do
+      # store package
+      if !data[:reinstall]
+        package_db = Package.create(meta)
+        Store.add(
+          object:        'Package',
+          o_id:          package_db.id,
+          data:          package.to_json,
+          filename:      "#{meta[:name]}-#{meta[:version]}.zpm",
+          preferences:   {},
+          created_by_id: UserInfo.current_user_id || 1,
+        )
+      end
 
-    # write files
-    package['files'].each do |file|
-      permission = file['permission'] || '644'
-      content    = Base64.decode64(file['content'])
-      _write_file(file['location'], permission, content)
-    end
+      # write files
+      package['files'].each do |file|
+        if !allowed_file_path?(file['location'])
+          raise "Can't create file, because of not allowed file location: #{file['location']}!"
+        end
 
-    # update package state
-    package_db.state = 'installed'
-    package_db.save
+        permission = file['permission'] || '644'
+        content    = Base64.decode64(file['content'])
+        _write_file(file['location'], permission, content)
+      end
+
+      # update package state
+      package_db.state = 'installed'
+      package_db.save
+    end
 
     # prebuild assets
-
     package_db
   end
 
@@ -483,4 +488,9 @@ execute all pending package migrations at once
 
     true
   end
+
+  def self.allowed_file_path?(file)
+    file.exclude?('..') && file.exclude?('%2e%2e')
+  end
+  private_class_method :allowed_file_path?
 end

spec/models/package_spec.rb

@@ -0,0 +1,82 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe Package, type: :model do
+  let(:package_zpm_files_json) do
+    <<-JSON
+      [
+        {
+          "permission": "644",
+          "location": "example.rb",
+          "content": "YWJjw6TDtsO8w58="
+        },
+        {
+          "permission": "644",
+          "location": "app/controllers/test_controller.rb",
+          "content": "YWJjw6TDtsO8w58="
+        }
+      ]
+    JSON
+  end
+  let(:package_zpm_json) do
+    <<-JSON
+    {
+      "name": "UnitTestSample",
+      "version": "1.0.1",
+      "vendor": "Zammad Foundation",
+      "license": "ABC",
+      "url": "https://zammad.org/",
+      "description": [
+        {
+          "language": "en",
+          "text": "some description"
+        }
+      ],
+      "files": #{package_zpm_files_json}
+    }
+    JSON
+  end
+
+  context 'with different file locations' do
+    context 'with correct file locations' do
+      it 'installation should work' do
+        expect(described_class.install(string: package_zpm_json)).to be_truthy
+      end
+    end
+
+    shared_examples 'check not allowed file location' do |file_location|
+      let(:package_zpm_files_json) do
+        <<-JSON
+          [
+            {
+              "permission": "644",
+              "location": "example.rb",
+              "content": "YWJjw6TDtsO8w58="
+            },
+            {
+              "permission": "644",
+              "location": "#{file_location}",
+              "content": "YWJjw6TDtsO8w58="
+            }
+          ]
+        JSON
+      end
+
+      it 'installation should raise a error and package/store should not be present, because of not allowed file location' do
+        expect { described_class.install(string: package_zpm_json) }
+          .to raise_error(RuntimeError)
+          .and change(described_class, :count).by(0)
+          .and change(Store, :count).by(0)
+      end
+    end
+
+    context "with not allowed file location part: '..'" do
+      include_examples 'check not allowed file location', '../../../../../tmp/test_controller.rb'
+    end
+
+    context "with not allowed file location part: '%2e%2e'" do
+      include_examples 'check not allowed file location', '%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/tmp/test_controller.rb'
+    end
+  end
+end