Fix to change Software Packages file validation

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;
-

software/view-lib.pl

@@ -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;

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 =~ /\.([^\.\/]+)$/) {

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');
 	}
 };