diff --git a/app/assets/javascripts/app/views/package.jst.eco b/app/assets/javascripts/app/views/package.jst.eco index c7c34023e..cb2a519ab 100644 --- a/app/assets/javascripts/app/views/package.jst.eco +++ b/app/assets/javascripts/app/views/package.jst.eco @@ -3,6 +3,11 @@
+

+ <%- @T('The installation of packages comes with security implications, because arbitrary code will be executed in the context of the Zammad application.') %> +
+ <%- @T('Only packages from known, trusted and verfied sources should be installed.') %> +

<%- @T('After installing, updating or uninstalling packages the following commands need to be executed on the server:') %>

\ No newline at end of file + diff --git a/app/models/package.rb b/app/models/package.rb index 22bd30d64..b81631b57 100644 --- a/app/models/package.rb +++ b/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 diff --git a/spec/models/package_spec.rb b/spec/models/package_spec.rb new file mode 100644 index 000000000..87eb74829 --- /dev/null +++ b/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