From 8a73639f3ddfa7d5e1633b7e59ff2c22bf8b411a Mon Sep 17 00:00:00 2001 From: Stephane Lesimple Date: Mon, 26 Jan 2026 10:17:59 +0100 Subject: [PATCH] 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 | 9 ++- bin/admin/fix-group-gid.sh | 32 +++++----- bin/helper/osh-groupCreate | 16 +++-- .../configuration/bastion_conf.rst | 12 ++++ etc/bastion/bastion.conf.dist | 5 ++ lib/perl/OVH/Bastion/allowkeeper.inc | 24 ++++++++ lib/perl/OVH/Bastion/configuration.inc | 58 ++++++++++++------- 7 files changed, 114 insertions(+), 42 deletions(-) diff --git a/bin/admin/check-consistency.pl b/bin/admin/check-consistency.pl index 75c23f8..a8d6f36 100755 --- a/bin/admin/check-consistency.pl +++ b/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{^^}; diff --git a/bin/admin/fix-group-gid.sh b/bin/admin/fix-group-gid.sh index 4c1de70..fbca242 100755 --- a/bin/admin/fix-group-gid.sh +++ b/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 " @@ -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 diff --git a/bin/helper/osh-groupCreate b/bin/helper/osh-groupCreate index d4c8ff9..b2f5435 100755 --- a/bin/helper/osh-groupCreate +++ b/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"); diff --git a/doc/sphinx/administration/configuration/bastion_conf.rst b/doc/sphinx/administration/configuration/bastion_conf.rst index 540da34..5295852 100644 --- a/doc/sphinx/administration/configuration/bastion_conf.rst +++ b/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 diff --git a/etc/bastion/bastion.conf.dist b/etc/bastion/bastion.conf.dist index 0961fe4..b008c6f 100644 --- a/etc/bastion/bastion.conf.dist +++ b/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 diff --git a/lib/perl/OVH/Bastion/allowkeeper.inc b/lib/perl/OVH/Bastion/allowkeeper.inc index 2fb0a0c..fda6058 100644 --- a/lib/perl/OVH/Bastion/allowkeeper.inc +++ b/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); diff --git a/lib/perl/OVH/Bastion/configuration.inc b/lib/perl/OVH/Bastion/configuration.inc index 4696527..cab1d0d 100644 --- a/lib/perl/OVH/Bastion/configuration.inc +++ b/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";