From 94b9761011f276ca379a2c524800a7de0a4b45c3 Mon Sep 17 00:00:00 2001 From: "Kyale, Eliud" Date: Wed, 21 Feb 2024 14:47:20 -0500 Subject: [PATCH] Replace bmc system() commands with fork() execv() Mtce uses the system() command to run the ipmitool and redfishtool. The system() command launches a shell process that is susceptible to code injection. By switching to fork() execv() we can prevent command injection attacks if for example the bmc parameters are compromised. The bmc parameters are: - bm_type - bm_ip - bm_username - bm_password These are initially provided as user input and stored in either barbican (bm_password) or the sysinv postgres database. If these parameters are compromised, the injected code will not be run. For example, if bm_username="root; reboot&" the reboot command will not be run. Test plan: PASS - Code testing: designer testing of failure paths, verifying logs by compiling errors in the code - fork fail error path - file open failure path - dup/dup2 failure path - execv failure PASS - AIO-SX: iso install PASS - AIO-DX: iso install PASS - AIO-SX: ipmi bmc sensor/device queries system host-sensor-list PASS - AIO-SX: ipmi bmc reset designer modification of sysinv to allow simplex reset PASS - AIO-SX: modify bmc parameters in postgres and verify bmc command failure and proper handling e.g bm_username="root; reboot&" PASS - AIO-SX: file leak testing of execv error path sudo lsof -p `pidof mtcAgent` sudo lsof -p `pidof hwmond` PASS - AIO-SX: memory leak and file leak testingsoak sudo /usr/sbin/dmemchk.sh --C mtcAgent hwmond PASS - AIO-DX: ipmi bmc reset Virtual machine AIO-DX configured to physical bmc simulate reset on virtual machine by power down at the same time as system host-reset PASS - AIO-DX: ipmi bmc sensor/device queries system host-sensor-list Example postgres commands to compromise the bm_username parameter: sudo -u postgres \ psql -d sysinv \ -c "select bm_username from i_host where hostname='controller-0';" sudo -u postgres \ psql -d sysinv \ -c \ "update i_host set bm_username='root; reboot&' "\ "where hostname='controller-0';" Story: 2011095 Task: 50344 Change-Id: I250900d1c757d7e04058f4c954502b1a38db235e Signed-off-by: Kyale, Eliud --- mtce-common/src/common/ipmiUtil.cpp | 15 +-- mtce-common/src/common/ipmiUtil.h | 4 +- mtce-common/src/common/nodeUtil.cpp | 161 ++++++++++++++++++++++++- mtce-common/src/common/nodeUtil.h | 15 +++ mtce-common/src/common/redfishUtil.cpp | 13 +- mtce-common/src/common/redfishUtil.h | 5 +- mtce-common/src/common/threadUtil.cpp | 6 +- mtce-common/src/common/threadUtil.h | 3 +- mtce/src/hwmon/hwmonThreads.cpp | 20 +-- mtce/src/maintenance/mtcThreads.cpp | 10 +- 10 files changed, 208 insertions(+), 44 deletions(-) diff --git a/mtce-common/src/common/ipmiUtil.cpp b/mtce-common/src/common/ipmiUtil.cpp index 0679df2e..2c0d1d9b 100644 --- a/mtce-common/src/common/ipmiUtil.cpp +++ b/mtce-common/src/common/ipmiUtil.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017 Wind River Systems, Inc. + * Copyright (c) 2017, 2024 Wind River Systems, Inc. * * SPDX-License-Identifier: Apache-2.0 * @@ -16,6 +16,8 @@ using namespace std; #include "ipmiUtil.h" /* for ... module header */ #include "hostUtil.h" /* for ... mtce host common definitions */ +#include "nodeUtil.h" /* for ... fork_execv */ + /*********************************************************************** * @@ -35,7 +37,7 @@ int ipmiUtil_init ( void ) } /* Create the ipmi request */ -string ipmiUtil_create_request ( string cmd, string & ip, string & un, string & pw, string & out ) +string ipmiUtil_create_request ( string cmd, string & ip, string & un, string & pw) { /* ipmitool -I lanplus -H $uut_ip -U $uut_un -E */ /* build the ipmitool command */ @@ -67,10 +69,6 @@ string ipmiUtil_create_request ( string cmd, string & ip, string & un, string & ipmitool_request.append(" "); ipmitool_request.append(cmd); - /* output filename */ - ipmitool_request.append (" > "); - ipmitool_request.append (out); - return (ipmitool_request); } @@ -233,8 +231,7 @@ int ipmiUtil_reset_host_now ( string hostname, ipmiUtil_create_request ( IPMITOOL_POWER_RESET_CMD, accessInfo.bm_ip, accessInfo.bm_un, - info.password_file, - output_filename ); + info.password_file); /* issue request * @@ -244,7 +241,7 @@ int ipmiUtil_reset_host_now ( string hostname, * UT showed that there is no stall at all. */ unsigned long long latency_threshold_secs = DEFAULT_SYSTEM_REQUEST_LATENCY_SECS ; unsigned long long before_time = gettime_monotonic_nsec () ; - int rc = system ( request.data()) ; + int rc = fork_execv ( hostname, request, output_filename ) ; unsigned long long after_time = gettime_monotonic_nsec () ; unsigned long long delta_time = after_time-before_time ; if ( rc ) diff --git a/mtce-common/src/common/ipmiUtil.h b/mtce-common/src/common/ipmiUtil.h index 39e8b9fd..95f48f82 100644 --- a/mtce-common/src/common/ipmiUtil.h +++ b/mtce-common/src/common/ipmiUtil.h @@ -2,7 +2,7 @@ #define __INCLUDE_IPMIUTIL_H__ /* - * Copyright (c) 2017 Wind River Systems, Inc. + * Copyright (c) 2017, 2024 Wind River Systems, Inc. * * SPDX-License-Identifier: Apache-2.0 * @@ -60,6 +60,6 @@ int ipmiUtil_bmc_info_load ( string hostname, const char * filename, bmc_info_t int ipmiUtil_reset_host_now ( string hostname, bmcUtil_accessInfo_type accessInfo, string output_filename ); /* Create the ipmi request */ -string ipmiUtil_create_request ( string cmd, string & ip, string & un, string & pw, string & out ); +string ipmiUtil_create_request ( string cmd, string & ip, string & un, string & pw); #endif diff --git a/mtce-common/src/common/nodeUtil.cpp b/mtce-common/src/common/nodeUtil.cpp index 86681213..9684f0c8 100755 --- a/mtce-common/src/common/nodeUtil.cpp +++ b/mtce-common/src/common/nodeUtil.cpp @@ -33,7 +33,7 @@ #include #include #include - +#include using namespace std; @@ -2460,3 +2460,162 @@ system_state_enum get_system_state ( bool verbose ) } return system_state ; } + +/** + * @brief Restore the stdout and stderr from copies + * + * @param stdout_copy copy of stdout file description + * @param stderr_copy copy of stderr file description + */ +static void restore_stdout_stderr(int& stdout_copy, int& stderr_copy) +{ + if(stdout_copy > 0) + { + dup2(stdout_copy, STDOUT_FILENO); + close(stdout_copy); + } + if(stderr_copy > 0) + { + dup2(stderr_copy, STDERR_FILENO); + close(stderr_copy); + } +} + +/** + * @brief Redirect the stdout and stderr to the + * output filename. store a copy of current + * stdout & stderr file descriptions + * + * @param hostname The hostname + * @param output_filename out filename for stdout/stderr redirection + * @param stdout_copy copy of stdout file description + * @param stderr_copy copy of stderr file description + */ +static void redirect_stdout_stderr(const string& hostname, + const string& output_filename, + int& stdout_copy, + int& stderr_copy) +{ + // default output values + stdout_copy = -1; + stderr_copy = -1; + + int redirect_fd = open(output_filename.c_str(), + O_CREAT | O_WRONLY | O_TRUNC); + if ( redirect_fd < 0 ) + { + elog ("%s failed to open output filename: [%s] - error code = %d (%s)", + hostname.c_str(), + output_filename.c_str(), + errno, + strerror(errno)); + return; + } + + stdout_copy = dup(STDOUT_FILENO); + if( stdout_copy < 0 ) + { + elog ("%s could not copy STDOUT file descriptor - error code = %d (%s)", + hostname.c_str(), + errno, + strerror(errno)); + return; + } + + stderr_copy = dup(STDERR_FILENO); + if( stderr_copy < 0 ) + { + elog ("%s could not copy STDERR file descriptor - error code = %d (%s)", + hostname.c_str(), + errno, + strerror(errno)); + return; + } + + if(dup2(redirect_fd, STDOUT_FILENO) < 0 ) + { + elog ("%s could not redirect STDOUT file descriptor - error code = %d (%s)", + hostname.c_str(), + errno, + strerror(errno)); + return; + } + + if (dup2(redirect_fd, STDERR_FILENO) < 0 ) + { + elog ("%s could not redirect STDERR file descriptor - error code = %d (%s)", + hostname.c_str(), + errno, + strerror(errno)); + return; + } + + close(redirect_fd); +} + +int fork_execv (const string& hostname, + const string& cmd, + const string& output_filename) +{ + int rc = FAIL_SYSTEM_CALL; + + pid_t child_pid = fork(); // fork child process in order to run cmd + switch(child_pid) + { + case -1: + { + // fork failed + rc = FAIL_FORK_HANDLING; + elog ("%s could not execute command - fork failed - error code = %d (%s)", + hostname.c_str(), + errno, + strerror(errno)); + break; + } + case 0: + { + // child process + std::vector argv; + std::regex ws_re("[^\\s]+"); // whitespace + std::sregex_token_iterator first{cmd.begin(), + cmd.end(), + ws_re, + 0}; + std::sregex_token_iterator last; + std::vector v(first,last); + for (const std::string& arg : v) + { + argv.push_back(const_cast(arg.c_str())); + } + argv.push_back(NULL); // end of argument list + { + int stdout_copy = -1, stderr_copy = -1; + redirect_stdout_stderr(hostname, + output_filename, + stdout_copy, + stderr_copy); + execv(argv[0], &argv[0]); + restore_stdout_stderr(stdout_copy, stderr_copy); + } + /* execv returns -1 on error, and does not return on success */ + elog ("%s could not execute command '%s' - error code = %d (%s)", + hostname.c_str(), + argv[0], + errno, + strerror(errno)); + exit(FAIL_SYSTEM_CALL); + } + default: + { + // parent process + int status = 0; + waitpid(child_pid , &status, 0 ); + if( WIFEXITED(status) && WEXITSTATUS(status) == PASS ) + { + rc = PASS; + } + break; + } + } + return (rc); +} diff --git a/mtce-common/src/common/nodeUtil.h b/mtce-common/src/common/nodeUtil.h index 938319e1..601da781 100755 --- a/mtce-common/src/common/nodeUtil.h +++ b/mtce-common/src/common/nodeUtil.h @@ -193,4 +193,19 @@ typedef enum system_state_enum get_system_state ( bool verbose=true ); const char * get_system_state_str ( system_state_enum system_state ); +/** + * @brief execute command string using fork execv + * + * @param hostname The hostname + * @param cmd command string + * @param output_filename output redirection file + * stdout and stderr will be redirected to + * this file + * + * @return int return code, 0 (PASS) on success + */ +int fork_execv (const string& hostname, + const string& cmd, + const string& output_filename); + #endif diff --git a/mtce-common/src/common/redfishUtil.cpp b/mtce-common/src/common/redfishUtil.cpp index a8d09d02..cd845747 100644 --- a/mtce-common/src/common/redfishUtil.cpp +++ b/mtce-common/src/common/redfishUtil.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019 Wind River Systems, Inc. + * Copyright (c) 2019, 2024 Wind River Systems, Inc. * * SPDX-License-Identifier: Apache-2.0 * @@ -332,7 +332,6 @@ bool redfishUtil_is_supported (string & hostname, string & response) * -r ip - the ip address to send the request to * -c config_file - the bmc cfgFile (password) filename * cmd - the redfish command to execute - * > out - the filename to where the output is directed * * Returns : full command request as a single string * @@ -340,8 +339,7 @@ bool redfishUtil_is_supported (string & hostname, string & response) string redfishUtil_create_request ( string cmd, string & ip, - string & config_file, - string & out ) + string & config_file) { /* build the command ; starting with the redfishtool binary */ string command_request = REDFISHTOOL_PATH_AND_FILENAME ; @@ -399,13 +397,6 @@ string redfishUtil_create_request ( string cmd, command_request.append(" "); command_request.append(cmd); - /* output filename */ - command_request.append (" > "); - command_request.append (out); - - /* direct stderr to stdio */ - command_request.append (" 2>&1"); - return (command_request); } diff --git a/mtce-common/src/common/redfishUtil.h b/mtce-common/src/common/redfishUtil.h index c9b6d283..9339c730 100644 --- a/mtce-common/src/common/redfishUtil.h +++ b/mtce-common/src/common/redfishUtil.h @@ -2,7 +2,7 @@ #define __INCLUDE_REDFISHUTIL_H__ /* - * Copyright (c) 2017 Wind River Systems, Inc. + * Copyright (c) 2017, 2024 Wind River Systems, Inc. * * SPDX-License-Identifier: Apache-2.0 * @@ -211,8 +211,7 @@ int redfishUtil_init ( void ); /* create a redfish tool thread request */ string redfishUtil_create_request ( string cmd, string & ip, - string & config_file, - string & out ); + string & config_file); /* interpret redfish root query response and check version */ bool redfishUtil_is_supported ( string & hostname, string & root_query_response ); diff --git a/mtce-common/src/common/threadUtil.cpp b/mtce-common/src/common/threadUtil.cpp index 46e650bb..d8f513dd 100644 --- a/mtce-common/src/common/threadUtil.cpp +++ b/mtce-common/src/common/threadUtil.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016-2017 Wind River Systems, Inc. + * Copyright (c) 2016-2017, 2024 Wind River Systems, Inc. * * SPDX-License-Identifier: Apache-2.0 * @@ -29,6 +29,7 @@ #include "nodeBase.h" /* for ... mtce node common definitions */ #include "hostUtil.h" /* for ... mtce host common definitions */ #include "threadUtil.h" /* for ... this module header */ +#include "nodeUtil.h" /* for ... fork_execv */ /* Stores the parent process's timer handler */ static void (*thread_timer_handler)(int, siginfo_t*, void*) = NULL ; @@ -141,10 +142,11 @@ void threadUtil_fini ( void ) int threadUtil_bmcSystemCall (string hostname, string request, + string datafile, unsigned long long latency_threshold_secs) { unsigned long long before_time = gettime_monotonic_nsec () ; - int rc = system ( request.data()) ; + int rc = fork_execv ( hostname, request , datafile ) ; unsigned long long after_time = gettime_monotonic_nsec () ; unsigned long long delta_time = after_time-before_time ; if ( delta_time > (latency_threshold_secs*1000000000)) diff --git a/mtce-common/src/common/threadUtil.h b/mtce-common/src/common/threadUtil.h index b2558fd3..20682b70 100644 --- a/mtce-common/src/common/threadUtil.h +++ b/mtce-common/src/common/threadUtil.h @@ -2,7 +2,7 @@ #define __INCLUDE_THREADBASE_H__ /* - * Copyright (c) 2017 Wind River Systems, Inc. + * Copyright (c) 2017, 2024 Wind River Systems, Inc. * * SPDX-License-Identifier: Apache-2.0 * @@ -262,6 +262,7 @@ int threadUtil_init ( void (*handler)(int, siginfo_t*, void* )); #define DEFAULT_SYSTEM_REQUEST_LATENCY_SECS (unsigned long long)(15) int threadUtil_bmcSystemCall (string hostname, string request, + string datafile, unsigned long long latency_threshold_secs); void threadUtil_setstack_size ( void ); diff --git a/mtce/src/hwmon/hwmonThreads.cpp b/mtce/src/hwmon/hwmonThreads.cpp index a572ce4c..d4c74e64 100644 --- a/mtce/src/hwmon/hwmonThreads.cpp +++ b/mtce/src/hwmon/hwmonThreads.cpp @@ -1,6 +1,6 @@ /* - * Copyright (c) 2016-2017 Wind River Systems, Inc. + * Copyright (c) 2016-2017, 2024 Wind River Systems, Inc. * * SPDX-License-Identifier: Apache-2.0 * @@ -36,6 +36,7 @@ using namespace std; #include "hwmonThreads.h" /* for ... BMC_THREAD_CMD__READ_SENSORS */ #include "hwmonBmc.h" /* for ... MAX_IPMITOOL_PARSE_ERRORS */ #include "hwmonClass.h" /* for ... thread_extra_info_type */ +#include "nodeUtil.h" /* for ... fork_execv */ /*************************************************************************** * @@ -376,8 +377,7 @@ void * hwmonThread_ipmitool ( void * arg ) ipmiUtil_create_request ( command, extra_ptr->bm_ip, extra_ptr->bm_un, - info_ptr->password_file, - datafile ); + info_ptr->password_file); dlog_t ("%s power status query cmd: %s\n", info_ptr->log_prefix, request.c_str()); @@ -390,7 +390,7 @@ void * hwmonThread_ipmitool ( void * arg ) else { /* Make the request */ - rc = system ( request.data()) ; + rc = fork_execv ( info_ptr->hostname, request, datafile ) ; } unlink(info_ptr->password_file.data()); @@ -484,8 +484,7 @@ void * hwmonThread_ipmitool ( void * arg ) ipmiUtil_create_request ( IPMITOOL_SENSOR_QUERY_CMD, extra_ptr->bm_ip, extra_ptr->bm_un, - info_ptr->password_file, - sensor_datafile ); + info_ptr->password_file); dlog_t ("%s sensor query cmd:%s\n", info_ptr->log_prefix, @@ -519,7 +518,7 @@ void * hwmonThread_ipmitool ( void * arg ) { /* remove the last query */ // daemon_remove_file ( sensor_datafile.data() ) ; - rc = system ( sensor_query_request.data()) ; + rc = fork_execv ( info_ptr->hostname, sensor_query_request, sensor_datafile ) ; } #ifdef WANT_FIT_TESTING @@ -1059,8 +1058,7 @@ static int _redfishUtil_send_request( thread_info_type * info_ptr, string & data /************** Create the redfishtool request **************/ request = redfishUtil_create_request ( redfish_cmd_str, extra_ptr->bm_ip, - info_ptr->password_file, - datafile ); + info_ptr->password_file); dlog_t ("%s query cmd: %s\n", info_ptr->log_prefix, @@ -1074,7 +1072,9 @@ static int _redfishUtil_send_request( thread_info_type * info_ptr, string & data else { daemon_remove_file ( datafile.data() ) ; - rc = threadUtil_bmcSystemCall (info_ptr->hostname, request, + rc = threadUtil_bmcSystemCall (info_ptr->hostname, + request, + datafile, DEFAULT_SYSTEM_REQUEST_LATENCY_SECS) ; if ( rc != PASS ) { diff --git a/mtce/src/maintenance/mtcThreads.cpp b/mtce/src/maintenance/mtcThreads.cpp index a93a7f88..f86b2d80 100644 --- a/mtce/src/maintenance/mtcThreads.cpp +++ b/mtce/src/maintenance/mtcThreads.cpp @@ -1,6 +1,6 @@ /* - * Copyright (c) 2016-2023 Wind River Systems, Inc. + * Copyright (c) 2016-2023, 2024 Wind River Systems, Inc. * * SPDX-License-Identifier: Apache-2.0 * @@ -287,8 +287,7 @@ void * mtcThread_bmc ( void * arg ) string request = redfishUtil_create_request (command, extra_ptr->bm_ip, - info_ptr->password_file, - datafile); + info_ptr->password_file); blog1_t ("%s %s", info_ptr->hostname.c_str(), request.c_str()); @@ -322,6 +321,7 @@ void * mtcThread_bmc ( void * arg ) rc = threadUtil_bmcSystemCall (info_ptr->hostname, request, + datafile, DEFAULT_SYSTEM_REQUEST_LATENCY_SECS); if ( rc != PASS ) @@ -437,8 +437,7 @@ void * mtcThread_bmc ( void * arg ) string request = ipmiUtil_create_request ( command, extra_ptr->bm_ip, extra_ptr->bm_un, - info_ptr->password_file, - datafile ); + info_ptr->password_file); dlog_t ("%s %s", info_ptr->hostname.c_str(), request.c_str()); @@ -505,6 +504,7 @@ void * mtcThread_bmc ( void * arg ) rc = threadUtil_bmcSystemCall (info_ptr->hostname, request, + datafile, DEFAULT_SYSTEM_REQUEST_LATENCY_SECS); if ( rc != PASS )