From 7e8366bcb825176edb23c4b04042dbe5171db703 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Thu, 28 May 2026 21:21:56 +0200 Subject: [PATCH] Fix to use direct ACL checks in GRUB 2 module https://github.com/webmin/webmin/pull/2743#discussion_r3319948219 --- grub2/acl_security.pl | 8 ++++---- grub2/custom_action.cgi | 3 ++- grub2/edit_custom.cgi | 3 ++- grub2/edit_defaults.cgi | 3 ++- grub2/edit_install.cgi | 3 ++- grub2/edit_manual.cgi | 3 ++- grub2/edit_security.cgi | 3 ++- grub2/edit_theme.cgi | 3 ++- grub2/generate.cgi | 3 ++- grub2/grub2-lib.pl | 45 ----------------------------------------- grub2/index.cgi | 6 +++--- grub2/install.cgi | 3 ++- grub2/reboot_once.cgi | 3 ++- grub2/save_custom.cgi | 3 ++- grub2/save_defaults.cgi | 3 ++- grub2/save_manual.cgi | 3 ++- grub2/save_security.cgi | 3 ++- grub2/save_theme.cgi | 3 ++- grub2/set_default.cgi | 3 ++- grub2/status.cgi | 9 ++++----- grub2/t/run-tests.t | 19 ++++++++--------- 21 files changed, 51 insertions(+), 84 deletions(-) diff --git a/grub2/acl_security.pl b/grub2/acl_security.pl index 8635b18f6..c7007c921 100644 --- a/grub2/acl_security.pl +++ b/grub2/acl_security.pl @@ -14,18 +14,18 @@ sub acl_security_form my ($o) = @_; # View permissions print &ui_table_row($text{'acl_view'}, - &ui_yesno_radio("view", &grub2_check_acl("view", $o)), 3); + &ui_yesno_radio("view", $o->{'view'}), 3); print &ui_table_hr(); # Change permissions modify GRUB configuration without granting install rights. foreach my $a (qw(edit security apply runtime)) { print &ui_table_row($text{'acl_'.$a}, - &ui_yesno_radio($a, &grub2_check_acl($a, $o)), 3); + &ui_yesno_radio($a, $o->{$a}), 3); } print &ui_table_hr(); # Admin permissions expose direct file editing, backup, and boot-loader install. foreach my $a (qw(manual install backup)) { print &ui_table_row($text{'acl_'.$a}, - &ui_yesno_radio($a, &grub2_check_acl($a, $o)), 3); + &ui_yesno_radio($a, $o->{$a}), 3); } } @@ -34,7 +34,7 @@ foreach my $a (qw(manual install backup)) { sub acl_security_save { my ($o) = @_; -foreach my $a (&grub2_acl_keys()) { +foreach my $a (qw(view edit security apply runtime manual install backup)) { # Missing checkbox/radio values fall back to denied. $o->{$a} = $in{$a} || 0; } diff --git a/grub2/custom_action.cgi b/grub2/custom_action.cgi index c173a1195..8daa7ddd5 100755 --- a/grub2/custom_action.cgi +++ b/grub2/custom_action.cgi @@ -9,7 +9,8 @@ our (%in, %text); &ReadParse(); &error_setup($text{'custom_err'}); -&grub2_assert_acl('manual'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pmanual'}") if (!$access{'manual'}); # Per-row up/down links post an index and direction directly. if (defined($in{'idx'}) || defined($in{'dir'})) { diff --git a/grub2/edit_custom.cgi b/grub2/edit_custom.cgi index 312491358..72cf52794 100755 --- a/grub2/edit_custom.cgi +++ b/grub2/edit_custom.cgi @@ -9,7 +9,8 @@ our (%in, %text); &ReadParse(); &error_setup($text{'custom_err'}); -&grub2_assert_acl('manual'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pmanual'}") if (!$access{'manual'}); my $is_new = !defined($in{'idx'}) || $in{'idx'} eq ''; my ($title, $id, $body) = diff --git a/grub2/edit_defaults.cgi b/grub2/edit_defaults.cgi index d29d5b7ed..1f7a45a5a 100755 --- a/grub2/edit_defaults.cgi +++ b/grub2/edit_defaults.cgi @@ -9,7 +9,8 @@ our (%text); &ReadParse(); &error_setup($text{'defaults_err'}); -&grub2_assert_acl('edit'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pedit'}") if (!$access{'edit'}); my $parsed = &read_grub_defaults(); my $values = $parsed->{'values'}; diff --git a/grub2/edit_install.cgi b/grub2/edit_install.cgi index cec642f4e..164600ba6 100755 --- a/grub2/edit_install.cgi +++ b/grub2/edit_install.cgi @@ -9,7 +9,8 @@ our (%text); &ReadParse(); &error_setup($text{'install_err'}); -&grub2_assert_acl('install'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pinstall'}") if (!$access{'install'}); my $cmd = &grub2_command('install_cmd'); my $platform = &grub2_default_platform_target(); diff --git a/grub2/edit_manual.cgi b/grub2/edit_manual.cgi index 593809640..6b6c1a4b4 100755 --- a/grub2/edit_manual.cgi +++ b/grub2/edit_manual.cgi @@ -9,7 +9,8 @@ our (%in, %text); &ReadParse(); &error_setup($text{'manual_err'}); -&grub2_assert_acl('manual'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pmanual'}") if (!$access{'manual'}); # The manual editor is restricted to discovered GRUB-related files only. my @files = &grub2_manual_files(); diff --git a/grub2/edit_security.cgi b/grub2/edit_security.cgi index cbe059987..107158cd0 100755 --- a/grub2/edit_security.cgi +++ b/grub2/edit_security.cgi @@ -9,7 +9,8 @@ our (%text); &ReadParse(); &error_setup($text{'security_err'}); -&grub2_assert_acl('security'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_psecurity'}") if (!$access{'security'}); my $state = &grub2_read_security_config(); diff --git a/grub2/edit_theme.cgi b/grub2/edit_theme.cgi index 5d1e9dd70..145e63c84 100755 --- a/grub2/edit_theme.cgi +++ b/grub2/edit_theme.cgi @@ -9,7 +9,8 @@ our (%text); &ReadParse(); &error_setup($text{'theme_err'}); -&grub2_assert_acl('edit'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pedit'}") if (!$access{'edit'}); my $parsed = &read_grub_defaults(); my $values = $parsed->{'values'}; diff --git a/grub2/generate.cgi b/grub2/generate.cgi index 6df900154..55395b738 100755 --- a/grub2/generate.cgi +++ b/grub2/generate.cgi @@ -9,7 +9,8 @@ our (%in, %text); &ReadParse(); &error_setup($text{'generate_err'}); -&grub2_assert_acl('apply'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_papply'}") if (!$access{'apply'}); my $return_url = $in{'redir'} || "index.cgi"; my ($current_step, $failed_printed, $failure_output_shown, $command_output) = diff --git a/grub2/grub2-lib.pl b/grub2/grub2-lib.pl index 7712bcb3d..1510b3571 100644 --- a/grub2/grub2-lib.pl +++ b/grub2/grub2-lib.pl @@ -21,51 +21,6 @@ $grub2_config_change_flag = $module_var_directory."/config-flag"; $grub2_generate_time_flag = $module_var_directory."/generate-flag"; &load_grub2_defaults(); -# grub2_acl_keys() -# Returns the supported GRUB 2 ACL capabilities. -sub grub2_acl_keys -{ -return qw(view edit security apply runtime manual install backup); -} - -# grub2_effective_acl([&raw-acl]) -# Returns normalized ACL settings for a supplied ACL hash or current user. -sub grub2_effective_acl -{ -my ($rawacl) = @_; -my %raw = $rawacl ? %$rawacl : &get_module_acl(); -return map { $_ => $raw{$_} ? 1 : 0 } &grub2_acl_keys(); -} - -# grub2_check_acl(action, [&raw-acl]) -# Returns true when an effective ACL permits the requested action. -sub grub2_check_acl -{ -my ($action, $rawacl) = @_; -my %acl = &grub2_effective_acl($rawacl); -return $acl{$action} ? 1 : 0; -} - -# grub2_assert_acl(action) -# Fails if the current Webmin user cannot perform an action. -sub grub2_assert_acl -{ -my ($action) = @_; -&grub2_check_acl($action) || - &error("$text{'eacl_np'} $text{'eacl_p'.$action}"); -} - -# grub2_can_enter_module(&acl) -# Returns true if a user has at least one useful module capability. -sub grub2_can_enter_module -{ -my ($acl) = @_; -foreach my $a (&grub2_acl_keys()) { - return 1 if ($acl->{$a}); - } -return 0; -} - # grub2_mark_regenerate_needed() # Updates the flag indicating that grub.cfg needs to be regenerated. sub grub2_mark_regenerate_needed diff --git a/grub2/index.cgi b/grub2/index.cgi index 0dd3d4b3c..a011cc0db 100755 --- a/grub2/index.cgi +++ b/grub2/index.cgi @@ -5,13 +5,13 @@ use strict; use warnings; require './grub2-lib.pl'; ## no critic -our (%in, %text); +our (%in, %text, %access); our $module_name; our $grub2_formno = 0; &ReadParse(); &error_setup($text{'acl_ecannot'}); -my %access = &grub2_effective_acl(); +%access = &get_module_acl(); &error("$text{'eacl_np'} $text{'eacl_pview'}") if (!&can_use_index(\%access)); @@ -361,7 +361,7 @@ if ($direct_file) { else { $html = &ui_tag('tt', &html_escape($file)); } -if (&grub2_check_acl('manual') && &grub2_manual_file($file)) { +if ($access{'manual'} && &grub2_manual_file($file)) { # The manual editor repeats its allowlist check on entry. $html = &ui_tag('a', $html, { 'href' => "edit_manual.cgi?file=".&urlize($file), diff --git a/grub2/install.cgi b/grub2/install.cgi index afb7cdbb6..7f01f7c81 100755 --- a/grub2/install.cgi +++ b/grub2/install.cgi @@ -9,7 +9,8 @@ our (%in, %text); &ReadParse(); &error_setup($text{'install_err'}); -&grub2_assert_acl('install'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pinstall'}") if (!$access{'install'}); # Trim text fields before validation so shell arguments are deterministic. foreach my $field (qw(target efi_dir platform directory boot_directory diff --git a/grub2/reboot_once.cgi b/grub2/reboot_once.cgi index 2849c7ce3..1d68f6fe3 100755 --- a/grub2/reboot_once.cgi +++ b/grub2/reboot_once.cgi @@ -9,7 +9,8 @@ our (%in, %text); &ReadParse(); &error_setup($text{'runtime_err'}); -&grub2_assert_acl('runtime'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pruntime'}") if (!$access{'runtime'}); # Runtime commands operate on the parsed generated menu index shown on index.cgi. my $entry = &grub2_entry_by_index($in{'idx'}); diff --git a/grub2/save_custom.cgi b/grub2/save_custom.cgi index 79b5e6f4d..b13a68573 100755 --- a/grub2/save_custom.cgi +++ b/grub2/save_custom.cgi @@ -9,7 +9,8 @@ our (%in, %text); &ReadParse(); &error_setup($text{'custom_err'}); -&grub2_assert_acl('manual'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pmanual'}") if (!$access{'manual'}); # A missing index means create; a present one must address a parsed entry. my $idx = defined($in{'idx'}) && $in{'idx'} ne '' ? $in{'idx'} : undef; diff --git a/grub2/save_defaults.cgi b/grub2/save_defaults.cgi index c40de4dd1..d3b6d434c 100755 --- a/grub2/save_defaults.cgi +++ b/grub2/save_defaults.cgi @@ -9,7 +9,8 @@ our (%in, %text); &ReadParse(); &error_setup($text{'defaults_err'}); -&grub2_assert_acl('edit'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pedit'}") if (!$access{'edit'}); # Capture both the current defaults file and generated entries for validation. my $current = &read_grub_defaults(); diff --git a/grub2/save_manual.cgi b/grub2/save_manual.cgi index 48e0d3df0..44b38b36b 100755 --- a/grub2/save_manual.cgi +++ b/grub2/save_manual.cgi @@ -9,7 +9,8 @@ our (%in, %text); &ReadParseMime(); &error_setup($text{'manual_err'}); -&grub2_assert_acl('manual'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pmanual'}") if (!$access{'manual'}); # Re-check the allowlist on save; the form value is not trusted. my $file = $in{'file'} || ''; diff --git a/grub2/save_security.cgi b/grub2/save_security.cgi index c1d0dc0b8..6c18160c2 100755 --- a/grub2/save_security.cgi +++ b/grub2/save_security.cgi @@ -9,7 +9,8 @@ our (%in, %text); &ReadParse(); &error_setup($text{'security_err'}); -&grub2_assert_acl('security'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_psecurity'}") if (!$access{'security'}); # The enable flag is deliberately boolean; all other fields normalize below. defined($in{'enabled'}) && $in{'enabled'} =~ /^[01]\z/ || diff --git a/grub2/save_theme.cgi b/grub2/save_theme.cgi index aa867ec48..b338a9c46 100755 --- a/grub2/save_theme.cgi +++ b/grub2/save_theme.cgi @@ -9,7 +9,8 @@ our (%in, %text); &ReadParse(); &error_setup($text{'theme_err'}); -&grub2_assert_acl('edit'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pedit'}") if (!$access{'edit'}); my $parsed = &read_grub_defaults(); my $current_values = $parsed->{'values'}; diff --git a/grub2/set_default.cgi b/grub2/set_default.cgi index 2ce6de4d9..2069a772c 100755 --- a/grub2/set_default.cgi +++ b/grub2/set_default.cgi @@ -9,7 +9,8 @@ our (%in, %text); &ReadParse(); &error_setup($text{'runtime_err'}); -&grub2_assert_acl('runtime'); +my %access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pruntime'}") if (!$access{'runtime'}); # Runtime commands operate on the parsed generated menu index shown on index.cgi. my $entry = &grub2_entry_by_index($in{'idx'}); diff --git a/grub2/status.cgi b/grub2/status.cgi index 1ae492853..c634468a8 100755 --- a/grub2/status.cgi +++ b/grub2/status.cgi @@ -5,12 +5,11 @@ use strict; use warnings; require './grub2-lib.pl'; ## no critic -our (%text); +our (%text, %access); &error_setup($text{'acl_ecannot'}); -&grub2_assert_acl('view'); - -my %access = &grub2_effective_acl(); +%access = &get_module_acl(); +&error("$text{'eacl_np'} $text{'eacl_pview'}") if (!$access{'view'}); &ui_print_header(undef, $text{'status_title'}, ""); # Missing-install output mirrors index.cgi but keeps this page read-only. @@ -194,7 +193,7 @@ return &ui_tag('tt', &html_escape($raw)).' '.$text{'index_not_readable'}; sub manual_path_link { my ($path, $html) = @_; -return $html if (!&grub2_check_acl('manual') || !&grub2_manual_file($path)); +return $html if (!$access{'manual'} || !&grub2_manual_file($path)); # Link only allowlisted paths; generated grub.cfg remains informational. return &ui_tag('a', $html, { 'href' => "edit_manual.cgi?file=".&urlize($path), diff --git a/grub2/t/run-tests.t b/grub2/t/run-tests.t index 7a7fc5d81..fc7b3078b 100644 --- a/grub2/t/run-tests.t +++ b/grub2/t/run-tests.t @@ -101,14 +101,6 @@ $config{'shell_cmd'} = '/bin/sh'; mkdir $config{'grub_dir'} or die "mkdir grub.d: $!"; mkdir $bls_dir or die "mkdir bls_dir: $!"; -{ - my %acl = grub2_effective_acl({ view => 1, edit => 0 }); - ok($acl{'view'}, 'view ACL is enabled'); - ok(!$acl{'edit'}, 'edit ACL is disabled'); - ok(!grub2_can_enter_module({ map { $_ => 0 } grub2_acl_keys() }), - 'empty ACL cannot enter module'); -} - ok(!grub2_needs_regenerate(), 'fresh module does not need regeneration'); grub2_mark_regenerate_needed(); ok(grub2_needs_regenerate(), 'config change flag requires regeneration'); @@ -191,7 +183,12 @@ like($acl_source, 'ACL view permission is rendered as a standalone row'); unlike($acl_source, qr/acl_section_view/, 'ACL view permission does not use a one-row section heading'); -like($status_source, qr/grub2_assert_acl\('view'\)/, +like($acl_source, qr/&ui_yesno_radio\("view", \$o->\{'view'\}\)/, + 'ACL editor uses the supplied view ACL value directly'); +unlike($acl_source, qr/grub2_(?:check|effective)_acl/, + 'ACL editor does not normalize supplied ACL values'); +like($status_source, + qr/%access = &get_module_acl\(\).*?\$access\{'view'\}/s, 'status page enforces view ACL directly'); like($status_source, qr/index_boot_mode.*boot_mode_cell/s, 'status page displays detected boot mode'); @@ -242,7 +239,7 @@ like($status_source, qr/sub path_cell\b.*manual_path_link\(\$path, &ui_tag\('tt'/s, 'status page renders paths with tt tags'); like($status_source, - qr/sub manual_path_link\b.*grub2_check_acl\('manual'\).*grub2_manual_file\(\$path\).*edit_manual\.cgi\?file=/s, + qr/sub manual_path_link\b.*\$access\{'manual'\}.*grub2_manual_file\(\$path\).*edit_manual\.cgi\?file=/s, 'status page links allowlisted paths to the manual editor when permitted'); like($status_source, qr/sub status_table_row\b.*hlink\(\$label, \$help\)/s, 'status page labels use contextual help links'); @@ -324,7 +321,7 @@ like($index_source, qr/sub entry_source_detail_line\b.*index_col_file.*index_col_generator.*ui_tag\('a'.*edit_manual\.cgi\?file=/ms, 'entry details label generator scripts and link direct entry files to the manual editor'); like($index_source, - qr/sub entry_source_detail_line\b.*else \{\s*\$html = &ui_tag\('tt', &html_escape\(\$file\)\).*?grub2_check_acl\('manual'\).*?edit_manual\.cgi\?file=/s, + qr/sub entry_source_detail_line\b.*else \{\s*\$html = &ui_tag\('tt', &html_escape\(\$file\)\).*?\$access\{'manual'\}.*?edit_manual\.cgi\?file=/s, 'entry details link editable generator scripts to the manual editor'); like($index_source, qr/sub entry_source_detail_line\b.*entry_file_display_name.*'title'\s*=>\s*\$file.*edit_manual\.cgi\?file=/s,