Fix ACL permission checks and partial user saves

This PR fixes missing permission checks when saving global user and group access settings.

Unauthorized ACL changes are rejected before any files are written.

User saves now finish validation before changing group membership or copying permissions, preventing rejected saves from leaving partial changes.

acl/save_group.cgi

@@ -21,6 +21,9 @@ elsif ($in{'but_delete'}) {
 	exit;
 	}
 
+# Check global ACL access before saving any group changes.
+$in{'acl_security_form'} && !$access{'acl'} && &error($text{'acl_emod'});
+
 # Find logged-in webmin user
 my @ulist = &list_users();
 my ($me) = grep { $_->{'name'} eq $base_remote_user } @ulist;

acl/save_user.cgi

@@ -38,6 +38,9 @@ elsif ($in{'but_forgot'}) {
 	return;
 	}
 
+# Check global ACL access before saving any user or group changes.
+$in{'acl_security_form'} && !$access{'acl'} && &error($text{'acl_emod'});
+
 # Get the user object
 my (%user, $old);
 if ($in{'old'}) {
@@ -119,30 +122,8 @@ if (defined($in{'group'})) {
 			&error($text{'save_egroup'});
 		}
 
-	# Store group membership
+	# Find the requested group; save membership after validation.
 	$newgroup = &get_group($in{'group'});
-	if ($in{'group'} ne ($oldgroup ? $oldgroup->{'name'} : '')) {
-		# Group has changed - update the member lists
-		if ($oldgroup) {
-			# Take out of old
-			$oldgroup->{'members'} =
-				[ grep { $_ ne $in{'old'} }
-				  @{$oldgroup->{'members'}} ];
-			&modify_group($oldgroup->{'name'}, $oldgroup);
-			}
-		if ($newgroup) {
-			# Put into new
-			push(@{$newgroup->{'members'}}, $in{'name'});
-			&modify_group($in{'group'}, $newgroup);
-			}
-		}
-	elsif ($in{'old'} ne $in{'name'} && $oldgroup && $newgroup) {
-		# Name has changed - rename in group
-		my $idx = &indexof(
-			$in{'old'}, @{$oldgroup->{'members'}});
-		$oldgroup->{'members'}->[$idx] = $in{'name'};
-		&modify_group($oldgroup->{'name'}, $oldgroup);
-		}
 	}
 
 # Store manually selected modules
@@ -175,11 +156,6 @@ if ($base_remote_user eq $in{'old'} &&
 	&error($text{'save_edeny'});
 	}
 
-if (!$in{'old'} && $access{'perms'}) {
-	# Copy .acl files from creator to new user
-	&copy_acl_files($me->{'name'}, $in{'name'}, $me->{'modules'});
-	}
-
 if ($newgroup) {
 	# Add modules from group to list
 	my @ownmods;
@@ -189,11 +165,6 @@ if ($newgroup) {
 		}
 	@mods = &unique(@mods, @{$newgroup->{'modules'}});
 	$user{'ownmods'} = \@ownmods;
-
-	# Copy ACL files for group
-	my $name = $in{'old'} ? $in{'old'} : $in{'name'};
-	&copy_group_user_acl_files($in{'group'}, $name,
-			      [ @{$newgroup->{'modules'}}, "" ]);
 	}
 $user{'modules'} = \@mods;
 
@@ -345,6 +316,42 @@ if ($in{'cancel'}) {
 	$user{'twofactor_apikey'} = undef;
 	}
 
+# Save membership and ACL files only after all user validation succeeds.
+if (defined($in{'group'})) {
+	if ($in{'group'} ne ($oldgroup ? $oldgroup->{'name'} : '')) {
+		# Group has changed - update the member lists
+		if ($oldgroup) {
+			# Take out of old
+			$oldgroup->{'members'} =
+				[ grep { $_ ne $in{'old'} }
+				  @{$oldgroup->{'members'}} ];
+			&modify_group($oldgroup->{'name'}, $oldgroup);
+			}
+		if ($newgroup) {
+			# Put into new
+			push(@{$newgroup->{'members'}}, $in{'name'});
+			&modify_group($in{'group'}, $newgroup);
+			}
+		}
+	elsif ($in{'old'} ne $in{'name'} && $oldgroup && $newgroup) {
+		# Name has changed - rename in group
+		my $idx = &indexof(
+			$in{'old'}, @{$oldgroup->{'members'}});
+		$oldgroup->{'members'}->[$idx] = $in{'name'};
+		&modify_group($oldgroup->{'name'}, $oldgroup);
+		}
+	}
+if (!$in{'old'} && $access{'perms'}) {
+	# Copy .acl files from creator to new user
+	&copy_acl_files($me->{'name'}, $in{'name'}, $me->{'modules'});
+	}
+if ($newgroup) {
+	# Copy ACL files for group
+	my $name = $in{'old'} ? $in{'old'} : $in{'name'};
+	&copy_group_user_acl_files($in{'group'}, $name,
+			      [ @{$newgroup->{'modules'}}, "" ]);
+	}
+
 if ($in{'old'}) {
 	# update user and all ACLs
 	&modify_user($in{'old'}, \%user);

acl/t/run-tests.t

@@ -1493,6 +1493,107 @@ subtest 'save_user.cgi rejected module escaping' => sub {
     }
 };
 
