fix/safe_pf_run (#8201)

* remove redundant test * use shell_quote * Add safe_pf_run * Change pf_run to safe_pf_run where there was no redirect of stdin * Add redirecting to stdout * Add the ability to append to stdout * Use safe_pf_run for commands that use use stdin stdout * Don't quote vars * Update docs * remove syntax error * cleanup * cleanup * cleanup * cleanup * cleanup * cleanup
inverse-inc · Jul 11, 2024 · d649f07 · d649f07
1 parent 1f1e871
commit d649f07
Show file tree

Hide file tree

Showing 25 changed files with 285 additions and 249 deletions.
diff --git a/html/pfappserver/lib/pfappserver/Model/Config/System.pm b/html/pfappserver/lib/pfappserver/Model/Config/System.pm
@@ -101,7 +101,7 @@ sub setDefaultRoute {
 
     my $cmd = "sudo ip route replace to default via $gateway 2>&1";
     $logger->debug("Replace default gateway: $cmd");
-    $status = pf_run($cmd, accepted_exit_status => [ $_EXIT_CODE_EXISTS ]);
+    $status = safe_pf_run(qw(sudo ip route replace to default via), $gateway , {accepted_exit_status => [ $_EXIT_CODE_EXISTS ]});
 
     # Everything goes as expected
     if (defined($status)) {
@@ -137,7 +137,7 @@ sub start_mysqld_service {
     # please keep LANG=C in case we need to fetch the output of the command
     my $cmd = "setsid sudo service $DB_SERVICE_NAME start 2>&1";
     $logger->debug("Starting $DB_SERVICE_NAME service: $cmd");
-    $status = pf_run($cmd);
+    $status = safe_pf_run(qw(setsid sudo service), $DB_SERVICE_NAME, 'start');
 
     # Everything goes as expected
     if ( defined($status) ) {
@@ -165,7 +165,7 @@ sub restart_pfconfig {
 
     my $cmd = "setsid sudo service packetfence-config restart 2>&1";
     $logger->debug("Restarting packetfence-config service: $cmd");
-    $status = pf_run($cmd);
+    $status = safe_pf_run(qw(setsid sudo service packetfence-config restart));
 
     # Everything goes as expected
     if ( defined($status) ) {

diff --git a/html/pfappserver/lib/pfappserver/Model/DB.pm b/html/pfappserver/lib/pfappserver/Model/DB.pm
@@ -255,10 +255,10 @@ sub make_mysql_command {
         return make_remote_mysql_command($args);
     }
 
-    my $user = quotemeta ($args->{username});
-    my $password = quotemeta ($args->{password});
+    my $user = $args->{username};
+    my $password = $args->{password};
     my $db = quotemeta ($args->{database});
-    my $mysql_cmd = "/usr/bin/mysql --socket=/var/lib/mysql/mysql.sock -u $user -p$password $db";
+    my $mysql_cmd = ['/usr/bin/mysql', '--socket=/var/lib/mysql/mysql.sock', '-u', $user, "-p$password", $db];
     return ($mysql_cmd, undef, "-p$password");
 }
 
@@ -307,9 +307,8 @@ sub apply_schema {
 
     my ( $status_msg, $result );
     my ($mysql_cmd, $fh, $log_strip) = make_mysql_command($args);
-    my $cmd = "$mysql_cmd < $install_dir/db/pf-schema.sql";
     my $db = $args->{database};
-    eval { $result = pf_run($cmd, (accepted_exit_status => [ 0 ]), log_strip => $log_strip) };
+    eval { $result = safe_pf_run(@$mysql_cmd, {stdin => "$install_dir/db/pf-schema.sql", accepted_exit_status => [ 0 ], log_strip => $log_strip}) };
     if ( $@ || !defined($result) ) {
         $status_msg = ["Error applying the schema to the database [_1]", $db ];
         $logger->warn("$@: $result");
@@ -318,8 +317,7 @@ sub apply_schema {
     my @custom_schemas = read_dir( "$install_dir/db/custom", prefix => 1, err_mode => 'quiet' ) ;
     @custom_schemas = sort @custom_schemas;
     foreach my $custom_schema (@custom_schemas) {
-        my $cmd = "$mysql_cmd < $custom_schema";
-        eval { $result = pf_run($cmd, (accepted_exit_status => [ 0 ]), log_strip => $log_strip) };
+        eval { $result = safe_pf_run(@$mysql_cmd, {stdin => $custom_schema, accepted_exit_status => [ 0 ], log_strip => $log_strip}) };
         if ( $@ || !defined($result) ) {
             $status_msg = ["Error applying the custom schema $custom_schema to the database [_1]", $db ];
             $logger->warn("$@: $result");
@@ -438,9 +436,9 @@ sub secureInstallation {
 
 =head2 schema
 
-TODO: Check error handling for pf_run... (undef or whatever)
+TODO: Check error handling for safe_pf_run... (undef or whatever)
 
-TODO: sanitize parameters going into pf_run with strict regex
+TODO: sanitize parameters going into safe_pf_run with strict regex
 
 =cut
 
@@ -449,12 +447,8 @@ sub schema {
     my $logger = get_logger();
 
     my ( $status_msg, $result );
-    $root_user = quotemeta ($root_user);
-    $root_password = quotemeta ($root_password);
-    $db = quotemeta ($db);
-    my $mysql_cmd = "/usr/bin/mysql --socket=/var/lib/mysql/mysql.sock -u $root_user -p$root_password $db";
-    my $cmd = "$mysql_cmd < $install_dir/db/pf-schema.sql";
-    eval { $result = pf_run($cmd, (accepted_exit_status => [ 0 ]), log_strip => quotemeta("-p$root_password")) };
+    my @mysql_cmd = ('/usr/bin/mysql', '--socket=/var/lib/mysql/mysql.sock', '-u', $root_user, "-p$root_password", $db);
+    eval { $result = safe_pf_run(@mysql_cmd, {stdin => "$install_dir/db/pf-schema.sql", accepted_exit_status => [ 0 ], log_strip => $root_password}) };
     if ( $@ || !defined($result) ) {
         $status_msg = ["Error applying the schema to the database [_1]",$db ];
         $logger->warn("$@: $result");
@@ -463,8 +457,7 @@ sub schema {
     my @custom_schemas = read_dir( "$install_dir/db/custom", prefix => 1, err_mode => 'quiet' ) ;
     @custom_schemas = sort @custom_schemas;
     foreach my $custom_schema (@custom_schemas) {
-        my $cmd = "$mysql_cmd < $custom_schema";
-        eval { $result = pf_run($cmd, (accepted_exit_status => [ 0 ], log_strip => quotemeta("-p$root_password"))) };
+        eval { $result = safe_pf_run(@mysql_cmd, {stdin => $custom_schema, accepted_exit_status => [ 0 ], log_strip => $root_password}) };
         if ( $@ || !defined($result) ) {
             $status_msg = ["Error applying the custom schema $custom_schema to the database [_1]",$db ];
             $logger->warn("$@: $result");

diff --git a/html/pfappserver/lib/pfappserver/Model/Interface.pm b/html/pfappserver/lib/pfappserver/Model/Interface.pm
@@ -63,8 +63,7 @@ sub create {
     }
 
     # Create requested virtual interface
-    my $cmd = "sudo ip link add link $physical_interface name $physical_interface.$vlan_id type vlan id $vlan_id";
-    eval { $status = pf_run($cmd) };
+    eval { $status = safe_pf_run("sudo", "ip", "link", "add", "link", $physical_interface, "name", "$physical_interface.$vlan_id", "type", "vlan", "id", $vlan_id) };
     if ( $@ ) {
         $status_msg = ["Error in creating interface VLAN [_1]",$interface];
         $logger->error($status_msg);
@@ -117,8 +116,7 @@ sub delete {
     }
 
     # Delete requested virtual interface
-    my $cmd = "sudo ip link delete $interface";
-    eval { $status = pf_run($cmd) };
+    eval { $status = safe_pf_run(qw(sudo ip link delete), "$interface") };
     if ( $@ ) {
         $status_msg = ["Error in deletion of interface VLAN [_1]",$interface];
         $logger->error("Error in deletion of interface VLAN $interface");
@@ -179,8 +177,7 @@ sub down {
     }
 
     # Disable interface using "ip"
-    my $cmd = sprintf "sudo ip link set %s down", $interface;
-    eval { $status = pf_run($cmd) };
+    eval { $status = safe_pf_run(qw(sudo ip link set), $interface, 'down') };
     if ( $@ ) {
         $status_msg = ["Can't disable interface [_1] : [_2]",$interface , $status];
         $logger->error("Can't disable interface $interface : $status");
@@ -335,14 +332,12 @@ sub update {
         my $isDefaultRoute = (defined($interface_before->{ipaddress}) && $gateway eq $interface_before->{ipaddress});
 
         # Delete previous IP address
-        my $cmd;
         if (defined($interface_before->{address}) && $interface_before->{address} ne '') {
-            $cmd = sprintf "sudo ip addr del %s dev %s", $interface_before->{address}, $interface_before->{name};
-            eval { $status = pf_run($cmd) };
+            eval { $status = safe_pf_run(qw(sudo ip addr del), $interface_before->{address},'dev',$interface_before->{name}) };
             if ( $@ || $status ) {
                 $status_msg = ["Can't delete previous IP address of interface [_1] ([_2])",$interface,$interface_before->{address}];
                 $logger->error("Can't delete previous IP address of interface $interface");
-                $logger->error("$cmd: $status");
+                $logger->error("$status");
                 return ($STATUS::INTERNAL_SERVER_ERROR, $status_msg);
             }
         }
@@ -355,12 +350,10 @@ sub update {
 
             $logger->debug("IP address has changed ($interface $ipaddress/$netmask)");
 
-            $cmd = sprintf "sudo ip addr add %s/%i broadcast %s dev %s", $ipaddress, $netmask, $broadcast, $interface;
-            eval { $status = pf_run($cmd) };
+            eval { $status = safe_pf_run(qw(sudo ip addr add), "${ipaddress}/${netmask}", 'broadcast', $broadcast, 'dev', $interface) };
             if ( $@ || $status ) {
                 $status_msg = ["Can't add new IP address on interface [_1] ([_2])",$interface,$ipaddress];
                 $logger->error($status);
-                $logger->error("$cmd: $status");
                 return ($STATUS::INTERNAL_SERVER_ERROR, $status_msg);
             }
             elsif ($isDefaultRoute) {
@@ -375,14 +368,12 @@ sub update {
         $network_configuration_changed = $TRUE;
 
         # Delete previous IP address
-        my $cmd;
         if ( defined($interface_before->{ipv6_network}) && $interface_before->{ipv6_network} ne '' ) {
-            $cmd = sprintf "sudo ip -6 addr del %s dev %s", $interface_before->{ipv6_network}, $interface_before->{name};
-            eval { $status = pf_run($cmd) };
+            eval { $status = safe_pf_run(qw(sudo ip -6 addr del), $interface_before->{ipv6_network}, 'dev', $interface_before->{name}) };
             if ( $@ || $status ) {
                 $status_msg = ["Can't delete previous IPv6 address of interface [_1] ([_2])", $interface, $interface_before->{ipv6_network}];
                 $logger->error("Can't delete previous IPv6 address of interface $interface");
-                $logger->error("$cmd: $status");
+                $logger->error("$status");
                 return ($STATUS::INTERNAL_SERVER_ERROR, $status_msg);
             }
         }
@@ -395,12 +386,10 @@ sub update {
 
             $logger->debug("IPv6 address has changed ($interface $ipv6_address/$ipv6_prefix)");
 
-            $cmd = sprintf "sudo ip -6 addr add %s/%i dev %s", $ipv6_address, $ipv6_prefix, $interface;
-            eval { $status = pf_run($cmd) };
+            eval { $status = safe_pf_run(qw(sudo ip -6 addr add), "${ipv6_address}/$ipv6_prefix", 'dev', $interface) };
             if ( $@ || $status ) {
                 $status_msg = ["Can't add new IPv6 address on interface [_1] ([_2])", $interface, $ipv6_address];
                 $logger->error($status);
-                $logger->error("$cmd: $status");
                 return ($STATUS::INTERNAL_SERVER_ERROR, $status_msg);
             }
         }
@@ -648,14 +637,8 @@ sub _listInterfaces {
     my @interfaces_list = ();
 
     $ifname = '' if ($ifname eq 'all');
-    my $cmd =
-      {
-        link        => "sudo ip -4 -o link show $ifname",
-        addr        => "sudo ip -4 -o addr show %s",
-        ipv6_addr   => "sudo ip -6 -o addr show %s",
-      };
     my ($link, $addr, $ipv6_addr);
-    eval { $link = pf_run($cmd->{link}) };
+    eval { $link = safe_pf_run(qw(sudo ip -4 -o link show),  $ifname) };
     if ($link) {
         # Parse output of ip command
         while ($link =~ m/^
@@ -675,7 +658,7 @@ sub _listInterfaces {
                is_running => ($state ne 'DOWN'),
                hwaddr => $hwaddr
               };
-            eval { $addr = pf_run(sprintf $cmd->{addr}, $name) };
+            eval { $addr = safe_pf_run(qw(sudo ip -4 -o addr show) , $name) };
             if ($addr) {
                 if ($addr =~ m/\binet (([^\/]+)\/\d+)/) {
                     $interface->{address} = $1,
@@ -684,7 +667,7 @@ sub _listInterfaces {
                     $interface->{netmask} = $netmask;
                 }
             }
-            eval { $ipv6_addr = pf_run(sprintf $cmd->{ipv6_addr}, $name) };
+            eval { $ipv6_addr = safe_pf_run(qw(sudo ip -6 -o addr show), $name) };
             if ( $ipv6_addr ) {
                 if ($ipv6_addr =~ m/\binet6 (([^\/]+)\/(\d+)) scope global/) {
                     $interface->{ipv6_network}  = $1,
@@ -800,8 +783,7 @@ sub up {
         return ($STATUS::PRECONDITION_FAILED, $status_msg);
     }
 
-    my $cmd = sprintf "sudo ip link set %s up", $interface;
-    eval { $status = pf_run($cmd) };
+    eval { $status = safe_pf_run(qw(sudo ip link set), $interface, 'up') };
     if ( $@ ) {
         $status_msg = ["Can't enable interface [_1]",$interface];
         $logger->error("Can't enable interface $interface");

diff --git a/lib/pf/Switch.pm b/lib/pf/Switch.pm
@@ -3257,7 +3257,7 @@ sub deauth_source_ip {
     my $logger = $self->logger();
     my $chi = pf::CHI->new(namespace => 'route_int');
     my $int = $chi->compute($dst_ip, sub {
-                                         my @interface_src = split(" ", pf_run("sudo ip route get $dst_ip"));
+                                         my @interface_src = split(" ", safe_pf_run(qw(sudo ip route get), $dst_ip));
                                          if ($interface_src[1] eq 'via') {
                                              return $interface_src[4];
                                          } else {

diff --git a/lib/pf/Switch/Cisco/Cisco_IOS.pm b/lib/pf/Switch/Cisco/Cisco_IOS.pm
@@ -699,13 +699,16 @@ sub setPortSecurityMaxSecureMacAddrVlanAccessByIfIndex {
     # we spawn a shell to workaround a thread safety bug in Net::Appliance::Session when using SSH transport
     # http://www.cpanforum.com/threads/6909
 
-    my $command =
-        "/usr/local/pf/bin/pfcmd_vlan -switch $self->{_ip} "
-        . "-runSwitchMethod _setPortSecurityMaxSecureMacAddrVlanAccessByIfIndex $ifIndex $maxSecureMac"
-    ;
-
     $logger->info("spawning a pfcmd_vlan process to set 'switchport port-security maximum $maxSecureMac vlan access'");
-    pf_run($command);
+    safe_pf_run(
+        "/usr/local/pf/bin/pfcmd_vlan",
+        '-switch', $self->{_ip},
+        "-runSwitchMethod",
+        '_setPortSecurityMaxSecureMacAddrVlanAccessByIfIndex',
+        $ifIndex,
+        $maxSecureMac
+    );
+
     return $TRUE;
 }
 

diff --git a/lib/pf/Switch/Cisco/Cisco_IOS_12_x.pm b/lib/pf/Switch/Cisco/Cisco_IOS_12_x.pm
@@ -706,13 +706,15 @@ sub setPortSecurityMaxSecureMacAddrVlanAccessByIfIndex {
     # we spawn a shell to workaround a thread safety bug in Net::Appliance::Session when using SSH transport
     # http://www.cpanforum.com/threads/6909
 
-    my $command =
-        "/usr/local/pf/bin/pfcmd_vlan -switch $self->{_ip} "
-        . "-runSwitchMethod _setPortSecurityMaxSecureMacAddrVlanAccessByIfIndex $ifIndex $maxSecureMac"
-    ;
-
     $logger->info("spawning a pfcmd_vlan process to set 'switchport port-security maximum $maxSecureMac vlan access'");
-    pf_run($command);
+    safe_pf_run(
+        "/usr/local/pf/bin/pfcmd_vlan",
+        '-switch', $self->{_ip},
+        '-runSwitchMethod',
+        '_setPortSecurityMaxSecureMacAddrVlanAccessByIfIndex',
+        $ifIndex ,
+        $maxSecureMac
+    );
     return $TRUE;
 }
 

diff --git a/lib/pf/Switch/Juniper.pm b/lib/pf/Switch/Juniper.pm
@@ -235,9 +235,9 @@ sub handleReAssignVlanTrapForWiredMacAuth {
     $logger->info("Bouncing $switch_ip:$ifIndex. A new VLAN will be assigned upon reconnection.");
     # we spawn a shell to workaround a thread safety bug in Net::Appliance::Session when using SSH transport
     # http://www.cpanforum.com/threads/6909
-    pf_run("/usr/local/pf/bin/pfcmd_vlan -setIfAdminStatus -switch $switch_ip -ifIndex $ifIndex -ifAdminStatus 0");
+    safe_pf_run(qw(/usr/local/pf/bin/pfcmd_vlan -setIfAdminStatus -switch), $switch_ip, '-ifIndex', $ifIndex, '-ifAdminStatus', 0);
     sleep(2);
-    pf_run("/usr/local/pf/bin/pfcmd_vlan -setIfAdminStatus -switch $switch_ip -ifIndex $ifIndex -ifAdminStatus 1");
+    safe_pf_run(qw(/usr/local/pf/bin/pfcmd_vlan -setIfAdminStatus -switch), $switch_ip, '-ifIndex', $ifIndex, '-ifAdminStatus', 1);
 
 }
 

diff --git a/lib/pf/UnifiedApi/Controller/Config/System.pm b/lib/pf/UnifiedApi/Controller/Config/System.pm
@@ -98,7 +98,7 @@ sub put_dns_servers {
 
 sub _get_hostname {
     my ($self) = @_;
-    my $hostname = pf_run("hostnamectl --static");
+    my $hostname = safe_pf_run(qw(hostnamectl --static));
     chomp($hostname);
     return $hostname if defined($hostname);
 }
@@ -112,7 +112,7 @@ sub put_hostname {
     my ($self) = @_;
     my $hostname = $self->get_json ? $self->get_json->{hostname} : undef;
     if($hostname) {
-        pf_run("sudo hostnamectl set-hostname $hostname");
+        safe_pf_run(qw(sudo hostnamectl set-hostname), $hostname);
         my $new_hostname = $self->_get_hostname();
         chomp($new_hostname);
         if($new_hostname eq $hostname) {

diff --git a/lib/pf/UnifiedApi/Controller/SystemSummary.pm b/lib/pf/UnifiedApi/Controller/SystemSummary.pm
@@ -17,7 +17,7 @@ use pf::db;
 use pf::config::util qw(is_inline_configured);
 use pf::version;
 use pf::cluster;
-use pf::util qw(pf_run isenabled);
+use pf::util qw(safe_pf_run isenabled);
 use pf::file_paths qw($git_commit_id_file $install_dir);
 use Fcntl qw(SEEK_SET);
 use pf::UnifiedApi::Controller::Config::System;
@@ -81,7 +81,7 @@ sub git_commit_id {
             chomp($id);
         }
     } elsif (-d "$install_dir/.git") {
-        $id = pf_run("git -C $install_dir log -n1 --format=%H");
+        $id = safe_pf_run('git', '-C', "$install_dir", qw(log -n1 --format=%H));
         chomp($id);
     }
 

diff --git a/lib/pf/action.pm b/lib/pf/action.pm
@@ -175,6 +175,7 @@ sub action_api {
     my $class_info = class_view($security_event_id);
     my @params = split(' ', $class_info->{'external_command'});
     my $return;
+    my @cmd;
     my $node_info = node_view($mac);
     my $ip = pf::ip4log::mac2ip($mac) || 0;
     $node_info = {%$node_info, 'last_ip' => $ip};
@@ -183,12 +184,13 @@ sub action_api {
         $param =~ s/\$security_event_id/$security_event_id/ge;
         $param =~ s/\$(.*)/$node_info->{$1}/ge;
         $return .= $param." ";
+        push @cmd, $param;
     }
     $logger->warn($return);
 
     my $cmd = "$return 2>&1";
 
-    my @lines  = pf_run($cmd);
+    my @lines  = safe_pf_run(@cmd);
     return;
 }
 

diff --git a/lib/pf/domain.pm b/lib/pf/domain.pm
@@ -80,7 +80,7 @@ sub add_computer {
     my $result;
     eval {
         my $command = "$ADD_COMPUTERS_BIN -computer-name '$computer_name' -computer-pass '$computer_password' -dc-ip $domain_controller_ip -dc-host '$domain_controller_host' -baseDN '$baseDN' -computer-group '$computer_group' '$domain_auth' $option -method=$method";
-        $result = pf_run($command, accepted_exit_status => [ 0 ]);
+        $result = safe_pf_run($ADD_COMPUTERS_BIN, '-computer-name', $computer_name, '-computer-pass', $computer_password, '-dc-ip', $domain_controller_ip, '-dc-host', $domain_controller_host, '-baseDN', $baseDN, '-computer-group', $computer_group, $domain_auth, $option, "-method=$method", {accepted_exit_status => [ 0 ]});
     };
     if ($@) {
         $result = "Executing add computers failed with unknown errors";