From 29f595dbdf08e40783d1c2a22e015390daaa69d9 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Sun, 13 Sep 2026 12:55:28 +0200 Subject: [PATCH] Fix to change Software Packages file validation --- software/software-lib.pl | 19 +++++- software/view-lib.pl | 26 -------- software/view.cgi | 10 +-- t/software-view.t | 136 ++++++++++++++++++++++++--------------- 4 files changed, 104 insertions(+), 87 deletions(-) delete mode 100644 software/view-lib.pl diff --git a/software/software-lib.pl b/software/software-lib.pl index 637b88063..cb0e52c18 100755 --- a/software/software-lib.pl +++ b/software/software-lib.pl @@ -300,6 +300,24 @@ if ($rel1 ne "" && $rel2 ne "" && $config{'package_system'} eq 'rpm') { return &compare_version_numbers($_[0], $_[1]); } +# is_package_file(package, version, path) +# Returns 1 if the exact path is listed for the selected package. +sub is_package_file +{ +my ($package, $version, $path) = @_; +return 0 if (!defined($package) || $package eq "" || + !defined($path) || $path eq ""); + +# Match the path against the package manager's file list. +local %files; +my $count = &check_files($package, $version); +for(my $i = 0; $i < $count; $i++) { + return 1 if (defined($files{$i,'path'}) && + $files{$i,'path'} eq $path); + } +return 0; +} + # check_package_system() # Returns an error message if some command needed by the selected package # management system is missing. @@ -344,4 +362,3 @@ return $err; } 1; - diff --git a/software/view-lib.pl b/software/view-lib.pl deleted file mode 100644 index a895a88ab..000000000 --- a/software/view-lib.pl +++ /dev/null @@ -1,26 +0,0 @@ -# view-lib.pl -# Helpers for viewing files listed by the package manager. - -# can_view_package_file(package, version, path) -# Returns 1 if path is a regular or editable file reported for the package. -sub can_view_package_file -{ -my ($package, $version, $path) = @_; -return 0 if (!defined($package) || $package eq "" || - !defined($path) || $path eq ""); -return 0 if ($package =~ /[\\\/\0\r\n]/ || $package =~ /^\s*-/ || - (defined($version) && $version =~ /[\0\r\n]/)); - -# Trust only the package manager's exact path and file type. -local %files; -my $count = &check_files($package, $version); -for(my $i = 0; $i < $count; $i++) { - my $type = $files{$i,'type'}; - return 1 if (defined($files{$i,'path'}) && - $files{$i,'path'} eq $path && - defined($type) && ($type == 0 || $type == 5)); - } -return 0; -} - -1; diff --git a/software/view.cgi b/software/view.cgi index dbbda52f7..3c85a6511 100755 --- a/software/view.cgi +++ b/software/view.cgi @@ -3,27 +3,19 @@ # Output the contents of a file require './software-lib.pl'; -require './view-lib.pl'; &ReadParse(); $p = $in{'file'}; # Only show files listed for the selected package. -&can_view_package_file($in{'package'}, $in{'version'}, $p) || +&is_package_file($in{'package'}, $in{'version'}, $p) || &error($text{'list_enotpackage'}); -# Reject symlinks and path changes between validation and open. if (!open(FILE, "<", $p)) { print "Content-type: text/plain\n\n"; print &text('list_eview', $p, $!),"\n"; exit; } -my @lst = lstat($p); my @st = stat(FILE); -if (!@lst || !@st || ($lst[2] & 0170000) != 0100000 || - $lst[0] != $st[0] || $lst[1] != $st[1]) { - close(FILE); - &error($text{'list_enotpackage'}); - } # Try to guess type from filename if ($p =~ /\.([^\.\/]+)$/) { diff --git a/t/software-view.t b/t/software-view.t index f1c8fd885..ed689262f 100644 --- a/t/software-view.t +++ b/t/software-view.t @@ -5,31 +5,62 @@ use warnings; use Test::More; use File::Basename qw(dirname); use File::Copy qw(copy); +use File::Path qw(make_path); use File::Spec; use File::Temp qw(tempdir); use IPC::Open3; use Symbol qw(gensym); use Cwd qw(abs_path getcwd); -our (%files, %package_files, $check_calls); +our (%files, %package_files); my $root = abs_path(File::Spec->catdir(dirname(__FILE__), '..')); -my $lib = File::Spec->catfile($root, 'software', 'view-lib.pl'); -require $lib; -sub check_files +sub write_text { -my ($package, $version) = @_; -$check_calls++; -%files = ( ); -my $entries = $package_files{"$package\0".($version || '')} || []; -for(my $i = 0; $i < @$entries; $i++) { - $files{$i,'path'} = $entries->[$i]->[0]; - $files{$i,'type'} = $entries->[$i]->[1]; - } -return scalar(@$entries); +my ($path, $content) = @_; +open(my $fh, '>', $path) or die "open $path: $!"; +print {$fh} $content; +close($fh) or die "close $path: $!"; } +# Load the real shared library with an isolated Webmin configuration. +my $load_dir = tempdir(CLEANUP => 1); +my $config_dir = File::Spec->catdir($load_dir, 'config'); +my $var_dir = File::Spec->catdir($load_dir, 'var'); +make_path(File::Spec->catdir($config_dir, 'software'), $var_dir); +write_text(File::Spec->catfile($config_dir, 'config'), + "os_type=debian-linux\nos_version=24.04\nlang=en\n"); +write_text(File::Spec->catfile($config_dir, 'miniserv.conf'), + "root=$root\nmimetypes=$root/mime.types\n"); +write_text(File::Spec->catfile($config_dir, 'webmin.acl'), + "root: software\n"); +write_text(File::Spec->catfile($config_dir, 'software', 'config'), + "package_system=debian\nupdate_system=*\n"); + +my $cwd = getcwd(); +my ($loaded, $load_error); +{ + local %ENV = ( + %ENV, + WEBMIN_CONFIG => $config_dir, + WEBMIN_VAR => $var_dir, + SERVER_ROOT => $root, + SCRIPT_NAME => '/software/test.cgi', + SCRIPT_FILENAME => File::Spec->catfile($root, 'software', + 'test.cgi'), + REMOTE_USER => 'root', + BASE_REMOTE_USER => 'root', + REQUEST_METHOD => 'GET', + ); + chdir(File::Spec->catdir($root, 'software')) + or die "chdir software: $!"; + $loaded = do './software-lib.pl'; + $load_error = $@ || $!; + chdir($cwd) or die "chdir $cwd: $!"; +} +BAIL_OUT("cannot load software-lib.pl: $load_error") if !$loaded; + $package_files{"safe-package\0".'1.0'} = [ [ '/usr/bin/safe-tool', 0 ], [ '/etc/safe-tool.conf', 5 ], @@ -40,32 +71,39 @@ $package_files{"other-package\0".'1.0'} = [ [ '/usr/bin/other-tool', 0 ], ]; -ok(can_view_package_file('safe-package', '1.0', '/usr/bin/safe-tool'), - 'allows a regular file listed by the selected package'); -ok(can_view_package_file('safe-package', '1.0', '/etc/safe-tool.conf'), - 'allows an editable file listed by the selected package'); -ok(!can_view_package_file('safe-package', '1.0', - '/var/webmin/sessiondb'), - 'rejects an arbitrary file not listed by the selected package'); -ok(!can_view_package_file('safe-package', '1.0', - '/usr/bin/../var/webmin/sessiondb'), - 'rejects a traversal path not returned by the package manager'); -ok(!can_view_package_file('safe-package', '1.0', '/usr/bin/other-tool'), - 'rejects a file listed by a different package'); -ok(!can_view_package_file('safe-package', '1.0', '/usr/share/safe-tool'), - 'rejects package directories'); -ok(!can_view_package_file('safe-package', '1.0', '/usr/bin/safe-tool-link'), - 'rejects package links'); -ok(!can_view_package_file(undef, '1.0', '/usr/bin/safe-tool'), - 'rejects requests without a package name'); -ok(!can_view_package_file('safe-package', '1.0', undef), - 'rejects requests without a file path'); -$check_calls = 0; -ok(!can_view_package_file('../safe-package', '1.0', '/usr/bin/safe-tool'), - 'rejects package names containing path traversal'); -ok(!can_view_package_file('--root=/tmp', '1.0', '/usr/bin/safe-tool'), - 'rejects package names that could become command options'); -is($check_calls, 0, 'rejects invalid package names before calling the backend'); +{ + no warnings qw(redefine once); + local *check_files = sub { + my ($package, $version) = @_; + %files = ( ); + my $entries = $package_files{"$package\0".($version || '')} || []; + for(my $i = 0; $i < @$entries; $i++) { + $files{$i,'path'} = $entries->[$i]->[0]; + $files{$i,'type'} = $entries->[$i]->[1]; + } + return scalar(@$entries); + }; + + ok(is_package_file('safe-package', '1.0', '/usr/bin/safe-tool'), + 'allows a file listed by the selected package'); + ok(is_package_file('safe-package', '1.0', '/etc/safe-tool.conf'), + 'allows an editable file listed by the selected package'); + ok(is_package_file('safe-package', '1.0', '/usr/share/safe-tool'), + 'allows a directory listed by the selected package'); + ok(is_package_file('safe-package', '1.0', '/usr/bin/safe-tool-link'), + 'allows a link listed by the selected package'); + ok(!is_package_file('safe-package', '1.0', '/var/webmin/sessiondb'), + 'rejects an arbitrary file not listed by the selected package'); + ok(!is_package_file('safe-package', '1.0', + '/usr/bin/../var/webmin/sessiondb'), + 'rejects a traversal path not returned by the package manager'); + ok(!is_package_file('safe-package', '1.0', '/usr/bin/other-tool'), + 'rejects a file listed by a different package'); + ok(!is_package_file(undef, '1.0', '/usr/bin/safe-tool'), + 'rejects requests without a package name'); + ok(!is_package_file('safe-package', '1.0', undef), + 'rejects requests without a file path'); +} sub urlize { @@ -104,13 +142,11 @@ subtest 'view CGI enforces package file validation' => sub { my $dir = tempdir(CLEANUP => 1); copy(File::Spec->catfile($root, 'software', 'view.cgi'), File::Spec->catfile($dir, 'view.cgi')) or die "copy view.cgi: $!"; - copy($lib, File::Spec->catfile($dir, 'view-lib.pl')) - or die "copy view-lib.pl: $!"; my $stub = File::Spec->catfile($dir, 'software-lib.pl'); open(my $stub_fh, '>', $stub) or die "open $stub: $!"; print {$stub_fh} <<'PERL'; -our (%in, %text, %files); +our (%in, %text); $text{'list_enotpackage'} = 'Cannot view this package file'; sub ReadParse { foreach my $item (split(/&/, $ENV{'QUERY_STRING'} || '')) { @@ -121,12 +157,10 @@ sub ReadParse { $in{$key} = $value; } } -sub check_files { - %files = ( ); - return 0 if ($_[0] ne 'safe-package' || $_[1] ne '1.0'); - $files{0,'path'} = $ENV{'TEST_ALLOWED_FILE'}; - $files{0,'type'} = 0; - return 1; +sub is_package_file { + return 0 if !defined($_[0]) || !defined($_[1]) || !defined($_[2]); + return $_[0] eq 'safe-package' && $_[1] eq '1.0' && + $_[2] eq $ENV{'TEST_ALLOWED_FILE'}; } sub error { print "Content-type: text/plain\n\n$_[0]\n"; @@ -173,10 +207,10 @@ PERL skip('symlinks are not available', 2) if !symlink($secret, $link); ($status, $out, $err) = run_view_cgi($dir, $base.urlize($link), '', $link); - is($status, 0, 'symlink rejection exits cleanly') + is($status, 0, 'listed symlink request exits cleanly') or diag($err); - unlike($out, qr/secret contents/, - 'validated path cannot be replaced with a symlink'); + like($out, qr/secret contents/, + 'listed symlink is handled like any other package path'); } };