diff --git a/acl/acl-lib.pl b/acl/acl-lib.pl index 765286004..809f0ee44 100755 --- a/acl/acl-lib.pl +++ b/acl/acl-lib.pl @@ -1809,6 +1809,7 @@ foreach my $g (&list_groups()) { return $g; } } +return; } =head2 check_password_restrictions(username, password) diff --git a/acl/log_parser.pl b/acl/log_parser.pl index e164d976e..d61cf8d9c 100755 --- a/acl/log_parser.pl +++ b/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, "$p->{'old'}", - "$p->{'name'}"); + return &text('log_rename'.$g, + "".&html_escape($p->{'old'})."", + "".&html_escape($p->{'name'}).""); } 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, "$p->{'clone'}", + return &text('log_clone'.$g, + "".&html_escape($p->{'clone'})."", "".&html_escape($object).""); } 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, "$object"); + return &text('log_delete'.$g, + "".&html_escape($object).""); } } 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', "$object", + return &text('log_acl', "".&html_escape($object)."", "".&html_escape($p->{'moddesc'}).""); } elsif ($action eq 'reset') { - return &text('log_reset', "$object", + return &text('log_reset', "".&html_escape($object)."", "".&html_escape($p->{'moddesc'}).""); } elsif ($action eq 'cert') { @@ -60,7 +64,9 @@ elsif ($action eq 'switch') { return &text('log_switch', "".&html_escape($object).""); } 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'}), diff --git a/acl/t/run-tests.t b/acl/t/run-tests.t index 958103cf8..5f81caae0 100644 --- a/acl/t/run-tests.t +++ b/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 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', - '', {}); - unlike($xss, qr/'; + my $esc = qr/<script>/; + + 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/