From 3dd9ee36bef15228984ce8b7f6ee70562eb520d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Lesimple?= Date: Fri, 30 Aug 2024 13:19:11 +0000 Subject: [PATCH] enh: add lock for group ACL change to avoid race conditions --- bin/helper/osh-accountCreate | 4 ++- bin/helper/osh-accountDelete | 9 +++++ bin/helper/osh-groupAddServer | 7 ++++ bin/helper/osh-groupCreate | 6 ++-- bin/helper/osh-groupDelete | 9 +++++ bin/helper/osh-groupSetServers | 7 ++++ lib/perl/OVH/Bastion/Helper.pm | 65 +++++++++++++++++++++++----------- 7 files changed, 84 insertions(+), 23 deletions(-) diff --git a/bin/helper/osh-accountCreate b/bin/helper/osh-accountCreate index 286fe44fb..0dfef58a1 100755 --- a/bin/helper/osh-accountCreate +++ b/bin/helper/osh-accountCreate @@ -69,7 +69,9 @@ if (not grep { $type eq $_ } qw{ normal realm }) { # take a lock here, do it before checking for account existence, # because another parallel creation of the same account might be # occuring, in which case we'd still hit a race condition -$fnret = OVH::Bastion::Helper::get_lock_fh(); +# additionally, the lock type "passwd" is used by all helpers +# that may modify /etc/passwd or /etc/group. +$fnret = OVH::Bastion::Helper::get_lock_fh(category => "passwd"); $fnret or HEXIT($fnret); my $lock_fh = $fnret->value; $fnret = OVH::Bastion::Helper::acquire_lock($lock_fh); diff --git a/bin/helper/osh-accountDelete b/bin/helper/osh-accountDelete index a452a1fb6..ba686091b 100755 --- a/bin/helper/osh-accountDelete +++ b/bin/helper/osh-accountDelete @@ -238,6 +238,15 @@ if (-d $fulldir) { } osh_info("Backup done"); +# take a lock here, as we're going to remove system accounts and groups. +# additionally, the lock type "passwd" is used by all helpers +# that may modify /etc/passwd or /etc/group. +$fnret = OVH::Bastion::Helper::get_lock_fh(category => "passwd"); +$fnret or HEXIT($fnret); +my $lock_fh = $fnret->value; +$fnret = OVH::Bastion::Helper::acquire_lock($lock_fh); +$fnret or HEXIT($fnret); + osh_info "Removing '$account' group membership from 'keyreader' user"; $fnret = OVH::Bastion::sys_delmemberfromgroup(user => "keyreader", group => $account); if (!$fnret) { diff --git a/bin/helper/osh-groupAddServer b/bin/helper/osh-groupAddServer index 9de9fadec..5c25d4e8b 100755 --- a/bin/helper/osh-groupAddServer +++ b/bin/helper/osh-groupAddServer @@ -78,6 +78,13 @@ else { # "groupacl", basepath => "/home/$group"); +$fnret or HEXIT($fnret); +my $lock_fh = $fnret->value; +$fnret = OVH::Bastion::Helper::acquire_lock($lock_fh); +$fnret or HEXIT($fnret); + #>CODE my $machine = $ip; $port and $machine .= ":$port"; diff --git a/bin/helper/osh-groupCreate b/bin/helper/osh-groupCreate index 624424874..f2bcdf747 100755 --- a/bin/helper/osh-groupCreate +++ b/bin/helper/osh-groupCreate @@ -96,8 +96,10 @@ my $shortGroup = $fnret->value->{'shortGroup'}; # take a lock here, do it before checking for group existence, # because another parallel creation of the same group might be -# occuring, in which case we'd still hit a race condition -$fnret = OVH::Bastion::Helper::get_lock_fh(); +# occuring, in which case we'd still hit a race condition. +# additionally, the lock type "passwd" is used by all helpers +# that may modify /etc/passwd or /etc/group. +$fnret = OVH::Bastion::Helper::get_lock_fh(category => "passwd"); $fnret or HEXIT($fnret); my $lock_fh = $fnret->value; $fnret = OVH::Bastion::Helper::acquire_lock($lock_fh); diff --git a/bin/helper/osh-groupDelete b/bin/helper/osh-groupDelete index 0f74e1fa4..b65dffdd0 100755 --- a/bin/helper/osh-groupDelete +++ b/bin/helper/osh-groupDelete @@ -162,6 +162,15 @@ if (-d $fulldir) { } osh_info("Backup done"); +# take a lock here, as we're going to remove system accounts and groups. +# additionally, the lock type "passwd" is used by all helpers +# that may modify /etc/passwd or /etc/group. +$fnret = OVH::Bastion::Helper::get_lock_fh(category => "passwd"); +$fnret or HEXIT($fnret); +my $lock_fh = $fnret->value; +$fnret = OVH::Bastion::Helper::acquire_lock($lock_fh); +$fnret or HEXIT($fnret); + # remove dead symlinks in users homes my $dh; if (opendir($dh, "/home/allowkeeper")) { diff --git a/bin/helper/osh-groupSetServers b/bin/helper/osh-groupSetServers index 892dd6382..6bfc7d0f2 100755 --- a/bin/helper/osh-groupSetServers +++ b/bin/helper/osh-groupSetServers @@ -77,6 +77,13 @@ if (!$data || ref $data ne 'ARRAY') { HEXIT('ERR_INVALID_ARGUMENT', msg => "Invalid JSON import format sent by the plugin"); } +# take a lock here, to block other group ACL modifying commands for this group until we're done. +$fnret = OVH::Bastion::Helper::get_lock_fh(category => "groupacl", basepath => "/home/$group"); +$fnret or HEXIT($fnret); +my $lock_fh = $fnret->value; +$fnret = OVH::Bastion::Helper::acquire_lock($lock_fh); +$fnret or HEXIT($fnret); + $fnret = OVH::Bastion::access_modify( way => 'group', action => 'clear', diff --git a/lib/perl/OVH/Bastion/Helper.pm b/lib/perl/OVH/Bastion/Helper.pm index 3a3b45de0..0c214bdca 100644 --- a/lib/perl/OVH/Bastion/Helper.pm +++ b/lib/perl/OVH/Bastion/Helper.pm @@ -75,27 +75,52 @@ if (not defined $self) { } sub get_lock_fh { - my $fh; - my $lockdir = "/tmp/bastion.lock"; - my $lockfile = "$lockdir/lock"; - - # to avoid symlink attacks, we first create a subdir only accessible by root - unlink $lockdir; # will silently fail if doesn't exist or is not a file - mkdir $lockdir; # will silently fail if we lost the race - chown 0, 0, $lockdir; - chmod 0700, $lockdir; - - # now, check if we do have a directory, or if we lost the race - if (!-d $lockdir) { - warn_syslog("Couldn't create $lockdir: are we being raced against?"); - return R('ERR_CANNOT_LOCK', msg => "Couldn't create lock file, please retry"); + my %params = @_; + my $category = $params{'category'}; + + return R('ERR_MISSING_PARAMETER', msg => "Missing category in get_lock_fh") if !$category; + + my ($lockdir, $lockfile, $lockdircreate); + if ($category eq 'passwd') { + $lockdir = "/tmp/bastion.lock.passwd"; + $lockfile = "$lockdir/lock"; + $lockdircreate = 1; } - # here, $lockdir is guaranteed to be a directory, check its perms - my @perms = stat($lockdir); - if ($perms[4] != 0 || $perms[5] != 0 || S_IMODE($perms[2]) != oct(700)) { - warn_syslog("The $lockdir directory has invalid perms: are we being raced against? mode=" - . sprintf("%04o", S_IMODE($perms[2]))); - return R('ERR_CANNOT_LOCK', msg => "Couldn't create lock file, please retry"); + elsif ($category eq 'groupacl') { + $lockdir = $params{'basepath'}; + $lockdircreate = 0; # must already exist + if (!$lockdir || !-d $lockdir || !-w $lockdir) { + return R('ERR_INVALID_PARAMETER', + msg => "Missing or invalid basepath '" . ($lockdir // '') . "' in get_lock_fh"); + } + # we use the .db suffix because it's already excluded from the cluster sync: + $lockfile = "$lockdir/lock.db"; + } + else { + return R('ERR_INVALID_PARAMETER', msg => "Unknown category '$category' in get_lock_fh"); + } + + my $fh; + + if ($lockdircreate) { + # to avoid symlink attacks, we first create a subdir only accessible by root + unlink $lockdir; # will silently fail if doesn't exist or is not a file + mkdir $lockdir; # will silently fail if we lost the race + chown 0, 0, $lockdir; + chmod 0700, $lockdir; + + # now, check if we do have a directory, or if we lost the race + if (!-d $lockdir) { + warn_syslog("Couldn't create $lockdir: are we being raced against?"); + return R('ERR_CANNOT_LOCK', msg => "Couldn't create lock file, please retry"); + } + # here, $lockdir is guaranteed to be a directory, check its perms + my @perms = stat($lockdir); + if ($perms[4] != $< || $perms[5] != $( || S_IMODE($perms[2]) != oct(700)) { + warn_syslog("The $lockdir directory has invalid perms: are we being raced against? mode=" + . sprintf("%04o", S_IMODE($perms[2]))); + return R('ERR_CANNOT_LOCK', msg => "Couldn't create lock file, please retry"); + } } # here, $lockdir is guaranteed to be owned only by us. but rogue files