fix: add accountGidMin to avoid stealing an account's GID

Between account system groups (bearing the same GID number
than the UID they pertain to) and bastion groups, there
might be collisions on bastions with a very high amount
of both accounts and groups.

This is only of importance if you're using fixed UIDs
to create accounts, and can't let the system pick the
UIDs itself (for example because these UIDs are referenced
in some other system of your company).

This fix applies a GID shifting to all the bastion groups
to ensure they can never take a GID that would pertain to
a later-to-be-created account with a fixed GID.

This shift amount is configurable in bastion.conf as
``accountGidMin``, 500000 by default.

Use the updated bin/admin/fix-group-gid.sh script to shift any
preexisting group GID that would be out of the new groupGidMin range.

bin/admin/check-consistency.pl

@@ -4,12 +4,15 @@ use common::sense;
 use Data::Dumper;
 use Term::ANSIColor;
 use Digest::MD5 ();
-
 use File::Basename;
+
+use lib dirname(__FILE__) . '/../../lib/perl';
+use OVH::Bastion;
+
 my $BASEDIR = dirname(__FILE__) . '/../..';
 
-my $MIN_KEYGROUP_GID       = 2000;
-my $MAX_KEYGROUP_GID       = 99999;
+my $MIN_KEYGROUP_GID       = OVH::Bastion::config("groupGidMin")->value;
+my $MAX_KEYGROUP_GID       = $MIN_KEYGROUP_GID + 99999;
 my @KEY_GROUPS_IGNORE      = qw{ keeper reader };
 my $HOME_SUBDIRS_IGNORE_RE = qr{^^};
 

bin/admin/fix-group-gid.sh

@@ -1,12 +1,16 @@
 #! /usr/bin/env bash
 # vim: set filetype=sh ts=4 sw=4 sts=4 et:
+# This script ensures that GIDs of bastion groups (including their group system
+# roles) are higher than the configured groupGidMin value.
+# It shifts group GIDs from the low range (where they may collide with reserved
+# account UIDs) to the high range defined by groupGidMin.
 set -e
 
 basedir=$(readlink -f "$(dirname "$0")"/../..)
 # shellcheck source=lib/shell/functions.inc
 . "$basedir"/lib/shell/functions.inc
 
-MINGID=10000
+MINGID=$(perl -I"$basedir/lib/perl" -MOVH::Bastion -e 'print OVH::Bastion::config("groupGidMin")->value')
 
 if [ -n "$2" ] || [ -z "$1" ] ; then
     echo "Usage: $0 <groupname|ALL>"
@@ -34,13 +38,12 @@ _run()
     fi
 }
 
+next_available_gid=$((MINGID + 1))
 find_next_available_gid()
 {
-    nextgid=$((MINGID + 1))
-    while getent group "$nextgid" >/dev/null; do
-        nextgid=$((nextgid + 1))
+    while getent group "$next_available_gid" >/dev/null; do
+        next_available_gid=$((next_available_gid + 1))
     done
-    echo $nextgid
 }
 
 change_gid()
@@ -63,7 +66,8 @@ change_gid()
 
     [ "$oldgid" -ge "$MINGID" ] && return
 
-    newgid=$(find_next_available_gid)
+    find_next_available_gid
+    newgid=$next_available_gid
 
     _run group_change_gid_compat "$group" "$newgid"
     tocheck=""
@@ -106,13 +110,15 @@ main()
         return
     fi
 
-    echo
-    echo "$group: OK to proceed? (CTRL+C to abort). You'll still have to validate each commands I'm going to run"
-    # shellcheck disable=SC2034
-    read -r ___
-    really_run_commands=1
-    batchrun
-    echo "$group: done."
+    if [ -z "${DRY_RUN:-}" ] || [ "$DRY_RUN" = 0 ]; then
+        echo
+        echo "$group: OK to proceed? (CTRL+C to abort). You'll still have to validate each commands I'm going to run"
+        # shellcheck disable=SC2034
+        read -r ___
+        really_run_commands=1
+        batchrun
+        echo "$group: done."
+    fi
 }
 
 if [ "$1" = "ALL" ]; then

bin/helper/osh-groupCreate

