From 9ceffce70f2e95c11b13a0528f0edafb35a418cf Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Tue, 7 Jul 2026 00:29:14 +0200 Subject: [PATCH] Fix local batch file imports to enforce file access ACLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⓘ This tightens local server-side batch file execution in the Users and Groups module. Both user and group batch endpoints now read local batch files through a shared helper that enforces the module `batchdir` ACL, the user’s global file chooser ACL (`root` / `otherdirs`), and the configured `fileunix` read identity. Existing ACLs remain compatible by treating a missing `batchdir` as `/`, while an explicitly blank value denies local server batch files. Regression coverage was added for allowed paths, denied paths, `otherdirs`, missing/blank `batchdir`, symlink escape rejection, and successful local batch reads. ---- Reproduction / verification steps: 1. Create a constrained Webmin user with access to the `Users and Groups` module. 2. Set the user's Global ACL: - Root directory for file browser: `/home/user1` - Other directories to allow: empty - Browse as Unix user: `nobody` 3. Set the user's `Users and Groups` module ACL: - Can view batch file form?: `Yes` - Batch files must be under directory: `/` 4. Create test files on the Webmin host: ```sh sudo mkdir -p /home/user1 echo ROOT_SECRET_MARKER | sudo tee /root/batch-secret.txt >/dev/null sudo chown root:root /root/batch-secret.txt sudo chmod 600 /root/batch-secret.txt echo ALLOWED_BATCH_MARKER | sudo tee /home/user1/allowed.batch >/dev/null sudo chmod 644 /home/user1/allowed.batch sudo ln -sf /root/batch-secret.txt /home/user1/link-secret.batch echo ROOT_ONLY_ALLOWED_MARKER | sudo tee /home/user1/root-only.batch >/dev/null sudo chown root:root /home/user1/root-only.batch sudo chmod 600 /home/user1/root-only.batch ``` 5. Log in to Webmin as the constrained user and request: ```text /useradmin/batch_exec.cgi?source=1&local=/root/batch-secret.txt /useradmin/gbatch_exec.cgi?source=1&local=/root/batch-secret.txt ``` Expected fixed result: ```text Local file location is not allowed ``` 6. Request an allowed readable file: ```text /useradmin/batch_exec.cgi?source=1&local=/home/user1/allowed.batch /useradmin/gbatch_exec.cgi?source=1&local=/home/user1/allowed.batch ``` Expected fixed result: ```text Invalid action at line 1 : ALLOWED_BATCH_MARKER ``` 7. Request a symlink inside the allowed directory that points outside it: ```text /useradmin/batch_exec.cgi?source=1&local=/home/user1/link-secret.batch /useradmin/gbatch_exec.cgi?source=1&local=/home/user1/link-secret.batch ``` Expected fixed result: ```text Local file location is not allowed ``` 8. Request a root-only file inside the allowed directory: ```text /useradmin/batch_exec.cgi?source=1&local=/home/user1/root-only.batch /useradmin/gbatch_exec.cgi?source=1&local=/home/user1/root-only.batch ``` Expected fixed result: ```text Local file not found ``` The fixed behavior confirms that local batch file imports now enforce the configured global file ACL, reject symlink escapes, and read files using the configured Unix identity instead of the privileged Webmin process. --- CHANGELOG.md | 1 + useradmin/batch_exec.cgi | 9 +-- useradmin/gbatch_exec.cgi | 9 +-- useradmin/t/run-tests.t | 114 ++++++++++++++++++++++++++++++++++++++ useradmin/user-lib.pl | 71 ++++++++++++++++++++++++ 5 files changed, 188 insertions(+), 16 deletions(-) create mode 100644 useradmin/t/run-tests.t diff --git a/CHANGELOG.md b/CHANGELOG.md index 62ab467bb..ddd36f575 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * Fix to recognize hex numeric HTML entities to work in various elements * Fix `patch` sub-command to reload Webmin instead of restarting * Fix SSL certificate and TCP monitors to report transient connection failures as down, and SSL check timeouts as timed out, rather than uninstalled +* Fix local batch file imports to enforce file access ACLs in Users and Groups module #### 2.651 (June 28, 2026) * Fix Certbot-backed certificate requests and renewals to correctly parse PEM paths after issuance diff --git a/useradmin/batch_exec.cgi b/useradmin/batch_exec.cgi index e15d0c6f6..49e61184f 100755 --- a/useradmin/batch_exec.cgi +++ b/useradmin/batch_exec.cgi @@ -15,13 +15,7 @@ if ($in{'source'} == 0) { $data =~ /\S/ || &error($text{'batch_efile'}); } elsif ($in{'source'} == 1) { - &is_under_directory($access{'batchdir'}, $in{'local'}) || - &error($text{'batch_elocaldir'}); - open(LOCAL, "<$in{'local'}") || &error($text{'batch_elocal'}); - while() { - $data .= $_; - } - close(LOCAL); + $data = &read_batch_local_file($in{'local'}); } elsif ($in{'source'} == 2) { $data = $in{'text'}; @@ -669,4 +663,3 @@ foreach my $f ('min', 'max', 'warn', 'inactive', 'expire', 'change') { } return undef; } - diff --git a/useradmin/gbatch_exec.cgi b/useradmin/gbatch_exec.cgi index 9a45ebda0..bd36447d2 100755 --- a/useradmin/gbatch_exec.cgi +++ b/useradmin/gbatch_exec.cgi @@ -14,13 +14,7 @@ if ($in{'source'} == 0) { $data =~ /\S/ || &error($text{'batch_efile'}); } elsif ($in{'source'} == 1) { - &is_under_directory($access{'batchdir'}, $in{'local'}) || - &error($text{'batch_elocaldir'}); - open(LOCAL, "<$in{'local'}") || &error($text{'batch_elocal'}); - while() { - $data .= $_; - } - close(LOCAL); + $data = &read_batch_local_file($in{'local'}); } elsif ($in{'source'} == 2) { $data = $in{'text'}; @@ -333,4 +327,3 @@ if ($_[1] && !$access{'ggid'} && $_[1]->{'gid'} != $_[0]->{'gid'}) { } return undef; } - diff --git a/useradmin/t/run-tests.t b/useradmin/t/run-tests.t new file mode 100644 index 000000000..4cfced6c6 --- /dev/null +++ b/useradmin/t/run-tests.t @@ -0,0 +1,114 @@ +#!/usr/bin/perl +use strict; +use warnings; +use Test::More; +use Cwd qw(abs_path); +use File::Basename qw(dirname); +use File::Path qw(make_path); +use File::Temp qw(tempdir); + +my $bindir = dirname(__FILE__); +my $rootdir = abs_path("$bindir/../..") or die "rootdir: $!"; +my $confdir = tempdir(CLEANUP => 1); +my $vardir = tempdir(CLEANUP => 1); +my $remote_user = getpwuid($<) || 'root'; + +sub write_text +{ +my ($file, $text) = @_; +open(my $fh, ">", $file) or die "write $file: $!"; +print $fh $text; +close($fh) or die "close $file: $!"; +} + +write_text("$confdir/config", "os_type=generic-linux\nos_version=0\n"); +write_text("$confdir/var-path", "$vardir\n"); +write_text("$confdir/miniserv.conf", + "root=$rootdir\nuserfile=$confdir/miniserv.users\n"); +write_text("$confdir/miniserv.users", ""); +make_path("$confdir/useradmin"); + +$ENV{'WEBMIN_CONFIG'} = $confdir; +$ENV{'WEBMIN_VAR'} = $vardir; +$ENV{'MINISERV_CONFIG'} = "$confdir/miniserv.conf"; +$ENV{'FOREIGN_MODULE_NAME'} = 'useradmin'; +$ENV{'FOREIGN_ROOT_DIRECTORY'} = $rootdir; +$ENV{'REMOTE_USER'} = $remote_user; +$ENV{'BASE_REMOTE_USER'} = $remote_user; + +chdir("$rootdir/useradmin") or die "chdir useradmin: $!"; +require "$rootdir/useradmin/user-lib.pl"; + +our %access; + +sub clear_acl_cache +{ +no warnings 'once'; +undef(%main::read_file_cache); +undef(%main::read_file_missing); +undef(%main::read_file_cache_time); +} + +sub write_global_acl +{ +my (%opts) = @_; +my $text = ""; +foreach my $k (sort keys %opts) { + $text .= "$k=$opts{$k}\n"; + } +write_text("$confdir/$remote_user.acl", $text); +clear_acl_cache(); +} + +my $tmp = tempdir(CLEANUP => 1); +my $allowed = "$tmp/allowed"; +my $other = "$tmp/other"; +my $outside = "$tmp/outside"; +make_path($allowed, $other, $outside); +write_text("$allowed/batch.txt", "batch\n"); +write_text("$other/also-batch.txt", "other\n"); +write_text("$outside/secret.txt", "secret\n"); + +write_global_acl( + root => $allowed, + otherdirs => $other, + fileunix => $remote_user, + ); + +subtest 'local batch file path ACLs' => sub { + %access = ( 'batchdir' => $allowed ); + ok(can_read_batch_local_file("$allowed/batch.txt"), + 'file under batchdir and global root is allowed'); + ok(!can_read_batch_local_file("$other/also-batch.txt"), + 'global otherdirs alone cannot bypass narrower batchdir'); + ok(!can_read_batch_local_file("$outside/secret.txt"), + 'outside file is rejected'); + ok(!can_read_batch_local_file("relative/batch.txt"), + 'relative paths are rejected'); + + %access = ( 'batchdir' => "" ); + ok(!can_read_batch_local_file("$allowed/batch.txt"), + 'blank batchdir denies local server files'); + + %access = ( ); + ok(can_read_batch_local_file("$allowed/batch.txt"), + 'missing batchdir falls back to / for old ACL compatibility'); + + %access = ( 'batchdir' => $tmp ); + ok(can_read_batch_local_file("$other/also-batch.txt"), + 'batchdir plus global otherdirs allows second configured tree'); + }; + +SKIP: { + skip('symlinks are unavailable', 1) + if !symlink("$outside/secret.txt", "$allowed/link.txt"); + %access = ( 'batchdir' => $allowed ); + ok(!can_read_batch_local_file("$allowed/link.txt"), + 'symlink escaping batchdir/global root is rejected'); + } + +%access = ( 'batchdir' => $allowed ); +is(read_batch_local_file("$allowed/batch.txt"), "batch\n", + 'allowed batch file is read'); + +done_testing(); diff --git a/useradmin/user-lib.pl b/useradmin/user-lib.pl index 6a43e4faf..d9a2c81cb 100755 --- a/useradmin/user-lib.pl +++ b/useradmin/user-lib.pl @@ -63,6 +63,77 @@ $password_file_cache{$file} = $rv; return $rv; } +# read_batch_local_file(file) +# Returns the contents of a server-side batch file, after checking both the +# Users and Groups batch directory ACL and the global file chooser ACL. +sub read_batch_local_file +{ +my ($file) = @_; +&can_read_batch_local_file($file) || &error($text{'batch_elocaldir'}); +my $data; +my ($fileunix, $fileunix_err) = &batch_acl_fileunix(); +if ($fileunix_err) { + &error($text{'batch_elocal'}); + } +if ($fileunix && &supports_users() && $< == 0) { + $data = &eval_as_unix_user($fileunix, + sub { return &read_file_contents($file); }); + } +else { + $data = &read_file_contents($file); + } +defined($data) || &error($text{'batch_elocal'}); +return $data; +} + +# can_read_batch_local_file(file) +# Returns 1 if a local batch file path is allowed by all relevant ACLs. +sub can_read_batch_local_file +{ +my ($file) = @_; +return 0 if ($file !~ /^\//); +my $batchdir = defined($access{'batchdir'}) ? $access{'batchdir'} : "/"; +return 0 if ($batchdir eq ""); +return 0 if (!&is_under_directory($batchdir, $file)); +return &is_under_global_file_acl($file); +} + +# is_under_global_file_acl(file) +# Returns 1 if a file is under the Webmin user's global file chooser ACL. +sub is_under_global_file_acl +{ +my ($file) = @_; +my %gaccess = &get_module_acl($base_remote_user, ""); +my @uinfo = getpwnam($remote_user); +my $rootdir; +if (!$gaccess{'root'}) { + $rootdir = $uinfo[7] ? $uinfo[7] : "/"; + } +else { + $rootdir = $gaccess{'root'}; + $rootdir =~ s/^\~/$uinfo[7]/; + } +foreach my $dir ($rootdir, split(/\t+/, $gaccess{'otherdirs'})) { + next if ($dir eq ""); + return 1 if (&is_under_directory($dir, $file)); + } +return 0; +} + +# batch_acl_fileunix() +# Returns the Unix user to read local batch files as, or an error flag. +sub batch_acl_fileunix +{ +my %gaccess = &get_module_acl($base_remote_user, ""); +my $fileunix = $gaccess{'fileunix'} || $remote_user; +my @uinfo = getpwnam($fileunix); +if (!@uinfo && !$gaccess{'fileunix'}) { + $fileunix = "nobody"; + @uinfo = getpwnam($fileunix); + } +return @uinfo ? ($fileunix, 0) : (undef, 1); +} + =head2 list_users Returns an array of hash references, each containing info about one user. Each