+# Both save handlers must enforce the global ACL editor's permission check.
+subtest 'user and group global ACL permissions' => sub {
+    for my $allowed (0, 1) {
+        _reset_fixture();
+        create_user({ name => 'operator', pass => 'x', modules => ['acl', 'proc'] });
+        create_user({ name => 'target', pass => 'x', modules => ['proc'] });
+        create_group({ name => 'limited', members => [], modules => ['proc'] });
+        _seed_user_acl('operator', {
+            users => 'target', acl => $allowed, groups => 1,
+            mode => 2, mods => 'proc', times => 0,
+        });
+        my %before = (root => '/restricted', fileunix => 'nobody',
+                      download_address_mode => 'public');
+        save_module_acl(\%before, 'target', '', 1);
+        save_group_module_acl(\%before, 'limited', '', 1);
+
+        for my $kind ('user', 'group') {
+            my $name = $kind eq 'user' ? 'target' : 'limited';
+            my $r = run_cgi("save_$kind.cgi", {
+                old => $name, name => $name, mod => 'proc',
+                pass_def => 1, oldpass => 'x', logouttime_def => 1, minsize_def => 1,
+                acl_security_form => 1, root => '/', fileunix => 'root',
+                download_address_mode => 'all',
+            }, user => 'operator');
+            my $file = $kind eq 'user' ? 'target.acl' : 'limited.gacl';
+            my %saved;
+            read_file("$confdir/$file", \%saved);
+            if (!$allowed) {
+                like($r->{out}, qr/\Q$text{'acl_emod'}\E/, "$kind rejects acl=0");
+                is_deeply(\%saved, \%before, "$kind keeps existing restrictions");
+            }
+            else {
+                ok($r->{location}, "$kind accepts an authorized ACL save");
+                is($saved{root}, '/', "$kind saves the allowed root directory");
+                is($saved{fileunix}, 'root', "$kind saves the file-access user");
+                is($saved{download_address_mode}, 'all', "$kind saves download access");
+            }
+        }
+    }
+};
+
+# Rejected user edits must leave membership and inherited ACL files untouched.
+subtest 'user validation before group writes' => sub {
+    _reset_fixture();
+    create_user({ name => 'operator', pass => 'x', modules => ['acl', 'proc'] });
+    create_user({ name => 'target', pass => 'x', modules => ['proc'] });
+    create_group({ name => 'first', members => ['target'], modules => ['proc'] });
+    create_group({ name => 'second', members => [], modules => ['proc'] });
+    _seed_user_acl('operator', {
+        users => '*', mode => 2, mods => 'proc', gassign => '*', rename => 1,
+        times => 0, acl => 0,
+    });
+    my %before = (root => '/original');
+    save_module_acl(\%before, 'target', '', 1);
+    save_group_module_acl({ root => '/second' }, 'second', '', 1);
+    my %form = (old => 'target', name => 'target', pass_def => 1, oldpass => 'x',
+                logouttime_def => 1, minsize_def => 1, group => 'second', mod => 'proc');
+
+    for my $invalid ({mod => 'cron'}, {email => 'invalid:email'},
+                     {acl_security_form => 1}) {
+        my $r = run_cgi('save_user.cgi', { %form, %$invalid }, user => 'operator');
+        ok(!$r->{location}, 'invalid user save is rejected');
+        _clear_caches();
+        is_deeply(get_group('first')->{members}, ['target'], 'old membership retained');
+        is_deeply(get_group('second')->{members}, [], 'new group not modified');
+        my %saved;
+        read_file("$confdir/target.acl", \%saved);
+        is_deeply(\%saved, \%before, 'inherited ACL not copied on rejection');
+    }
+
+    # Valid transfers and same-group renames still update membership and ACLs.
+    my $r = run_cgi('save_user.cgi', \%form, user => 'operator');
+    ok($r->{location}, 'valid transfer succeeds');
+    _clear_caches();
+    is_deeply(get_group('first')->{members}, [], 'valid transfer removes old membership');
+    is_deeply(get_group('second')->{members}, ['target'], 'valid transfer adds membership');
+    my %saved;
+    read_file("$confdir/target.acl", \%saved);
+    is($saved{root}, '/second', 'valid transfer copies inherited ACL');
+    $r = run_cgi('save_user.cgi', { %form, name => 'renamed' }, user => 'operator');
+    ok($r->{location}, 'same-group rename succeeds');
+    _clear_caches();
+    is_deeply(get_group('second')->{members}, ['renamed'], 'membership follows rename');
+
+    # Failed creation must not add a nonexistent member; valid creation still can.
+    my %new = (%form, old => '', name => 'newmember', mod => 'cron');
+    $r = run_cgi('save_user.cgi', \%new, user => 'operator');
+    ok(!$r->{location}, 'invalid new user is rejected');
+    _clear_caches();
+    is_deeply(get_group('second')->{members}, ['renamed'], 'no member added on rejection');
+    $new{mod} = 'proc';
+    $r = run_cgi('save_user.cgi', \%new, user => 'operator');
+    ok($r->{location}, 'valid new group member is created');
+    _clear_caches();
+    is_deeply(get_group('second')->{members}, ['renamed', 'newmember'],
+              'new user is added to the selected group');
+    %saved = ();
+    read_file("$confdir/newmember.acl", \%saved);
+    is($saved{root}, '/second', 'new user inherits group ACL');
+};
+
 # Unix conversion must not bypass the privileged-name guard in save_user.cgi.
 # An ACL-capable administrator can still deliberately convert such a user.
 subtest 'convert.cgi privileged username gate' => sub {

shorewall/lang/en

@@ -435,7 +435,6 @@ comment_header=Table comment details
 comment_msg=Message
 comment_err=Failed to save comment
 comment_enone=No message entered
-comment_ereturn=Invalid message entered
 
 log_create_table=Added entry to the $1 table
 log_modify_table=Modified entry in the $1 table

shorewall/list.cgi

@@ -59,7 +59,7 @@ if (@table) {
 			push(@cols, &ui_link(
 				"editcmt.cgi?table=".&urlize($in{'table'}).
 				  "&idx=".&urlize($i),
-				"<i>".&html_escape(join(" ",@t[1..$#t]))."</i>"));
+				"<i>".join(" ", @t[1..$#t])."</i>"));
 			@tds = ( "width=5", "colspan=".scalar(@colnames) );
 			}
 		else {
@@ -70,12 +70,11 @@ if (@table) {
 			for($j=0; $j<@colnames; $j++) {
 				if ($j == 0) {
 					$lnk = &ui_link(
-					  "edit.cgi?table=".&urlize($in{'table'}).
+					    "edit.cgi?table=".&urlize($in{'table'}).
-					  "&idx=".&urlize($i),
+					    "&idx=".&urlize($i), $t[$j]);
-					  &html_escape($t[$j]));
 					}
 				else {
-					$lnk = &html_escape($t[$j]);
+					$lnk = $t[$j];
 					}
 				push(@cols, $lnk);
 				}

shorewall/savecmt.cgi

@@ -15,7 +15,6 @@ if ($in{'delete'}) {
 else {
 	# Validate inputs
 	$in{'msg'} =~ /\S/ || &error($text{'comment_enone'});
-	$in{'msg'} =~ /\r|\0/ && &error($text{'comment_ereturn'});
 	$line = (&version_atleast(4, 5, 11) ? "?" : "") . "COMMENT $in{'msg'}";
 
 	# Update or add

shorewall6/lang/en

@@ -421,7 +421,6 @@ comment_header=Table comment details
 comment_msg=Message
 comment_err=Failed to save comment
 comment_enone=No message entered
-comment_ereturn=Invalid message entered
 
 log_create_table=Added entry to the $1 table
 log_modify_table=Modified entry in the $1 table

shorewall6/list.cgi

@@ -57,10 +57,8 @@ if (@table) {
 		local @tds;
 		if ($t[0] =~ /\??COMMENT/) {
 			# Special case - a comment line
-			push(@cols, "<a href='editcmt.cgi?table=".
+			push(@cols, "<a href='editcmt.cgi?table=$in{'table'}&".
-				    &urlize($in{'table'})."&idx=".
+				    "idx=$i'><i>".join(" ", @t[1..$#t]).
-				    &urlize($i)."'><i>".
-				    &html_escape(join(" ", @t[1..$#t])).
 				    "</i></a>" );
 			@tds = ( "width=5", "colspan=".scalar(@colnames) );
 			}
@@ -71,13 +69,10 @@ if (@table) {
 				}
 			for($j=0; $j<@colnames; $j++) {
 				if ($j == 0) {
-					$lnk = &ui_link(
+					$lnk = &ui_link("edit.cgi?table=$in{'table'}&idx=$i",$t[$j]);
-					  "edit.cgi?table=".&urlize($in{'table'}).
-					  "&idx=".&urlize($i),
-					  &html_escape($t[$j]));
 					}
 				else {
-					$lnk = &html_escape($t[$j]);
+					$lnk = $t[$j];
 					}
 				push(@cols, $lnk);
 				}

shorewall6/savecmt.cgi

@@ -15,7 +15,6 @@ if ($in{'delete'}) {
 else {
 	# Validate inputs
 	$in{'msg'} =~ /\S/ || &error($text{'comment_enone'});
-	$in{'msg'} =~ /\r|\0/ && &error($text{'comment_ereturn'});
 	$line = (&version_atleast(4, 5, 11) ? "?" : "") . "COMMENT $in{'msg'}";
 
 	# Update or add

version

@@ -1 +1 @@
-2.671
+2.670