@@ -153,11 +153,19 @@ if ($encrypted) {
 
 # First create group
 osh_info("Creating groups");
-foreach my $tocreate ($group, "$group-aclkeeper", "$group-gatekeeper", "$group-owner") {
-    $fnret = OVH::Bastion::sys_groupadd(group => $tocreate, noisy_stderr => 1);
+
+# Create all 4 groups with allocated GIDs to avoid collisions with account UIDs
+my @groups_to_create = ($group, "$group-aclkeeper", "$group-gatekeeper", "$group-owner");
+foreach my $tocreate (@groups_to_create) {
+    $fnret = OVH::Bastion::get_next_available_gid();
+    $fnret or HEXIT('ERR_GID_ALLOCATION_FAILED', msg => "Failed to allocate GID for group $tocreate: " . $fnret->msg);
+    my $gid = $fnret->value;
+
+    $fnret = OVH::Bastion::sys_groupadd(group => $tocreate, gid => $gid, noisy_stderr => 1);
     $fnret->err eq 'OK'
-      or
-      HEXIT('ERR_GROUPADD_FAILED', msg => "Error while running groupadd command for $tocreate (" . $fnret->msg . ")");
+      or HEXIT('ERR_GROUPADD_FAILED',
+        msg => "Error while running groupadd command for $tocreate with GID $gid (" . $fnret->msg . ")");
+    osh_debug("Created group $tocreate with GID $gid");
 }
 
 osh_debug("Creating directory");

doc/sphinx/administration/configuration/bastion_conf.rst

@@ -142,6 +142,7 @@ These options are either discouraged (in which case this is explained in the des
 
 - `accountUidMin`_
 - `accountUidMax`_
+- `groupGidMin`_
 - `ttyrecGroupIdOffset`_
 - `documentationURL`_
 - `debug`_
@@ -1007,6 +1008,17 @@ accountUidMax
 
 Maximum allowed UID for accounts on this bastion.
 
+.. _groupGidMin:
+
+groupGidMin
+***********
+
+:Type: ``int >= 10000``
+
+:Default: ``500000``
+
+Minimum GID for bastion groups. Should be set high enough (>> accountUidMax) to avoid collision with account UIDs/GIDs.
+
 .. _ttyrecGroupIdOffset:
 
 ttyrecGroupIdOffset

etc/bastion/bastion.conf.dist

@@ -472,6 +472,11 @@
 #  DEFAULT: 99999
 "accountUidMax": 99999,
 #
+# groupGidMin (int >= 10000)
+#     DESC: Minimum GID for bastion groups. Should be set high enough (>> accountUidMax) to avoid collision with account UIDs/GIDs.
+#  DEFAULT: 500000
+"groupGidMin": 500000,
+#
 # ttyrecGroupIdOffset (int > 0)
 #     DESC: Offset to apply on user group uid to create its ``-tty`` group, should be > ``accountUidMax - accountUidMin`` to ensure there is no overlap.
 #  DEFAULT: 100000

lib/perl/OVH/Bastion/allowkeeper.inc

@@ -153,6 +153,30 @@ sub get_next_available_uid {
     return R('ERR_UID_COLLISION', msg   => "No available UID in the allowed range");
 }
 
+sub get_next_available_gid {
+    my %params = @_;
+
+    # Get the minimum GID from configuration (should be >> accountUidMax to avoid collisions)
+    my $groupGidMin = OVH::Bastion::config('groupGidMin')->value();
+
+    # Start from groupGidMin and find the next available GID
+    my $next        = $groupGidMin;
+    my $maxAttempts = 100_000;        # safety limit
+    my $attempts    = 0;
+
+    while ($attempts < $maxAttempts) {
+        # Check if this GID is available
+        if (!scalar(getgrgid($next))) {
+            # Found an available GID
+            return R('OK', value => $next);
+        }
+        $next++;
+        $attempts++;
+    }
+
+    return R('ERR_GID_COLLISION', msg => "No available GID found in the allowed range starting from $groupGidMin");
+}
+
 sub is_bastion_account_valid_and_existing {
     my %params = @_;
     my $fnret  = OVH::Bastion::is_account_valid(%params);

lib/perl/OVH/Bastion/configuration.inc

@@ -190,28 +190,29 @@ sub load_configuration {
 
     # 2/6) Options that must be numbers, between min and max.
     foreach my $o (
-        {name => 'accountUidMin',                         min => 100,  max => 999_999_999, default => 2000},
-        {name => 'accountUidMax',                         min => 100,  max => 999_999_999, default => 99999},
-        {name => 'ttyrecGroupIdOffset',                   min => 1,    max => 999_999_999, default => 100_000},
-        {name => 'minimumIngressRsaKeySize',              min => 1024, max => 16384,       default => 2048},
-        {name => 'minimumEgressRsaKeySize',               min => 1024, max => 16384,       default => 2048},
-        {name => 'maximumIngressRsaKeySize',              min => 1024, max => 32768,       default => 8192},
-        {name => 'maximumEgressRsaKeySize',               min => 1024, max => 32768,       default => 8192},
-        {name => 'moshTimeoutNetwork',                    min => 0,    max => 86400 * 365, default => 86400},
-        {name => 'moshTimeoutSignal',                     min => 0,    max => 86400 * 365, default => 30},
-        {name => 'idleLockTimeout',                       min => 0,    max => 86400 * 365, default => 0},
-        {name => 'idleKillTimeout',                       min => 0,    max => 86400 * 365, default => 0},
-        {name => 'warnBeforeLockSeconds',                 min => 0,    max => 86400 * 365, default => 0},
-        {name => 'warnBeforeKillSeconds',                 min => 0,    max => 86400 * 365, default => 0},
-        {name => 'MFAPasswordInactiveDays',               min => -1,   max => 365 * 5,     default => -1},
-        {name => 'MFAPasswordMinDays',                    min => 0,    max => 365 * 5,     default => 0},
-        {name => 'MFAPasswordMaxDays',                    min => 0,    max => 365 * 5,     default => 90},
-        {name => 'MFAPasswordWarnDays',                   min => 0,    max => 365 * 5,     default => 15},
-        {name => 'sshClientDebugLevel',                   min => 0,    max => 3,           default => 0},
-        {name => 'accountMaxInactiveDays',                min => 0,    max => 365 * 5,     default => 0},
-        {name => 'interactiveModeTimeout',                min => 0,    max => 86400 * 365, default => 15},
-        {name => 'interactiveModeProactiveMFAexpiration', min => 0,    max => 86400,       default => 900},
-        {name => 'dnsSupportLevel',                       min => 0,    max => 2,           default => 2},
+        {name => 'accountUidMin',                         min => 100,   max => 999_999_999, default => 2000},
+        {name => 'accountUidMax',                         min => 100,   max => 999_999_999, default => 99999},
+        {name => 'groupGidMin',                           min => 10000, max => 999_999_999, default => 500_000},
+        {name => 'ttyrecGroupIdOffset',                   min => 1,     max => 999_999_999, default => 100_000},
+        {name => 'minimumIngressRsaKeySize',              min => 1024,  max => 16384,       default => 2048},
+        {name => 'minimumEgressRsaKeySize',               min => 1024,  max => 16384,       default => 2048},
+        {name => 'maximumIngressRsaKeySize',              min => 1024,  max => 32768,       default => 8192},
+        {name => 'maximumEgressRsaKeySize',               min => 1024,  max => 32768,       default => 8192},
+        {name => 'moshTimeoutNetwork',                    min => 0,     max => 86400 * 365, default => 86400},
+        {name => 'moshTimeoutSignal',                     min => 0,     max => 86400 * 365, default => 30},
+        {name => 'idleLockTimeout',                       min => 0,     max => 86400 * 365, default => 0},
+        {name => 'idleKillTimeout',                       min => 0,     max => 86400 * 365, default => 0},
+        {name => 'warnBeforeLockSeconds',                 min => 0,     max => 86400 * 365, default => 0},
+        {name => 'warnBeforeKillSeconds',                 min => 0,     max => 86400 * 365, default => 0},
+        {name => 'MFAPasswordInactiveDays',               min => -1,    max => 365 * 5,     default => -1},
+        {name => 'MFAPasswordMinDays',                    min => 0,     max => 365 * 5,     default => 0},
+        {name => 'MFAPasswordMaxDays',                    min => 0,     max => 365 * 5,     default => 90},
+        {name => 'MFAPasswordWarnDays',                   min => 0,     max => 365 * 5,     default => 15},
+        {name => 'sshClientDebugLevel',                   min => 0,     max => 3,           default => 0},
+        {name => 'accountMaxInactiveDays',                min => 0,     max => 365 * 5,     default => 0},
+        {name => 'interactiveModeTimeout',                min => 0,     max => 86400 * 365, default => 15},
+        {name => 'interactiveModeProactiveMFAexpiration', min => 0,     max => 86400,       default => 900},
+        {name => 'dnsSupportLevel',                       min => 0,     max => 2,           default => 2},
       )
     {
         if (not defined $C->{$o->{'name'}}) {
@@ -456,6 +457,19 @@ sub load_configuration {
         $C->{'ttyrecGroupIdOffset'} = $fixed;
     }
 
+    # ... groupGidMin must be high enough to avoid overlap with account UIDs and ttyrec groups
+    my $minRequiredGroupGid = $C->{'accountUidMax'} + $C->{'ttyrecGroupIdOffset'} + 1;
+    if ($C->{'groupGidMin'} < $minRequiredGroupGid) {
+        my $fixed = $minRequiredGroupGid;
+        push @errors,
+            "Configuration error: the configured 'groupGidMin' ("
+          . $C->{'groupGidMin'}
+          . ") would overlap with account UIDs and/or ttyrec GIDs (max ttyrec GID would be "
+          . ($C->{'accountUidMax'} + $C->{'ttyrecGroupIdOffset'})
+          . "), setting it to $fixed";
+        $C->{'groupGidMin'} = $fixed;
+    }
+
     # ... ensure min <= max
     foreach my $key (qw{ Ingress Egress }) {
         my $minkey = "minimum${key}RsaKeySize";