From 12ec9b069ee50a4c36fa0e4d5ba7bc98c2f5e0dd Mon Sep 17 00:00:00 2001 From: Evgeni Golov Date: Wed, 20 Sep 2023 09:39:46 +0200 Subject: [PATCH] Fixes #36759 - only call allowed transpilers CVE-2022-3874: OS command injection via ct_command and fcct_command Instead of allowing to call *any* command by changing a setting, only allow specific paths to ct/fcct. If the user needs a different path, they can set it via settings.yaml. --- .../foreman/settings/provisioning.rb | 31 ++++++++++++++----- .../renderer/scope/macros/transpilers.rb | 6 ++-- ...093000_move_ct_command_into_own_setting.rb | 23 ++++++++++++++ test/fixtures/settings.yml | 8 ++--- .../renderer/scope/macros/transpilers_test.rb | 4 +-- 5 files changed, 56 insertions(+), 16 deletions(-) create mode 100644 db/migrate/20230920093000_move_ct_command_into_own_setting.rb diff --git a/app/registries/foreman/settings/provisioning.rb b/app/registries/foreman/settings/provisioning.rb index 2be758bb8bad..caf4d10f7a45 100644 --- a/app/registries/foreman/settings/provisioning.rb +++ b/app/registries/foreman/settings/provisioning.rb @@ -2,6 +2,9 @@ Foreman::SettingManager.define(:foreman) do category(:provisioning, N_('Provisioning')) do + CT_LOCATIONS = %w(/usr/bin/ct /usr/local/bin/ct) + FCCT_LOCATIONS = %w(/usr/bin/fcct /usr/local/bin/fcct) + owner_select = proc do [{:name => _("Users"), :class => 'user', :scope => 'visible', :value_method => 'id_and_type', :text_method => 'login'}, {:name => _("Usergroups"), :class => 'usergroup', :scope => 'visible', :value_method => 'id_and_type', :text_method => 'name'}] @@ -119,16 +122,28 @@ description: N_("Default 'Host initial configuration' template, automatically assigned when a new operating system is created"), default: 'Linux host_init_config default', full_name: N_("Default 'Host initial configuration' template")) - setting('ct_command', + setting('ct_location', + type: :string, + description: N_("Full path to CoreOS transpiler (ct)"), + default: CT_LOCATIONS.first, + full_name: N_("CoreOS Transpiler Command"), + collection: proc { CT_LOCATIONS.zip(CT_LOCATIONS).to_h }) + setting('ct_arguments', type: :array, - description: N_("Full path to CoreOS transpiler (ct) with arguments as an comma-separated array"), - default: [which('ct'), '--pretty', '--files-dir', Rails.root.join('config', 'ct').to_s], - full_name: N_("CoreOS Transpiler Command")) - setting('fcct_command', + description: N_("CoreOS transpiler (ct) arguments as an comma-separated array"), + default: ['--pretty', '--files-dir', Rails.root.join('config', 'ct').to_s], + full_name: N_("CoreOS Transpiler Command Arguments")) + setting('fcct_location', + type: :string, + description: N_("Full path to Fedora CoreOS transpiler (fcct)"), + default: FCCT_LOCATIONS.first, + full_name: N_("Fedora CoreOS Transpiler Command"), + collection: proc { FCCT_LOCATIONS.zip(FCCT_LOCATIONS).to_h }) + setting('fcct_arguments', type: :array, - description: N_("Full path to Fedora CoreOS transpiler (fcct) with arguments as an comma-separated array"), - default: [which('fcct'), '--pretty', '--files-dir', Rails.root.join('config', 'ct').to_s], - full_name: N_("Fedora CoreOS Transpiler Command")) + description: N_("Fedora CoreOS transpiler (fcct) arguments as an comma-separated array"), + default: ['--pretty', '--files-dir', Rails.root.join('config', 'ct').to_s], + full_name: N_("Fedora CoreOS Transpiler Command Arguments")) # We have following loop twice to keep the historical order. # TODO: First resolve the correct order and then optimize this loop. diff --git a/app/services/foreman/renderer/scope/macros/transpilers.rb b/app/services/foreman/renderer/scope/macros/transpilers.rb index 3682842debba..8d808f77ce83 100644 --- a/app/services/foreman/renderer/scope/macros/transpilers.rb +++ b/app/services/foreman/renderer/scope/macros/transpilers.rb @@ -15,7 +15,8 @@ module Transpilers end def transpile_coreos_linux_config(input, validate_input = true, validate_output = false) YAML.safe_load(input) if validate_input - result = Foreman::CommandRunner.new(Setting[:ct_command], input).run! + ct_command = [Setting[:ct_location]] + Setting[:ct_arguments] + result = Foreman::CommandRunner.new(ct_command, input).run! JSON.parse(result) if validate_output result end @@ -29,7 +30,8 @@ def transpile_coreos_linux_config(input, validate_input = true, validate_output example "transpile_coreos_linux_config(\"---\\nyaml: blah\") #=> JSON" end def transpile_fedora_coreos_config(input) - Foreman::CommandRunner.new(Setting[:fcct_command], input).run! + fcct_command = [Setting[:fcct_location]] + Setting[:fcct_arguments] + Foreman::CommandRunner.new(fcct_command, input).run! end end end diff --git a/db/migrate/20230920093000_move_ct_command_into_own_setting.rb b/db/migrate/20230920093000_move_ct_command_into_own_setting.rb new file mode 100644 index 000000000000..f5cfd71f613f --- /dev/null +++ b/db/migrate/20230920093000_move_ct_command_into_own_setting.rb @@ -0,0 +1,23 @@ +class MoveCtCommandIntoOwnSetting < ActiveRecord::Migration[6.1] + def up + ['ct', 'fcct'].each do |setting_prefix| + setting = Setting.find_by(name: "#{setting_prefix}_command") + if setting + setting.value = setting.value.drop(1) + setting.name = "#{setting_prefix}_arguments" + setting.save + end + end + end + + def down + ['ct', 'fcct'].each do |setting_prefix| + setting = Setting.find_by(name: "#{setting_prefix}_arguments") + if setting + setting.value = ["/usr/bin/#{setting_prefix}"] + setting.value + setting.name = "#{setting_prefix}_command" + setting.save + end + end + end +end diff --git a/test/fixtures/settings.yml b/test/fixtures/settings.yml index a807f021fe75..4c2b5aa6b1de 100644 --- a/test/fixtures/settings.yml +++ b/test/fixtures/settings.yml @@ -3,8 +3,8 @@ attributes66: name: delivery_method value: :test attribute97: - name: ct_command - value: "['/usr/bin/cat']" + name: ct_location + value: "/usr/bin/cat" attribute98: - name: fcct_command - value: "['/usr/bin/cat']" + name: fcct_location + value: "/usr/bin/cat" diff --git a/test/unit/foreman/renderer/scope/macros/transpilers_test.rb b/test/unit/foreman/renderer/scope/macros/transpilers_test.rb index 4ee261710414..c8abbba98b1d 100644 --- a/test/unit/foreman/renderer/scope/macros/transpilers_test.rb +++ b/test/unit/foreman/renderer/scope/macros/transpilers_test.rb @@ -19,7 +19,7 @@ class TranspilersTest < ActiveSupport::TestCase describe '#transpile_coreos_linux_config' do test 'should call the transpiler' do - Foreman::CommandRunner.any_instance.expects(:capture3).with(Setting[:ct_command], "IGNORE") + Foreman::CommandRunner.any_instance.expects(:capture3).with(Setting[:ct_location], "IGNORE") .returns(["JSON", "", @success]) assert_equal "JSON", @scope.transpile_coreos_linux_config("IGNORE") @@ -28,7 +28,7 @@ class TranspilersTest < ActiveSupport::TestCase describe '#transpile_fedora_coreos_config' do test 'should call the transpiler' do - Foreman::CommandRunner.any_instance.expects(:capture3).with(Setting[:fcct_command], "IGNORE") + Foreman::CommandRunner.any_instance.expects(:capture3).with(Setting[:fcct_location], "IGNORE") .returns(["JSON", "", @success]) assert_equal "JSON", @scope.transpile_fedora_coreos_config("IGNORE")