Merge pull request #2721 from swelljoe/fix-acl-bugs

Fixes minor bugs in acl surfaced by tests

acl/acl-lib.pl

@@ -1809,6 +1809,7 @@ foreach my $g (&list_groups()) {
 		return $g;
 		}
 	}
+return;
 }
 
 =head2 check_password_restrictions(username, password)

acl/log_parser.pl

@@ -16,8 +16,9 @@ my ($user, $script, $action, $type, $object, $p) = @_;
 my $g = $type eq 'group' ? "_g" : "";
 if ($action eq 'modify') {
 	if ($p->{'old'} ne $p->{'name'}) {
-		return &text('log_rename'.$g, "<tt>$p->{'old'}</tt>",
-					      "<tt>$p->{'name'}</tt>");
+		return &text('log_rename'.$g,
+			     "<tt>".&html_escape($p->{'old'})."</tt>",
+			     "<tt>".&html_escape($p->{'name'})."</tt>");
 		}
 	else {
 		return &text('log_modify'.$g,
@@ -26,7 +27,8 @@ if ($action eq 'modify') {
 	}
 elsif ($action eq 'create') {
 	if ($p->{'clone'}) {
-		return &text('log_clone'.$g, "<tt>$p->{'clone'}</tt>",
+		return &text('log_clone'.$g,
+			     "<tt>".&html_escape($p->{'clone'})."</tt>",
 			     "<tt>".&html_escape($object)."</tt>");
 		}
 	else {
@@ -36,21 +38,23 @@ elsif ($action eq 'create') {
 	}
 elsif ($action eq 'delete') {
 	if ($type eq "users" || $type eq "groups") {
-		return &text('log_delete_'.$type, $object);
+		return &text('log_delete_'.$type, &html_escape($object));
 		}
 	else {
-		return &text('log_delete'.$g, "<tt>$object</tt>");
+		return &text('log_delete'.$g,
+			     "<tt>".&html_escape($object)."</tt>");
 		}
 	}
 elsif ($action eq 'joingroup') {
-	return &text('log_joingroup', $object, $p->{'group'});
+	return &text('log_joingroup', &html_escape($object),
+		     &html_escape($p->{'group'}));
 	}
 elsif ($action eq 'acl') {
-	return &text('log_acl', "<tt>$object</tt>",
+	return &text('log_acl', "<tt>".&html_escape($object)."</tt>",
 		     "<i>".&html_escape($p->{'moddesc'})."</i>");
 	}
 elsif ($action eq 'reset') {
-	return &text('log_reset', "<tt>$object</tt>",
+	return &text('log_reset', "<tt>".&html_escape($object)."</tt>",
 		     "<i>".&html_escape($p->{'moddesc'})."</i>");
 	}
 elsif ($action eq 'cert') {
@@ -60,7 +64,9 @@ elsif ($action eq 'switch') {
 	return &text('log_switch', "<tt>".&html_escape($object)."</tt>");
 	}
 elsif ($action eq 'twofactor') {
-	return &text('log_twofactor', $object, $p->{'provider'}, $p->{'id'});
+	return &text('log_twofactor', &html_escape($object),
+		     &html_escape($p->{'provider'}),
+		     &html_escape($p->{'id'}));
 	}
 elsif ($action eq 'forgot') {
 	return &text('log_forgot_'.$type, &html_escape($p->{'user'}),

acl/t/run-tests.t

@@ -865,11 +865,9 @@ is(group_line({ name => 'empty' }),
 
     my $g = get_users_group('bob');
     is($g && $g->{'name'}, 'wheel', 'get_users_group finds bob in wheel');
-    # get_users_group falls off the end without an explicit return; in scalar
-    # context that yields '' (the failed loop condition) rather than undef.
-    # Asserting falsy keeps the test honest about the current contract.
-    ok(!get_users_group('nobody'),
-       'get_users_group returns falsy for unaffiliated user');
+    my $miss = get_users_group('nobody');
+    ok(!defined($miss),
+       'get_users_group returns nothing when user is in no group');
 
     delete_from_groups('bob');
     _clear_caches();
@@ -934,22 +932,56 @@ is(group_line({ name => 'empty' }),
     like(parse_webmin_log('admin', 's', 'switch', 'user', 'alice', {}),
          qr/Switched to.*alice/,    'parse_webmin_log switch');
 
-    # html_escape: a malicious $object or moddesc must come back escaped.
-    # Several branches (delete, modify rename, joingroup, twofactor) currently
-    # do NOT escape $object — they interpolate it into a <tt> tag verbatim.
-    # That's an audit-trail XSS gap; mark these TODO so the suite documents
-    # the expected post-fix behavior without going red.
-    TODO: {
-        local $TODO = 'parse_webmin_log delete branch does not html_escape $object';
-        my $xss = parse_webmin_log('admin', 's', 'delete', 'user',
-                                   '<script>x</script>', {});
-        unlike($xss, qr/<script>/,
-               'parse_webmin_log html-escapes user-supplied object in delete');
-        like($xss, qr/&lt;script&gt;/,
-             'parse_webmin_log emits escaped entities for delete object');
+    # html_escape: every branch that interpolates a user-controlled value into
+    # the rendered audit-trail string must come back escaped. These are
+    # defense-in-depth — usernames are normally restricted to a safe charset,
+    # but the audit log is the wrong place to trust upstream validation.
+    my $payload = '<script>x</script>';
+    my $esc     = qr/&lt;script&gt;/;
+
+    for my $case (
+        [ 'delete user',
+          [ 'admin', 's', 'delete', 'user', $payload, {} ] ],
+        [ 'create with clone (clone source)',
+          [ 'admin', 's', 'create', 'user', 'safe',
+            { clone => $payload } ] ],
+        [ 'create with clone (new name)',
+          [ 'admin', 's', 'create', 'user', $payload,
+            { clone => 'safe' } ] ],
+        [ 'modify rename (old name)',
+          [ 'admin', 's', 'modify', 'user', 'safe',
+            { old => $payload, name => 'safe' } ] ],
+        [ 'modify rename (new name)',
+          [ 'admin', 's', 'modify', 'user', 'safe',
+            { old => 'safe', name => $payload } ] ],
+        [ 'joingroup (object)',
+          [ 'admin', 's', 'joingroup', 'user', $payload,
+            { group => 'wheel' } ] ],
+        [ 'joingroup (group)',
+          [ 'admin', 's', 'joingroup', 'user', 'alice',
+            { group => $payload } ] ],
+        [ 'twofactor (object)',
+          [ 'admin', 's', 'twofactor', 'user', $payload,
+            { provider => 'totp', id => 'k' } ] ],
+        [ 'acl (object)',
+          [ 'admin', 's', 'acl', 'user', $payload,
+            { moddesc => 'User Admin' } ] ],
+        [ 'reset (object)',
+          [ 'admin', 's', 'reset', 'user', $payload,
+            { moddesc => 'User Admin' } ] ],
+        [ 'delete users bulk (object)',
+          [ 'admin', 's', 'delete', 'users', $payload, {} ] ],
+        ) {
+        my ($name, $args) = @$case;
+        my $out = parse_webmin_log(@$args);
+        unlike($out, qr/<script>/,
+               "parse_webmin_log html-escapes $name");
+        like($out, $esc,
+             "parse_webmin_log emits entities for $name");
     }
 
-    # acl branch DOES escape moddesc, so this one should pass today.
+    # acl/reset moddesc was already escaped pre-fix; keep the test as a
+    # regression guard.
     my $xss2 = parse_webmin_log('admin', 's', 'acl', 'user', 'alice',
                                 { moddesc => '<img src=x>' });
     unlike($xss2, qr/<img/,