From a1812e34bb72c8df456b218ae8f81a7dcf7bbd14 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Lesimple?=
 <stephane.lesimple+bastion@ovhcloud.com>
Date: Tue, 21 Mar 2023 11:06:57 +0000
Subject: [PATCH] fix: race condition when two parallel account creations used
 --uid-auto

Fixes #363
---
 bin/helper/osh-accountCreate   |  9 +++++
 bin/helper/osh-groupCreate     |  9 +++++
 lib/perl/OVH/Bastion/Helper.pm | 63 ++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+)

diff --git a/bin/helper/osh-accountCreate b/bin/helper/osh-accountCreate
index de17373..286fe44 100755
--- a/bin/helper/osh-accountCreate
+++ b/bin/helper/osh-accountCreate
@@ -66,6 +66,15 @@ if (not grep { $type eq $_ } qw{ normal realm }) {
 
 #<PARAMS:TYPE
 
+# 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();
+$fnret or HEXIT($fnret);
+my $lock_fh = $fnret->value;
+$fnret = OVH::Bastion::Helper::acquire_lock($lock_fh);
+$fnret or HEXIT($fnret);
+
 #>PARAMS:ACCOUNT
 osh_debug("Checking account");
 $fnret = OVH::Bastion::is_account_valid(account => $account);
diff --git a/bin/helper/osh-groupCreate b/bin/helper/osh-groupCreate
index 43c3cd1..6244248 100755
--- a/bin/helper/osh-groupCreate
+++ b/bin/helper/osh-groupCreate
@@ -94,6 +94,15 @@ $fnret or HEXIT($fnret);
 $group = $fnret->value->{'group'};
 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();
+$fnret or HEXIT($fnret);
+my $lock_fh = $fnret->value;
+$fnret = OVH::Bastion::Helper::acquire_lock($lock_fh);
+$fnret or HEXIT($fnret);
+
 foreach my $test ($group, "$group-gatekeeper", "$group-owner") {
     $fnret = OVH::Bastion::is_group_existing(group => $test);
     $fnret->is_err and HEXIT($fnret);
diff --git a/lib/perl/OVH/Bastion/Helper.pm b/lib/perl/OVH/Bastion/Helper.pm
index b9a8206..3a3b45d 100644
--- a/lib/perl/OVH/Bastion/Helper.pm
+++ b/lib/perl/OVH/Bastion/Helper.pm
@@ -3,6 +3,9 @@ package OVH::Bastion::Helper;
 # vim: set filetype=perl ts=4 sw=4 sts=4 et:
 use common::sense;
 
+use Fcntl qw{ :flock :mode };
+use Time::HiRes qw{ usleep };
+
 use File::Basename;
 use lib dirname(__FILE__) . '/../../../../lib/perl';
 use OVH::Bastion;
@@ -71,4 +74,64 @@ 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");
+    }
+    # 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");
+    }
+
+    # here, $lockdir is guaranteed to be owned only by us. but rogue files
+    # might have appeared in it after the mkdir and before the chown/chmod,
+    # so check for the lockfile existence. if it does exist, it must be a normal
+    # file and not a symlink or any other file type. Note that we don't have
+    # a TOCTTOU problem here because no rogue user can no longer create files
+    # in $lockdir, as we checked just above.
+    if (-l $lockfile || -e !-f $lockfile) {
+        warn_syslog("The $lockfile file exists but is not a file, unlinking it and bailing out");
+        unlink($lockfile);
+        # don't give too much info to the caller
+        return R('ERR_CANNOT_LOCK', msg => "Couldn't create lock file, please retry");
+    }
+
+    if (!open($fh, '>>', $lockfile)) {
+        return R('ERR_CANNOT_LOCK', msg => "Couldn't create lock file, please retry");
+    }
+    return R('OK', value => $fh);
+}
+
+sub acquire_lock {
+    my $fh = shift;
+    return R('ERR_INVALID_PARAMETER', msg => "Invalid filehandle") if !$fh;
+
+    # try to lock for at most 60 seconds
+    my $limit = time() + 60;
+    my $locked;
+    my $first = 1;
+    while (!($locked = flock($fh, LOCK_EX | LOCK_NB)) && time() < $limit) {
+        usleep(rand(200_000) + 100_000);    # sleep for 100-300ms
+        OVH::Bastion::osh_info("Acquiring lock, this may take a few seconds...") if $first;
+        $first = 0;
+    }
+    return R('OK') if $locked;
+    return R('KO_LOCK_FAILED', msg => "Couldn't acquire lock in a reasonable amount of time, please retry later");
+}
+
 1;