starlingx/metal
Code Issues Proposed changes

Replace bmc system() commands with fork() execv()

Browse Source 
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 <controller-0>
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 <controller>
PASS - AIO-DX: ipmi bmc sensor/device queries
               system host-sensor-list <controller-0|1>

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 <Eliud.Kyale@windriver.com>
This commit is contained in:
Kyale, Eliud 2024-02-21 14:47:20 -05:00
parent dbb9543c08
commit 94b9761011
10 changed files with 208 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
mtce-common/src/common/ipmiUtil.cpp
View File

@ -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 )

4
mtce-common/src/common/ipmiUtil.h
View File

@ -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

161
mtce-common/src/common/nodeUtil.cpp
View File

@ -33,7 +33,7 @@
#include <fstream>
#include <stdlib.h>
#include <stdio.h>
#include <regex>
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<char*> 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<std::string> v(first,last);
for (const std::string& arg : v)
{
argv.push_back(const_cast<char*>(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);
}

15
mtce-common/src/common/nodeUtil.h
View File

@ -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

13
mtce-common/src/common/redfishUtil.cpp
View File

@ -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);
}

5
mtce-common/src/common/redfishUtil.h
View File

@ -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 );

6
mtce-common/src/common/threadUtil.cpp
View File

@ -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))

3
mtce-common/src/common/threadUtil.h
View File

@ -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 );

20
mtce/src/hwmon/hwmonThreads.cpp
View File

@ -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 )
{

10
mtce/src/maintenance/mtcThreads.cpp
View File

@ -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 )