diff --git a/bin/dev/perl-critic.sh b/bin/dev/perl-critic.sh index 97b1c54..c0d04e4 100755 --- a/bin/dev/perl-critic.sh +++ b/bin/dev/perl-critic.sh @@ -9,8 +9,9 @@ cd "$basedir" || exit 1 action_doing "Checking perlcritic" # shellcheck disable=SC2086 -perlcritic --color -q -p "$(dirname "$0")"/perlcriticrc .; ret=$? -if [ "$ret" = 0 ]; then +perlcritic --color -q -p "$(dirname "$0")"/perlcriticrc .; ret1=$? +perlcritic --color -q -p "$(dirname "$0")"/perlcriticrc lib/perl/OVH/Bastion/*.inc; ret2=$? +if [ "$ret1" = 0 ] && [ "$ret2" = 0 ]; then # shellcheck disable=SC2119 action_done else diff --git a/bin/dev/perlcriticrc b/bin/dev/perlcriticrc index 501529d..dfca38a 100644 --- a/bin/dev/perlcriticrc +++ b/bin/dev/perlcriticrc @@ -19,6 +19,9 @@ terminal_funcs = HEXIT osh_exit osh_ok [ControlStructures::ProhibitDeepNests] max_nests = 6 +[Variables::ProhibitPackageVars] +packages = Data::Dumper File::Find FindBin Log::Log4perl DBI + [-BuiltinFunctions::ProhibitBooleanGrep] [-ControlStructures::ProhibitCascadingIfElse] [-ControlStructures::ProhibitPostfixControls] diff --git a/bin/plugin/restricted/accountCreate b/bin/plugin/restricted/accountCreate index ecceb41..262600a 100755 --- a/bin/plugin/restricted/accountCreate +++ b/bin/plugin/restricted/accountCreate @@ -104,7 +104,6 @@ if (!$pubKey && !$noKey) { } osh_info "\nIn any case, don't save it without a passphrase (your paste won't be echoed)."; $pubKey = ; - ## use critic } if (!$noKey) { diff --git a/lib/perl/OVH/Bastion/allowdeny.inc b/lib/perl/OVH/Bastion/allowdeny.inc index 947796f..58d89b5 100644 --- a/lib/perl/OVH/Bastion/allowdeny.inc +++ b/lib/perl/OVH/Bastion/allowdeny.inc @@ -349,7 +349,6 @@ sub get_user_groups { # build cache, it'll be faster than even one exec `id -nG` anyway setgrent(); - my %users; while (my ($name, $passwd, $gid, $members) = getgrent()) { foreach my $member (split / /, $members) { push @{$_cache_get_user_groups{$member}}, $name; @@ -509,7 +508,8 @@ sub print_acls { my $expiry = $entry->{'expiry'} ? (duration2human(seconds => ($entry->{'expiry'} - time()))->value->{'human'}) : '-'; # type => member ('full'), guest ('partial'), personal or legacy - my $ipReverse = OVH::Bastion::ip2host($entry->{'ip'})->value if $reverse; + my $ipReverse; + $ipReverse = OVH::Bastion::ip2host($entry->{'ip'})->value if $reverse; $entry->{'reverseDns'} = $ipReverse; push @flatArray, $entry; @@ -602,7 +602,7 @@ sub is_access_granted { # 0c/3 check if there are more complex "ingressToEgressRules" defined, and potentially bail out whether needed $fnret = OVH::Bastion::config('ingressToEgressRules'); my @rules = @{$fnret->value || []}; - for (my $ruleNb = 0 ; $ruleNb < @rules ; $ruleNb++) { + foreach my $ruleNb (0 .. $#rules) { my ($inNets, $outNets, $policy) = @{$rules[$ruleNb]}; $fnret = _is_in_any_net(ip => $ipfrom, networks => $inNets); @@ -850,20 +850,18 @@ sub ssh_test_access_way { $fnret = OVH::Bastion::execute(cmd => \@command, noisy_stderr => 1); $fnret or return $fnret; - no strict 'subs'; - if (grep { $fnret->value->{'sysret'} eq $_ } (0, OVH::Bastion::EXIT_ACCOUNT_INVALID, OVH::Bastion::EXIT_HOST_NOT_FOUND)) { + if (grep { $fnret->value->{'sysret'} eq $_ } (0, OVH::Bastion::EXIT_ACCOUNT_INVALID(), OVH::Bastion::EXIT_HOST_NOT_FOUND())) { return R('OK'); } - use strict 'subs'; my $hint; # 124 is the return code from the timeout system command when it times out # tested on Linux, NetBSD - if ($fnret->value->{'sysret'} == 124 || grep { $_ =~ /timed out/i } @{$fnret->value->{'stderr'} || []}) { + if ($fnret->value->{'sysret'} == 124 || grep { /timed out/i } @{$fnret->value->{'stderr'} || []}) { $hint = "Hint: did you remotely allow this bastion to access the SSH port?"; } - elsif (grep { $_ =~ /Permission denied/i } @{$fnret->value->{'stderr'} || []}) { + elsif (grep { /Permission denied/i } @{$fnret->value->{'stderr'} || []}) { $hint = "Hint: did you add the proper public key to the remote's authorized_keys?"; } my $msg = "Couldn't connect to $user\@$ip (ssh returned error " . $fnret->value->{'sysret'} . ")"; diff --git a/lib/perl/OVH/Bastion/allowkeeper.inc b/lib/perl/OVH/Bastion/allowkeeper.inc index c63013a..dc38574 100644 --- a/lib/perl/OVH/Bastion/allowkeeper.inc +++ b/lib/perl/OVH/Bastion/allowkeeper.inc @@ -13,7 +13,7 @@ sub is_user_in_group { my $user = $params{'user'} || OVH::Bastion::get_user_from_env()->value; # mandatory keys - if (!$user or !$group) { + if (!$user || !$group) { return R('ERR_MISSING_PARAMETER', msg => "Missing parameter 'user' or 'group'"); } @@ -80,9 +80,12 @@ sub is_valid_uid { return R('KO_BAD_RANGE', msg => "Parameter 'uid' should be between $low and $high"); } - $uid =~ m/^(\d+)$/; # Untaint - - return R('OK', value => $1); + # untaint + if ($uid =~ m/^(\d+)$/) { + return R('OK', value => $1); + } + warn_syslog("Got an invalid uid ('$uid')"); + return R('ERR_INVALID_UID', msg => "Got an invalid uid ('$uid')"); } sub get_next_available_uid { @@ -316,8 +319,12 @@ sub access_modify { } if ($ttl) { - return R('ERR_INVALID_PARAMETER', msg => "The TTL must be numeric") if ($ttl !~ /^(\d+)$/); - $ttl = $1; + if ($ttl =~ /^(\d+)$/) { + $ttl = $1; + } + else { + return R('ERR_INVALID_PARAMETER', msg => "The TTL must be numeric"); + } } # check if the caller has the right to make the change he's asking @@ -342,18 +349,20 @@ sub access_modify { push @one_should_succeed, OVH::Bastion::is_user_in_group(user => $requester, group => 'osh-account' . ucfirst($action) . 'PersonalAccess', sudo => 1); } elsif ($way eq 'group') { - $expected_running_as = "$group-aclkeeper"; + $expected_running_as = $group; push @one_should_succeed, OVH::Bastion::is_group_aclkeeper(account => $requester, group => $shortGroup, superowner => 1, sudo => 1); } elsif ($way eq 'groupguest') { push @one_should_succeed, OVH::Bastion::is_group_gatekeeper(account => $requester, group => $shortGroup, superowner => 1, sudo => 1); } - if (not defined $expected_running_as || $running_as ne $expected_running_as) { + if ($running_as ne $expected_running_as) { + warn_syslog("Security violation: current running user ($running_as) unexpected (wanted $expected_running_as)"); return R('ERR_SECURITY_VIOLATION', msg => "Current running user unexpected"); } if (grep({ $_ } @one_should_succeed) == 0 && $requester ne 'root') { + warn_syslog("Security violation: requesting user '$requester' doesn't have the right to do that (way=$way, group=" . ($shortGroup ? '' : $shortGroup) . ")"); return R('ERR_SECURITY_VIOLATION', msg => "You're not allowed to do that"); } @@ -393,7 +402,7 @@ sub access_modify { if (!(-e $file)) { # it doesn't exist yet, create it - OVH::Bastion::touch_file($file, 0644); + OVH::Bastion::touch_file($file, oct(644)); if (!(-e $file)) { return R('ERR_CANNOT_CREATE_FILE', msg => "File '$file' is missing and couldn't be created"); } @@ -654,7 +663,7 @@ sub get_group_list { my ($name, $passwd, $gid, $members) = @nextgroup; if ( $groupType eq 'key' and $name =~ /^key/ - and $name !~ /-(owner|gatekeeper|aclkeeper)$/ + and $name !~ /-(?:owner|gatekeeper|aclkeeper)$/ and not grep { $name eq $_ } qw{ keykeeper keyreader }) { $name =~ s/^key//; @@ -727,7 +736,7 @@ sub get_realm_list { # check if account is a bastion admin (gives access to adminXyz commands) # hint: an admin is also always a superowner -sub is_admin { +sub is_admin { ## no critic(Subroutines::RequireArgUnpacking) my %params = @_; my $sudo = $params{'sudo'}; # we're run under sudo my $account = $params{'account'}; @@ -761,7 +770,7 @@ sub is_admin { # check if account is a superowner # hint: an admin is also always a superowner -sub is_super_owner { +sub is_super_owner { ## no critic(Subroutines::RequireArgUnpacking) my %params = @_; my $sudo = $params{'sudo'}; # we're run under sudo my $account = $params{'account'}; @@ -796,7 +805,7 @@ sub is_super_owner { } # check if account is an auditor -sub is_auditor { +sub is_auditor { ## no critic(Subroutines::RequireArgUnpacking) my %params = @_; my $sudo = $params{'sudo'}; # we're run under sudo my $account = $params{'account'}; @@ -825,13 +834,14 @@ sub is_auditor { } # used by funcs below -sub _has_group_role { +sub _has_group_role { ## no critic(Subroutines::RequireArgUnpacking) my %params = @_; my $account = $params{'account'}; my $shortGroup = $params{'group'}; my $role = $params{'role'}; # regular or gatekeeper or owner my $superowner = $params{'superowner'}; # allow superowner (will always return yes if so) my $sudo = $params{'sudo'}; # are we run under sudo ? + my $fnret; if (not $account) { $account = $sudo ? $ENV{'SUDO_USER'} : OVH::Bastion::get_user_from_env()->value; @@ -860,12 +870,12 @@ sub _has_group_role { } # for the realm case, we need to test sysaccount and not just account - my $fnret = OVH::Bastion::is_bastion_account_valid_and_existing(account => $account); + $fnret = OVH::Bastion::is_bastion_account_valid_and_existing(account => $account); $fnret or return $fnret; my $sysaccount = $fnret->value->{'sysaccount'}; - my $fnret = OVH::Bastion::is_user_in_group(user => $sysaccount, group => $group); + $fnret = OVH::Bastion::is_user_in_group(user => $sysaccount, group => $group); osh_debug("is <$sysaccount> in <$group> ? => " . ($fnret ? 'yes' : 'no')); if ($fnret) { $fnret->{'value'} = {account => $account, sysaccount => $sysaccount}; @@ -968,8 +978,9 @@ sub get_remote_accounts_from_realm { my %accounts; if (opendir(my $dh, "/home/allowkeeper/$sysaccount")) { while (my $filename = readdir($dh)) { - next if $filename !~ /allowed_([a-zA-Z0-9._-]+)\.(ip|partial|private)/; - $accounts{$1} = 1; + if ($filename =~ /allowed_([a-zA-Z0-9._-]+)\.(ip|partial|private)/) { + $accounts{$1} = 1; + } } closedir($dh); } diff --git a/lib/perl/OVH/Bastion/configuration.inc b/lib/perl/OVH/Bastion/configuration.inc index c279a67..a71d486 100644 --- a/lib/perl/OVH/Bastion/configuration.inc +++ b/lib/perl/OVH/Bastion/configuration.inc @@ -113,17 +113,17 @@ sub load_configuration { $C->{'defaultLogin'} =~ s/[^a-zA-Z0-9_-]//g; $C->{'accountUidMin'} = 2000 if (not defined $C->{'accountUidMin'} or $C->{'accountUidMin'} !~ /^\d+$/); - $C->{'accountUidMin'} > 100 or $C->{'accountUidMin'} = 100; - $C->{'accountUidMin'} < 999999999 or $C->{'accountUidMin'} = 999999999; # usually 2^31-2 but well... + $C->{'accountUidMin'} > 100 or $C->{'accountUidMin'} = 100; + $C->{'accountUidMin'} < 999_999_999 or $C->{'accountUidMin'} = 999_999_999; # usually 2^31-2 but well... $C->{'accountUidMax'} = 99999 if (not defined $C->{'accountUidMax'} or $C->{'accountUidMax'} !~ /^\d+$/); - $C->{'accountUidMax'} > 100 or $C->{'accountUidMax'} = 100; - $C->{'accountUidMax'} < 999999999 or $C->{'accountUidMax'} = 999999999; # usually 2^31-2 but well... + $C->{'accountUidMax'} > 100 or $C->{'accountUidMax'} = 100; + $C->{'accountUidMax'} < 999_999_999 or $C->{'accountUidMax'} = 999_999_999; # usually 2^31-2 but well... $C->{'accountUidMax'} = $C->{'accountUidMin'} + 1000 if ($C->{'accountUidMin'} + 1000 > $C->{'accountUidMax'}); - $C->{'ttyrecGroupIdOffset'} = 100000 if (not defined $C->{'ttyrecGroupIdOffset'} or $C->{'ttyrecGroupIdOffset'} !~ /^\d+$/); - $C->{'ttyrecGroupIdOffset'} < 999999999 or $C->{'ttyrecGroupIdOffset'} = 999999999; + $C->{'ttyrecGroupIdOffset'} = 100_000 if (not defined $C->{'ttyrecGroupIdOffset'} or $C->{'ttyrecGroupIdOffset'} !~ /^\d+$/); + $C->{'ttyrecGroupIdOffset'} < 999_999_999 or $C->{'ttyrecGroupIdOffset'} = 999_999_999; if ($C->{'ttyrecGroupIdOffset'} < $C->{'accountUidMax'} - $C->{'accountUidMin'}) { # avoid overlap @@ -237,10 +237,7 @@ sub load_configuration { foreach my $key (qw{ ingressKeysFrom egressKeysFrom }) { s=[^0-9.:/]==g for @{$C->{$key}}; } - $C->{'adminAccounts'} = [ - grep { OVH::Bastion::is_bastion_account_valid_and_existing(account => $_) } - map { s/[^a-zA-Z0-9_-]//g; $_ } @{$C->{'adminAccounts'}} - ]; + $C->{'adminAccounts'} = [grep { OVH::Bastion::is_bastion_account_valid_and_existing(account => $_) } @{$C->{'adminAccounts'}}]; $C->{'documentationURL'} ||= "https://ovh.github.io/the-bastion/"; @@ -335,9 +332,9 @@ sub account_config { if (!open($fh, '<', $filename)) { return R('ERR_CANNOT_OPEN_FILE', msg => "Error while trying to open file $filename for read ($!)"); } - my $value = do { local $/; <$fh> }; + my $getvalue = do { local $/ = undef; <$fh> }; close($fh); - return R('OK', value => $value); + return R('OK', value => $getvalue); } return R('ERR_INTERNAL'); # we shouldn't be here @@ -461,7 +458,12 @@ sub group_config { } print $fh $value; close($fh); - chmod($secret ? 0640 : 0644), $filename; + if ($secret) { + chmod 0640, $filename; + } + else { + chmod 0644, $filename; + } # need to chown to group:group my (undef, undef, $groupuid, $groupgid) = getpwnam($group); @@ -499,7 +501,7 @@ sub json_load { # Load file content my $rawConf; if (open(my $fh, '<', $file)) { - foreach (<$fh>) { + while (<$fh>) { chomp; s/^((?:(?:[^"]*"){2}|[^"]*)*[^"]*)\/\/.*$/$1/; # Remove comment that start with // /^\s*#/ and next; # Comment start with ^# diff --git a/lib/perl/OVH/Bastion/execute.inc b/lib/perl/OVH/Bastion/execute.inc index cab0a60..20596b0 100644 --- a/lib/perl/OVH/Bastion/execute.inc +++ b/lib/perl/OVH/Bastion/execute.inc @@ -38,6 +38,7 @@ sub _sysret2human { } } +## no critic(ControlStructures::ProhibitDeepNests) sub execute { my %params = @_; my $cmd = $params{'cmd'}; # command to execute, must be an array ref (with possible parameters) @@ -69,13 +70,12 @@ sub execute { #=cut only to debug tainted stuff require Scalar::Util; foreach (@$cmd) { - if (Scalar::Util::tainted($_)) { + if (Scalar::Util::tainted($_) && /(.+)/) { # to be able to warn under -T; untaint it. we're going to crash right after anyway. - my ($untainted) = $_ =~ /(.+)/; require Carp; - warn(Carp::longmess("would exec <" . join('^', @$cmd) . "> but param '$untainted' is tainted!")); - osh_warn("about to execute a cmd but param '$untainted' is tainted, I'm gonna crash!"); + warn(Carp::longmess("would exec <" . join('^', @$cmd) . "> but param '$1' is tainted!")); + osh_warn("about to execute a cmd but param '$1' is tainted, I'm gonna crash!"); } } @@ -85,7 +85,7 @@ sub execute { my $child_exit_status = system(@$cmd); $fnret = _sysret2human($child_exit_status); return R( - $child_exit_status eq 0 ? 'OK' : ($must_succeed ? 'ERR_NON_ZERO_EXIT' : 'OK_NON_ZERO_EXIT'), + $child_exit_status == 0 ? 'OK' : ($must_succeed ? 'ERR_NON_ZERO_EXIT' : 'OK_NON_ZERO_EXIT'), value => { sysret => $child_exit_status + 0, status => $fnret->value->{'status'}, @@ -137,7 +137,7 @@ sub execute { } # then, while we still have fh to monitor - while ($select->count() > 1 || ($select->count() == 1 && not $select->exists(\*STDIN))) { + while ($select->count() > 1 || ($select->count() == 1 && !$select->exists(\*STDIN))) { # block only for 50ms, before checking if child is dead my @ready = $select->can_read(0.05); @@ -314,7 +314,7 @@ sub execute { $fnret = _sysret2human($child_exit_status); osh_debug("cmd returned with " . $fnret->msg); return R( - $fnret->value->{'status'} eq 0 ? 'OK' : ($must_succeed ? 'ERR_NON_ZERO_EXIT' : 'OK_NON_ZERO_EXIT'), + $fnret->value->{'status'} == 0 ? 'OK' : ($must_succeed ? 'ERR_NON_ZERO_EXIT' : 'OK_NON_ZERO_EXIT'), value => { sysret => $child_exit_status >> 8, stdout => $output{stdout}, diff --git a/lib/perl/OVH/Bastion/interactive.inc b/lib/perl/OVH/Bastion/interactive.inc index 42622be..ea13079 100644 --- a/lib/perl/OVH/Bastion/interactive.inc +++ b/lib/perl/OVH/Bastion/interactive.inc @@ -20,7 +20,8 @@ sub interactive { my $interactiveModeTimeout = OVH::Bastion::config('interactiveModeTimeout')->value() || 0; my $slaveOrMaster = (OVH::Bastion::config('readOnlySlaveMode')->value() ? 'slave' : 'master'); - my $term = Term::ReadLine->new('Bastion Interactive'); + my $term = Term::ReadLine->new('Bastion Interactive'); + ## no critic(ValuesAndExpressions::ProhibitEscapedCharacters) my $prompt = "" . "\001\033[0m\033[33m\002" . $self @@ -37,7 +38,7 @@ sub interactive { my $prompt_non_readline = $prompt; $prompt_non_readline =~ s=\001|\002==g; - print < and for autocompletion. @@ -51,7 +52,7 @@ EOM $fnret or return (); my $pluginList = $fnret->value; - my @cmdlist = ('exit', 'ssh'); + my @cmdlist = qw{ exit ssh }; foreach my $plugin (sort keys %$pluginList) { $fnret = OVH::Bastion::can_account_execute_plugin(plugin => $plugin, account => $self); next if !$fnret; @@ -113,9 +114,9 @@ EOM return @validcmds; } - for (my $i = 0 ; $i < @rules ; $i += 2) { - my $re = $rules[$i]; - my $item = $rules[$i + 1]; + foreach my $i (0 .. $#rules) { + my $re = $rules[$i++]; + my $item = $rules[$i++]; next unless ($line =~ m{^$re\s*$} or $line =~ m{^$re\s\Q$word\E\s*$}); @@ -132,35 +133,35 @@ EOM my @autocomplete; foreach (@{$item->{'ac'}}) { if ($_ eq '') { - my $fnret = OVH::Bastion::get_account_list(cache => 1); + $fnret = OVH::Bastion::get_account_list(cache => 1); if ($fnret) { push @autocomplete, sort keys %{$fnret->value()}; next; } } elsif ($_ eq '') { - my $fnret = OVH::Bastion::get_group_list(cache => 1, groupType => 'key'); + $fnret = OVH::Bastion::get_group_list(cache => 1, groupType => 'key'); if ($fnret) { push @autocomplete, sort keys %{$fnret->value()}; next; } } elsif ($_ eq '') { - my $fnret = OVH::Bastion::get_realm_list(); + $fnret = OVH::Bastion::get_realm_list(); if ($fnret) { push @autocomplete, sort keys %{$fnret->value()}; next; } } elsif ($_ eq '') { - my $fnret = OVH::Bastion::get_plugin_list(restrictedOnly => 1); + $fnret = OVH::Bastion::get_plugin_list(restrictedOnly => 1); if ($fnret) { push @autocomplete, 'auditor', sort keys %{$fnret->value()}; next; } } elsif ($_ eq '') { - my $fnret = OVH::Bastion::get_plugin_list(); + $fnret = OVH::Bastion::get_plugin_list(); if ($fnret) { push @autocomplete, sort keys %{$fnret->value()}; next; @@ -230,7 +231,7 @@ EOM chomp $fnret->value->{'stdout'}->[0]; %after = map { $_ => 1 } split(/ /, $fnret->value->{'stdout'}->[0]); } - my @newgroups = grep { !exists $before{$_} && $_ !~ /^mfa-/ } keys %after; + my @newgroups = grep { !exists $before{$_} && !/^mfa-/ } keys %after; if (@newgroups) { osh_warn("IMPORTANT: You have been added to new groups since the session started."); osh_warn("You'll need to logout/login again from this interactive session to have"); diff --git a/lib/perl/OVH/Bastion/log.inc b/lib/perl/OVH/Bastion/log.inc index 29774f4..a8c6452 100644 --- a/lib/perl/OVH/Bastion/log.inc +++ b/lib/perl/OVH/Bastion/log.inc @@ -38,6 +38,7 @@ sub syslog_close { Sys::Syslog::closelog(); $_syslog_inited = 0; } + return; } END { @@ -135,7 +136,8 @@ sub _sql_update_db { # create table $result = $dbh->do( $sqltype eq 'local' - ? "CREATE TABLE IF NOT EXISTS connections( + ? <<'EOS' + CREATE TABLE IF NOT EXISTS connections( id INTEGER PRIMARY KEY, timestamp INTEGER, timestampusec INTEGER, @@ -159,8 +161,10 @@ sub _sql_update_db { timestampendusec INTEGER, returnvalue INTEGER, comment TEXT, - uniqid TEXT)" - : "CREATE TABLE IF NOT EXISTS connections_summary( + uniqid TEXT) +EOS + : <<'EOS' + CREATE TABLE IF NOT EXISTS connections_summary( id INTEGER PRIMARY KEY, timestamp INTEGER, account TEXT, @@ -171,7 +175,8 @@ sub _sql_update_db { portto INTEGER, user TEXT, plugin TEXT, - uniqid TEXT)" + uniqid TEXT) +EOS ); return R('KO', msg => "creating table ($sqltype)") if not $result; @@ -235,7 +240,7 @@ sub _sql_log_insert_file { # create the file ourselves, and set it rw rw rw (for global log) # -journal -shm and -wal files are created with same perms by sqlite - OVH::Bastion::touch_file($file, 0666); + OVH::Bastion::touch_file($file, oct(666)); } # big db-related retry block: @@ -249,7 +254,7 @@ sub _sql_log_insert_file { foreach my $retry (0 .. 19) { # if we're retrying, sleep a bit before, to ease concurrency - select(undef, undef, undef, $retry / 50 + rand() / 10) if $retry; + sleep($retry / 50 + rand() / 10) if $retry; # on each retry, clean those vars (undef $dbh disconnects if connected) undef $dbh; @@ -274,17 +279,17 @@ sub _sql_log_insert_file { my $prepare; my @execute; if ($sqltype eq 'local') { - $prepare = "INSERT INTO connections -(uniqid,timestamp,timestampusec,account,cmdtype,allowed,hostfrom,ipfrom,portfrom,bastionip,bastionport,hostto,ipto,portto,user,plugin,params,comment,ttyrecfile) -VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)"; + $prepare = + "INSERT INTO connections" + . "(uniqid,timestamp,timestampusec,account,cmdtype,allowed,hostfrom,ipfrom,portfrom,bastionip,bastionport,hostto,ipto,portto,user,plugin,params,comment,ttyrecfile)" + . "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)"; @execute = ( $uniqid, $timestamp, $timestampusec, $account, $cmdtype, $allowed, $hostfrom, $ipfrom, $portfrom, $bastionip, $bastionport, $hostto, $ipto, $portto, $user, $plugin, $params, $comment, $ttyrecfile ); } elsif ($sqltype eq 'global') { - $prepare = "INSERT INTO connections_summary (uniqid,timestamp,account,cmdtype,allowed,ipfrom,ipto,portto,user,plugin) -VALUES (?,?,?,?,?,?,?,?,?,?)"; + $prepare = "INSERT INTO connections_summary (uniqid,timestamp,account,cmdtype,allowed,ipfrom,ipto,portto,user,plugin)" . "VALUES (?,?,?,?,?,?,?,?,?,?)"; @execute = ($uniqid, $timestamp, $account, $cmdtype, $allowed, $ipfrom, $ipto, $portto, $user, $plugin); } @@ -457,7 +462,7 @@ sub _sql_log_update_file { foreach my $retry (0 .. 19) { # if we're retrying, sleep a bit before, to ease concurrency - select(undef, undef, undef, $retry / 50 + rand() / 10) if $retry; + sleep($retry / 50 + rand() / 10) if $retry; # on each retry, clean those vars (undef $dbh disconnects if connected) undef $dbh; @@ -517,7 +522,7 @@ sub _sql_log_update_file { foreach my $retry (0 .. 19) { # if we're retrying, sleep a bit before, to ease concurrency - select(undef, undef, undef, $retry / 50 + rand() / 10) if $retry; + sleep($retry / 50 + rand() / 10) if $retry; # on each retry, clean those vars (undef $dbh disconnects if connected) undef $dbh; @@ -533,12 +538,13 @@ sub _sql_log_update_file { } $sth = $dbh->prepare( - "CREATE TABLE IF NOT EXISTS plugincalls( - id INTEGER PRIMARY KEY, - connection_id INTEGER UNIQUE, - stdout TEXT, - stderr TEXT) - " + <<'EOS' + CREATE TABLE IF NOT EXISTS plugincalls( + id INTEGER PRIMARY KEY, + connection_id INTEGER UNIQUE, + stdout TEXT, + stderr TEXT) +EOS ); if (!$sth) { $doing = "creating plugins table (prepare)"; @@ -635,8 +641,9 @@ sub log_access_get { # first, check in account sql file if (OVH::Bastion::config('enableAccountSqlLog')->value()) { - $params{'file'} = sprintf("/home/%s/%s-log-%04d%02d.sqlite", $params{'account'}, $params{'account'}, $localtime[5] + 1900, $localtime[4] + 1), $params{'sqltype'} = 'local'; - $fnret = _sql_log_fetch_from_file(%params); + $params{'file'} = sprintf("/home/%s/%s-log-%04d%02d.sqlite", $params{'account'}, $params{'account'}, $localtime[5] + 1900, $localtime[4] + 1); + $params{'sqltype'} = 'local'; + $fnret = _sql_log_fetch_from_file(%params); return $fnret; } @@ -644,8 +651,8 @@ sub log_access_get { =cut not now, too slow and table columns differ if (OVH::Bastion::config('enableGlobalSqlLog')->value()) { - $params{'file'} = sprintf("/home/logkeeper/global-log-%04d.sqlite", $localtime[5] + 1900), - $params{'sqltype'} = 'global', + $params{'file'} = sprintf("/home/logkeeper/global-log-%04d.sqlite", $localtime[5] + 1900); + $params{'sqltype'} = 'global'; $fnret = _sql_log_fetch_from_file(%params); return $fnret; } @@ -764,7 +771,7 @@ sub _sql_log_fetch_from_file { foreach my $retry (0 .. 19) { # if we're retrying, sleep a bit before, to ease concurrency - select(undef, undef, undef, $retry / 50 + rand() / 10) if $retry; + sleep($retry / 50 + rand() / 10) if $retry; # on each retry, clean those vars (undef $dbh disconnects if connected) undef $dbh; diff --git a/lib/perl/OVH/Bastion/mock.inc b/lib/perl/OVH/Bastion/mock.inc index d65d264..96f6265 100644 --- a/lib/perl/OVH/Bastion/mock.inc +++ b/lib/perl/OVH/Bastion/mock.inc @@ -9,6 +9,7 @@ my $_mock_data; sub enable_mocking { $_mocking_enabled = 1; + return; } sub is_mocking { @@ -18,6 +19,7 @@ sub is_mocking { sub set_mock_data { die "tried to set_mock_data without enabling mocking first" unless is_mocking(); $_mock_data = shift; + return; } sub mock_get_account_entry { diff --git a/lib/perl/OVH/Bastion/os.inc b/lib/perl/OVH/Bastion/os.inc index e7e959b..0507c52 100644 --- a/lib/perl/OVH/Bastion/os.inc +++ b/lib/perl/OVH/Bastion/os.inc @@ -45,7 +45,7 @@ sub _sys_autoretry { my %params = @_; my $fnret; - for (my $try = 1 ; $try < 10 ; $try++) { + foreach my $try (1 .. 10) { $fnret = OVH::Bastion::execute(%params); if ( ($fnret->value && $fnret->value->{'sysret'} == 10) || ($fnret->value && $fnret->value->{'stdout'} && grep { /retry|lock/i } @{$fnret->value->{'stdout'}}) @@ -309,7 +309,7 @@ sub sys_delmemberfromgroup { if (open(my $fh, '<', '/etc/group')) { while (<$fh>) { if (/^\Q$group\E:/) { - s/([:,])\Q$user\E(,|$)/$1/; + s/([:,])\Q$user\E(?:,|$)/$1/; s/,$//; } $contents .= $_; @@ -391,7 +391,7 @@ sub sys_neutralizepassword { my $stdin_str; if (is_linux()) { - @cmd = ('chpasswd', '-e'); + @cmd = qw{ chpasswd -e }; $stdin_str = "$user:*"; } elsif (is_freebsd()) { @@ -461,8 +461,12 @@ sub sys_getpasswordinfo { $fnret = OVH::Bastion::execute(cmd => ['getent', 'shadow', $user]); $fnret or return $fnret; return R('KO_NOT_FOUND') if ($fnret->value->{'sysret'} != 0); - return R('ERR_CANNOT_PARSE_SHADOW') if ($fnret->value->{'stdout'}->[0] !~ m{^([^:]*):([^:]*):([^:]*):([^:]*):([^:]*):([^:]*):([^:]*):([^:]*):([^:]*)$}); - %ret = (user => $1, password => $2, epoch_changed_days => $3, min_days => $4, max_days => $5, warn_days => $6, inactive_days => $7, epoch_disabled_days => $8); + if ($fnret->value->{'stdout'}->[0] =~ m{^([^:]*):([^:]*):([^:]*):([^:]*):([^:]*):([^:]*):([^:]*):([^:]*):([^:]*)$}) { + %ret = (user => $1, password => $2, epoch_changed_days => $3, min_days => $4, max_days => $5, warn_days => $6, inactive_days => $7, epoch_disabled_days => $8); + } + else { + return R('ERR_CANNOT_PARSE_SHADOW'); + } } elsif (is_bsd()) { @@ -473,8 +477,12 @@ sub sys_getpasswordinfo { close($masterfd); my @userlines = grep { /^\Q$user\E:/ } @lines; return R('KO_NOT_FOUND') if (@userlines != 1); - return R('ERR_CANNOT_PARSE_SHADOW') if ($userlines[0] !~ m{^([^:]*):([^:]*)}); - %ret = (user => $1, password => $2); + if ($userlines[0] =~ m{^([^:]*):([^:]*)}) { + %ret = (user => $1, password => $2); + } + else { + return R('ERR_CANNOT_PARSE_SHADOW'); + } } else { return R('ERR_CANNOT_READ_FILE', msg => "Couldn't open /etc/master.passwd: $!"); @@ -545,7 +553,7 @@ sub sys_setfacl { my @perms; foreach (@{$fnret->value->{'stdout'}}) { chomp; - /^((user|group|other)::...)$/ or next; + /^((?:user|group|other)::...)$/ or next; push @perms, $1; # untaint } if (@perms != 3) { diff --git a/lib/perl/OVH/Bastion/ssh.inc b/lib/perl/OVH/Bastion/ssh.inc index 2b3121c..b8423e7 100644 --- a/lib/perl/OVH/Bastion/ssh.inc +++ b/lib/perl/OVH/Bastion/ssh.inc @@ -201,12 +201,13 @@ sub get_ssh_pub_key_info { return R('KO_PRIVATE_KEY'); } - if ($pubKey !~ m{^\s*((\S+)\s+)?(ssh-dss|ssh-rsa|ecdsa-sha\d+-nistp\d+|ssh-ed\d+)\s+([a-zA-Z0-9/=+]+)(\s+(.{1,128}))?$} - || length($pubKey) > 3000) - { + my ($prefix, $typecode, $base64, $comment); + if ($pubKey =~ m{^\s*((\S+)\s+)?(ssh-dss|ssh-rsa|ecdsa-sha\d+-nistp\d+|ssh-ed\d+)\s+([a-zA-Z0-9/=+]+)(\s+(.{1,128})?)?$} && length($pubKey) <= 3000) { + ($prefix, $typecode, $base64, $comment) = ($2, $3, $4, $6); + } + else { return R('KO_NOT_A_KEY', value => {line => $pubKey}); } - my ($prefix, $typecode, $base64, $comment) = ($2, $3, $4, $6); my $line = "$typecode $base64"; $prefix = '' if not defined $prefix; $line .= " " . $comment if $comment; @@ -231,7 +232,7 @@ sub get_ssh_pub_key_info { print {$fh} $typecode . " " . $base64; close($fh); $fnret = OVH::Bastion::execute(cmd => ['ssh-keygen', '-l', '-f', $filename]); - if ($fnret->is_err || not $fnret->value || ($fnret->value->{'sysret'} != 0 && $fnret->value->{'sysret'} != 1)) { + if ($fnret->is_err || !$fnret->value || ($fnret->value->{'sysret'} != 0 && $fnret->value->{'sysret'} != 1)) { # sysret == 1 means ssh-keygen didn't recognize this key, handled below. return R('ERR_SSH_KEYGEN_FAILED', msg => "Couldn't read the fingerprint of $filename (" . $fnret->msg . ")"); @@ -258,8 +259,8 @@ sub get_ssh_pub_key_info { if (grep { "$family-$size" eq $_ } @blacklistfiles) { # check for vulnkeys - my $file = '/usr/share/ssh/blacklist.' . $family . '-' . $size; - if (-r $file && open(my $fh_blacklist, '<', $file)) { + my $blfile = '/usr/share/ssh/blacklist.' . $family . '-' . $size; + if (-r $blfile && open(my $fh_blacklist, '<', $blfile)) { my $shortfp = $fingerprint; $shortfp =~ s/://g; $shortfp =~ s/^.{12}//; @@ -277,7 +278,7 @@ sub get_ssh_pub_key_info { # check allowed algos and key size my $allowedSshAlgorithms = OVH::Bastion::config("allowed${way}SshAlgorithms"); my $minimumRsaKeySize = OVH::Bastion::config("minimum${way}RsaKeySize"); - if ($allowedSshAlgorithms && not grep { lc($return{'family'}) eq $_ } @{$allowedSshAlgorithms->value}) { + if ($allowedSshAlgorithms && !grep { lc($return{'family'}) eq $_ } @{$allowedSshAlgorithms->value}) { return R('KO_FORBIDDEN_ALGORITHM', value => \%return); } if ($minimumRsaKeySize && lc($return{'family'}) eq 'rsa' && $minimumRsaKeySize->value > $return{'size'}) { @@ -312,19 +313,19 @@ sub is_valid_public_key { if ($fnret->err eq 'KO_PRIVATE_KEY') { # n00b check - $fnret->{'msg'} = <{'msg'} = <<'EOS'; HOLY SH*T, did you just paste your PRIVATE KEY?!! You have no idea what you are doing, do you? No matter what, don't do that ever again. Hint: in 'private key', the most important word is 'private', which means, well, you know, NOT PUBLIC. EOS } elsif ($fnret->err eq 'KO_NOT_A_KEY') { - $fnret->{'msg'} = <{'msg'} = <<'EOS'; This doesn't look like an SSH public key, accepted formats are RSA (>= 2048 bits) and if supported by the OS, ECDSA and Ed25519. EOS } elsif ($fnret->err eq 'KO_VULNERABLE_KEY') { - $fnret->{'msg'} = <{'msg'} = <<'EOS'; This key is COMPROMISED due to tue Debian OpenSSL 2008 debacle (aka CVE-2008-0166). *********************************************** DO NOT USE THIS KEY ANYWHERE, IT IS VULNERABLE! @@ -460,8 +461,8 @@ sub generate_ssh_key { $fnret->err eq 'OK' or return R('ERR_SSH_KEYGEN_FAILED', msg => "Error while generating group key (" . $fnret->msg . ")"); my %files = ( - $sshKeyName => ($group_readable ? 0440 : 0400), - $sshKeyName . '.pub' => 0444, + $sshKeyName => ($group_readable ? oct(440) : oct(400)), + $sshKeyName . '.pub' => oct(444), ); while (my ($file, $chmod) = each(%files)) { if (not -e $file) { @@ -580,7 +581,7 @@ sub is_allowed_algo_and_size { return R('KO_KEY_SIZE_INVALID', msg => "For the selected algorithm, valid key sizes are 256, 384, 521"); } } - elsif ($algo eq 'ed25519' and $size and $size ne 256) { + elsif ($algo eq 'ed25519' && $size && $size ne '256') { return R('KO_KEY_SIZE_INVALID', msg => "For the selected algorithm, key size must be 256"); } return R('OK'); @@ -649,6 +650,7 @@ sub print_public_key { osh_info(Term::ANSIColor::colored('keyline', 'red') . ' follows, please copy the *whole* line:'); print($line. "\n"); osh_info(' '); + return; } sub account_ssh_config_get { @@ -678,7 +680,7 @@ sub account_ssh_config_get { } # remove empty lines & comments - my @lines = grep { /./ && !/^\s*#/ } split("\n", $sshconfig_data); + my @lines = grep { /./ && !/^\s*#/ } split(/\n/, $sshconfig_data); # lowercase all keys my %keys = map { m/^(\S+)\s+(.+)$/ ? (lc($1) => $2) : () } @lines;