From c8fb79016c30c4746425f97ff587df12f2a8b441 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Tue, 7 Jul 2026 23:33:18 +0200 Subject: [PATCH] Fix ACL session lookups for HMAC session keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⓘ Load Miniserv’s session HMAC key in ACL session helpers so session listing, lookup, and user switching continue to work with HMAC-hashed session DB keys. https://forum.virtualmin.com/t/usermin-2-550-login-from-the-virtualmin-edit-users-page-stopped-working/137491/37?u=ilia --- acl/acl-lib.pl | 61 ++++++++++++++++++++++++++++------- acl/t/run-tests.t | 82 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 129 insertions(+), 14 deletions(-) diff --git a/acl/acl-lib.pl b/acl/acl-lib.pl index 9d465e6ec..02ad50a00 100755 --- a/acl/acl-lib.pl +++ b/acl/acl-lib.pl @@ -20,7 +20,10 @@ use WebminCore; &init_config(); do 'md5-lib.pl'; our ($module_root_directory, %text, %sessiondb, %config, %gconfig, - $base_remote_user, %hash_session_id_cache); + $base_remote_user, %hash_session_id_cache, $session_hmac_key, + $loaded_session_keyfile, $use_hmac_sha256); +eval "use Digest::SHA qw(hmac_sha256_hex); hmac_sha256_hex('x', 'y');"; +$use_hmac_sha256 = 1 if (!$@); our %access = &get_module_acl(); $access{'switch'} = 0 if (&is_readonly_mode()); @@ -1332,6 +1335,7 @@ my ($miniserv) = @_; my $sfile = $miniserv->{'sessiondb'} ? $miniserv->{'sessiondb'} : $miniserv->{'pidfile'} =~ /^(.*)\/[^\/]+$/ ? "$1/sessiondb" : return; +&load_session_hmac_key($miniserv); eval { require SDBM_File; SDBM_File->import; 1 }; dbmopen(%sessiondb, $sfile, 0700); eval { $sessiondb{'1111111111'} = 'foo bar' }; @@ -1345,6 +1349,36 @@ else { } } +=head2 load_session_hmac_key(\%miniserv) + +Loads miniserv's existing session HMAC key, if present. + +=cut +sub load_session_hmac_key +{ +my ($miniserv) = @_; +return if (!$use_hmac_sha256); +if (!$miniserv) { + $miniserv = { }; + &get_miniserv_config($miniserv); + } +my $kf = $miniserv->{'session_keyfile'}; +if (!$kf && $miniserv->{'pidfile'} =~ /^(.*)\/[^\/]+$/) { + $kf = "$1/session.key"; + } +return if (!$kf); +return if (defined($loaded_session_keyfile) && $loaded_session_keyfile eq $kf); +$loaded_session_keyfile = $kf; +$session_hmac_key = undef; +if (open(my $fh, "<", $kf)) { + binmode($fh); + local $/; + $session_hmac_key = <$fh>; + close($fh); + } +undef(%hash_session_id_cache); +} + =head2 delete_session_id(\%miniserv, id) Deletes one session from the database. Parameters are : @@ -2002,23 +2036,28 @@ return; =head2 hash_session_id(sid) -Returns an MD5 or Unix-crypted session ID. +Returns a session DB key for a session ID. =cut sub hash_session_id { my ($sid) = @_; my $use_md5 = &md5_perl_module(); +&load_session_hmac_key() if ($use_hmac_sha256 && !$loaded_session_keyfile); if (!$hash_session_id_cache{$sid}) { - if ($use_md5) { - # Take MD5 hash - $hash_session_id_cache{$sid} = &hash_md5_session($sid); - } - else { - # Unix crypt - $hash_session_id_cache{$sid} = &unix_crypt($sid, "XX"); - } - } + if ($use_hmac_sha256 && length($session_hmac_key) >= 16) { + $hash_session_id_cache{$sid} = + Digest::SHA::hmac_sha256_hex($sid, $session_hmac_key); + } + elsif ($use_md5) { + # Take MD5 hash + $hash_session_id_cache{$sid} = &hash_md5_session($sid); + } + else { + # Unix crypt + $hash_session_id_cache{$sid} = &unix_crypt($sid, "XX"); + } + } return $hash_session_id_cache{$sid}; } diff --git a/acl/t/run-tests.t b/acl/t/run-tests.t index 310956382..71b115f92 100644 --- a/acl/t/run-tests.t +++ b/acl/t/run-tests.t @@ -531,8 +531,12 @@ is(group_line({ name => 'empty' }), # hash_session_id: deterministic, with an in-process cache. Use a local cache # hash so test ordering doesn't pollute it. { - our %hash_session_id_cache; + our (%hash_session_id_cache, $session_hmac_key, $loaded_session_keyfile, + $use_hmac_sha256); local %hash_session_id_cache; + local $session_hmac_key; + local $loaded_session_keyfile; + local $use_hmac_sha256 = 0; my $sid = '0123456789abcdef0123456789abcdef'; my $h1 = hash_session_id($sid); ok(length($h1) > 0, 'hash_session_id returns a non-empty hash'); @@ -542,6 +546,40 @@ is(group_line({ name => 'empty' }), 'different session id hashes differently'); } +# hash_session_id / session_db_key: HMAC session keys must match miniserv. +{ + my $have_hmac = eval { + require Digest::SHA; + Digest::SHA::hmac_sha256_hex('x', 'y'); + 1; + }; + SKIP: { + skip 'Digest::SHA hmac_sha256_hex not available', 2 unless $have_hmac; + our (%hash_session_id_cache, %sessiondb, $session_hmac_key, + $loaded_session_keyfile, $use_hmac_sha256); + local %hash_session_id_cache; + local %sessiondb; + local $session_hmac_key; + local $loaded_session_keyfile; + local $use_hmac_sha256 = 1; + my $key = 'k' x 32; + my $keyfile = "$confdir/session.key"; + open(my $kfh, ">", $keyfile) or die "$keyfile: $!"; + binmode($kfh); + print $kfh $key; + close($kfh); + load_session_hmac_key({ session_keyfile => $keyfile }); + + my $sid = '0123456789abcdef0123456789abcdef'; + my $expected = Digest::SHA::hmac_sha256_hex($sid, $key); + is(hash_session_id($sid), $expected, + 'hash_session_id uses miniserv HMAC session key'); + $sessiondb{$expected} = 'alice 1700000000 203.0.113.10'; + is(session_db_key($sid), $expected, + 'session_db_key finds existing HMAC-hashed session row'); + } +} + # md5_perl_module: should report a usable MD5 class on any system with either # MD5 or Digest::MD5 available. On a box with neither it returns undef, so # skip in that case rather than fail. @@ -1501,8 +1539,6 @@ subtest 'save_user.cgi self-lockout guard' => sub { }; # switch.cgi — access{switch} required AND can_edit_user(target) required. -# The happy path is harder to assert because it touches the session DB and -# kills the cookie. We test the gates here and the no-op contract. subtest 'switch.cgi gating' => sub { # 1. access{switch}=0 -> error (defaultacl has switch=1, must override) _reset_fixture(); @@ -1522,6 +1558,46 @@ subtest 'switch.cgi gating' => sub { ok(!$r->{location}, 'no redirect when can_edit_user denies (even with switch=1)'); delete_user('target'); + + # 3. happy path: the current HMAC-keyed session is rewritten to target + _reset_fixture(); + _seed_user_acl('admin', { users => '*', switch => 1 }); + create_user({ name => 'target', pass => 'x', modules => ['acl'] }); + my $sid = '0123456789abcdef0123456789abcdef'; + my $ip = '203.0.113.10'; + my $key = 's' x 32; + open(my $kfh, ">", "$vardir/session.key") or die "$vardir/session.key: $!"; + binmode($kfh); + print $kfh $key; + close($kfh); + { + our (%sessiondb, %hash_session_id_cache, $session_hmac_key, + $loaded_session_keyfile); + local %hash_session_id_cache; + local $session_hmac_key; + local $loaded_session_keyfile; + my %miniserv = ( pidfile => "$vardir/miniserv.pid" ); + open_session_db(\%miniserv); + my $skey = hash_session_id($sid); + $sessiondb{$skey} = "admin 1700000000 $ip"; + dbmclose(%sessiondb); + + $r = run_cgi('switch.cgi', { user => 'target' }, + env => { SESSION_ID => $sid, REMOTE_ADDR => $ip }); + like($r->{location}, qr{^(?:https?://[^/]+)?/$}, + 'switch.cgi redirects on successful switch') + or diag("stdout: $r->{out}\nstderr: $r->{err}"); + open_session_db(\%miniserv); + my ($switched_user, $switched_time, $switched_ip) = + split(/\s+/, $sessiondb{$skey}); + dbmclose(%sessiondb); + is($switched_user, 'target', + 'switch.cgi rewrites HMAC-keyed session user'); + is($switched_time, '1700000000', + 'switch.cgi preserves original session timestamp'); + is($switched_ip, $ip, 'switch.cgi stores caller IP'); + } + delete_user('target'); }; done_testing();