futility: Replace host_shell with host_exec_output using subprocess Replace all usages of host_shell() with a new helper function host_exec_output() that invokes subprocess_run() with an argument array to avoid system shell evaluation and potential command injection. In archive fallback, use subprocess_run() directly for mkdir -p. BUG=b:293560339, b:537409763 TEST=make runtests BRANCH=none TAG=agy CONV=0ed59c5f-379d-47ac-8eda-adf83afbe0e3 Signed-off-by: Yu-Ping Wu <yupingso@google.com> Change-Id: I75df038fca834cc3b86b3c78edcaacfe57f7eba0 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/8238251 Commit-Queue: Yu-Ping Wu <yupingso@chromium.org> Tested-by: Yu-Ping Wu <yupingso@chromium.org> Auto-Submit: Yu-Ping Wu <yupingso@chromium.org> Reviewed-by: Hsuan Ting Chen <roccochen@chromium.org>
diff --git a/futility/archive/updater_archive_fallback.c b/futility/archive/updater_archive_fallback.c index 235e0ed..c36b2c5 100644 --- a/futility/archive/updater_archive_fallback.c +++ b/futility/archive/updater_archive_fallback.c
@@ -19,6 +19,7 @@ #include "2common.h" #include "futility.h" #include "host_misc.h" +#include "subprocess.h" #include "updater_archive.h" #include "updater_utils.h" @@ -135,12 +136,21 @@ if (strchr(path, '/')) { char *dirname = strdup(path); *strrchr(dirname, '/') = '\0'; - /* TODO(hungte): call mkdir(2) instead of shell invocation. */ - if (access(dirname, W_OK) != 0) { - char *command; - ASPRINTF(&command, "mkdir -p %s", dirname); - free(host_shell(command)); - free(command); + /* + * If path is in the root directory (e.g. "/foo"), dirname + * becomes "" after truncation. Root directory already exists + * and does not need to be created. + */ + /* TODO(hungte): call mkdir(2) instead of subprocess invocation. */ + if (*dirname && access(dirname, W_OK) != 0) { + const char *const argv[] = {"mkdir", "-p", dirname, NULL}; + if (subprocess_run(argv, &subprocess_null, &subprocess_null, + &subprocess_null) != 0) { + ERROR("Failed to create directory %s\n", dirname); + free(dirname); + free(temp_path); + return 1; + } } free(dirname); }
diff --git a/futility/updater_manifest.c b/futility/updater_manifest.c index cfad4f9..40623cb 100644 --- a/futility/updater_manifest.c +++ b/futility/updater_manifest.c
@@ -78,13 +78,11 @@ /* Returns the VPD value by given key name, or NULL on error (or no value). */ static char *vpd_get_value(const char *fpath, const char *key) { - char *command, *result; + char *result; assert(fpath); - ASPRINTF(&command, "vpd -g %s -f %s 2>/dev/null", key, fpath); - result = host_shell(command); - free(command); - + const char *const argv[] = {"vpd", "-g", key, "-f", fpath, NULL}; + result = host_exec_output(argv); if (result && !*result) { free(result); result = NULL;
diff --git a/futility/updater_quirks.c b/futility/updater_quirks.c index 9a64fc1..f4bf902 100644 --- a/futility/updater_quirks.c +++ b/futility/updater_quirks.c
@@ -16,6 +16,7 @@ #include "futility.h" #include "host_misc.h" #include "platform_csme.h" +#include "subprocess.h" #include "updater.h" struct quirks_record { @@ -174,7 +175,6 @@ { const char *smm_store_name = "smm_store"; const char *old_store; - char *command; const char *temp_image = get_firmware_image_temp_file( &cfg->image_current, &cfg->tempfiles); @@ -198,15 +198,31 @@ if (!temp_image) return -1; - /* crosreview.com/1165109: The offset is fixed at 0x1bf000. */ - ASPRINTF(&command, - "cbfstool \"%s\" remove -r %s -n \"%s\" 2>/dev/null; " - "cbfstool \"%s\" add -r %s -n \"%s\" -f \"%s\" " - " -t raw -b 0x1bf000", temp_image, FMAP_RW_LEGACY, - smm_store_name, temp_image, FMAP_RW_LEGACY, - smm_store_name, old_store); - free(host_shell(command)); - free(command); + /* + * https://crrev.com/c/1165109: The offset is fixed at 0x1bf000. + * Remove old SMM store if present. Ignore exit code > 0 (file not + * found), but abort on execution error (< 0). + */ + const char *const rm_argv[] = { + "cbfstool", temp_image, "remove", "-r", FMAP_RW_LEGACY, + "-n", smm_store_name, NULL + }; + if (subprocess_run(rm_argv, &subprocess_null, &subprocess_null, + &subprocess_null) < 0) { + ERROR("Failed to execute cbfstool remove.\n"); + return -1; + } + + const char *const add_argv[] = { + "cbfstool", temp_image, "add", "-r", FMAP_RW_LEGACY, + "-n", smm_store_name, "-f", old_store, "-t", "raw", + "-b", "0x1bf000", NULL + }; + if (subprocess_run(add_argv, &subprocess_null, &subprocess_null, + &subprocess_null) != 0) { + ERROR("Failed to re-add SMM store to image.\n"); + return -1; + } return reload_firmware_image(temp_image, &cfg->image); }
diff --git a/futility/updater_utils.c b/futility/updater_utils.c index 032dc3b..c362728 100644 --- a/futility/updater_utils.c +++ b/futility/updater_utils.c
@@ -19,6 +19,7 @@ #include "2common.h" #include "cbfstool.h" #include "host_misc.h" +#include "subprocess.h" #include "util_misc.h" #include "updater.h" @@ -473,46 +474,49 @@ } test_mockable -char *host_shell(const char *command) +char *host_exec_output(const char *const argv[]) { + assert(argv && *argv); + /* Currently all commands we use do not have large output. */ char buf[COMMAND_BUFFER_SIZE]; + struct subprocess_target output = { + .type = TARGET_BUFFER_NULL_TERMINATED, + .buffer = { + .buf = buf, + .size = sizeof(buf), + }, + }; - int result; - FILE *fp = popen(command, "r"); - - VB2_DEBUG("%s\n", command); buf[0] = '\0'; - if (!fp) { - VB2_DEBUG("Execution error for %s.\n", command); + int status = subprocess_run(argv, &subprocess_null, &output, + &subprocess_null); + if (status == 0) { + strip_string(buf, NULL); return strdup(buf); } - if (fgets(buf, sizeof(buf), fp)) - strip_string(buf, NULL); - result = pclose(fp); - if (!WIFEXITED(result) || WEXITSTATUS(result) != 0) { - VB2_DEBUG("Execution failure with exit code %d: %s\n", - WEXITSTATUS(result), command); - /* - * Discard all output if command failed, for example command - * syntax failure may lead to garbage in stdout. - */ - buf[0] = '\0'; - } - return strdup(buf); + if (status < 0) + ERROR("Execution error %d for command: %s\n", + status, argv[0] ? argv[0] : ""); + else + VB2_DEBUG("Command %s exited with status %d\n", + argv[0] ? argv[0] : "", status); + + return NULL; } test_mockable void prepare_servo_control(const char *control_name, bool on) { - char *cmd; + char *arg; if (!control_name) return; - ASPRINTF(&cmd, "dut-control %s:%s", control_name, on ? "on" : "off"); - free(host_shell(cmd)); - free(cmd); + ASPRINTF(&arg, "%s:%s", control_name, on ? "on" : "off"); + const char *const argv[] = {"dut-control", arg, NULL}; + free(host_exec_output(argv)); + free(arg); } test_mockable @@ -521,7 +525,8 @@ const char *servo_port = getenv(ENV_SERVOD_PORT); const char *servo_name = getenv(ENV_SERVOD_NAME); const char *servo_id = servo_port, *servo_id_type = ENV_SERVOD_PORT; - char *servo_type = host_shell("dut-control -o servo_type 2>/dev/null"); + const char *const type_argv[] = {"dut-control", "-o", "servo_type", NULL}; + char *servo_type = host_exec_output(type_argv); const char *programmer = NULL; char *ret = NULL; char *servo_serial = NULL; @@ -529,11 +534,11 @@ static const char * const raiden_debug_spi = "raiden_debug_spi"; static const char * const cpu_fw_spi = "cpu_fw_spi"; static const char * const ccd_cpu_fw_spi = "ccd_cpu_fw_spi"; - const char *serial_cmd = "dut-control -o serialname 2>/dev/null"; + const char *serial_name = "serialname"; /* By default, no control is needed. */ *prepare_ctrl_name = NULL; - VB2_DEBUG("servo_type: %s\n", servo_type); + VB2_DEBUG("servo_type: %s\n", servo_type ? servo_type : "<null>"); /* dut-control defaults to port 9999, or non-empty servo_name. */ if (!servo_id || !*servo_id) { @@ -547,7 +552,7 @@ assert(servo_id && *servo_id); /* servo_type names: chromite/lib/firmware/servo_lib.py */ - if (!*servo_type) { + if (!servo_type || !*servo_type) { ERROR("Failed to get servo type. Check servod.\n"); } else if (strcmp(servo_type, "servo_v2") == 0) { VB2_DEBUG("Selected Servo V2.\n"); @@ -557,15 +562,14 @@ VB2_DEBUG("Selected Servo Micro.\n"); programmer = raiden_debug_spi; *prepare_ctrl_name = cpu_fw_spi; - serial_cmd = ("dut-control -o servo_micro_serialname" - " 2>/dev/null"); + serial_name = "servo_micro_serialname"; } else if (strstr(servo_type, "ccd_cr50") || strstr(servo_type, "ccd_gsc") || strstr(servo_type, "ccd_ti50")) { VB2_DEBUG("Selected CCD.\n"); programmer = "raiden_debug_spi:target=AP,custom_rst=true"; *prepare_ctrl_name = ccd_cpu_fw_spi; - serial_cmd = "dut-control -o ccd_serialname 2>/dev/null"; + serial_name = "ccd_serialname"; } else if (strstr(servo_type, "c2d2")) { /* Most C2D2 devices don't support flashing AP, so this must * come after CCD. @@ -573,8 +577,7 @@ VB2_DEBUG("Selected C2D2.\n"); programmer = raiden_debug_spi; *prepare_ctrl_name = cpu_fw_spi; - serial_cmd = ("dut-control -o c2d2_serialname" - " 2>/dev/null"); + serial_name = "c2d2_serialname"; } else { WARN("Unknown servo: %s\nAssuming debug header.\n", servo_type); programmer = raiden_debug_spi; @@ -586,20 +589,20 @@ * should always try to get the serial number. */ VB2_DEBUG("Select servod by %s=%s\n", servo_id_type, servo_id); - servo_serial = host_shell(serial_cmd); - VB2_DEBUG("Servo SN=%s (serial cmd: %s)\n", servo_serial, serial_cmd); - if (!(servo_serial && *servo_serial)) { + const char *const serial_argv[] = { + "dut-control", "-o", serial_name, NULL + }; + servo_serial = host_exec_output(serial_argv); + VB2_DEBUG("Servo SN=%s (serial name: %s)\n", + servo_serial ? servo_serial : "<null>", serial_name); + if (!servo_serial || !*servo_serial) { ERROR("Failed to get serial: %s=%s\n", servo_id_type, servo_id); /* If there is no servo serial, undo the prepare_ctrl_name. */ *prepare_ctrl_name = NULL; } else if (programmer) { - if (!servo_serial) { - ret = strdup(programmer); - } else { - const char prefix = strchr(programmer, ':') ? ',' : ':'; - ASPRINTF(&ret, "%s%cserial=%s", programmer, prefix, - servo_serial); - } + const char prefix = strchr(programmer, ':') ? ',' : ':'; + ASPRINTF(&ret, "%s%cserial=%s", programmer, prefix, + servo_serial); VB2_DEBUG("Servo programmer: %s\n", ret); }
diff --git a/futility/updater_utils.h b/futility/updater_utils.h index 79da7f9..0253726 100644 --- a/futility/updater_utils.h +++ b/futility/updater_utils.h
@@ -199,10 +199,10 @@ /* * Executes a command on current host and returns stripped command output. - * If the command has failed (exit code is not zero), returns an empty string. + * Returns NULL if the command failed to execute or exited with a non-zero status. * The caller is responsible for releasing the returned string. */ -char *host_shell(const char *command); +char *host_exec_output(const char *const argv[]); /* The environment variable name for setting servod port. */ #define ENV_SERVOD_PORT "SERVOD_PORT"
diff --git a/tests/futility/test_updater_utils.c b/tests/futility/test_updater_utils.c index 0404906..2eec2df 100644 --- a/tests/futility/test_updater_utils.c +++ b/tests/futility/test_updater_utils.c
@@ -572,12 +572,27 @@ updater_delete_config(cfg); cfg = NULL; - res_shell = host_shell("echo test"); - TEST_STR_EQ(res_shell, "test", "Host shell: echo"); + const char *const argv_echo[] = {"echo", "test", NULL}; + res_shell = host_exec_output(argv_echo); + TEST_STR_EQ(res_shell, "test", "Host exec output: echo"); free(res_shell); - res_shell = host_shell(")certainly_not_a_valid_thing"); - TEST_STR_EQ(res_shell, "", "Host shell: invalid command"); + const char *const argv_empty[] = {"true", NULL}; + res_shell = host_exec_output(argv_empty); + TEST_STR_EQ(res_shell, "", "Host exec output: empty"); + free(res_shell); + + const char *const argv_spaces[] = { + "echo", "hello world; test 'quoted' $VAR", NULL + }; + res_shell = host_exec_output(argv_spaces); + TEST_STR_EQ(res_shell, "hello world; test 'quoted' $VAR", + "Host exec output: special chars"); + free(res_shell); + + const char *const argv_invalid[] = {")certainly_not_a_valid_thing", NULL}; + res_shell = host_exec_output(argv_invalid); + TEST_PTR_EQ(res_shell, NULL, "Host exec output: invalid command"); free(res_shell); model = get_model_from_frid("some.frid");
diff --git a/tests/futility/test_updater_utils_servo.c b/tests/futility/test_updater_utils_servo.c index a0a8bbb..1d6bed4 100644 --- a/tests/futility/test_updater_utils_servo.c +++ b/tests/futility/test_updater_utils_servo.c
@@ -20,35 +20,35 @@ SHELL_SHOW_SERIAL_NUMBER, }; -static int host_shell_show; +static int host_exec_show; /* To emulate servo responses. */ -char *host_shell(const char *command) +char *host_exec_output(const char *const argv[]) { - switch (host_shell_show) { + switch (host_exec_show) { case SHELL_SHOW_SERVO_V2: - host_shell_show = SHELL_SHOW_SERIAL_NUMBER; + host_exec_show = SHELL_SHOW_SERIAL_NUMBER; return strdup("servo_v2"); case SHELL_SHOW_SERVO_MICRO: - host_shell_show = SHELL_SHOW_SERIAL_NUMBER; + host_exec_show = SHELL_SHOW_SERIAL_NUMBER; return strdup("servo_micro"); case SHELL_SHOW_CCD_CR50: - host_shell_show = SHELL_SHOW_SERIAL_NUMBER; + host_exec_show = SHELL_SHOW_SERIAL_NUMBER; return strdup("ccd_cr50"); case SHELL_SHOW_CCD_GSC: - host_shell_show = SHELL_SHOW_SERIAL_NUMBER; + host_exec_show = SHELL_SHOW_SERIAL_NUMBER; return strdup("ccd_gsc"); case SHELL_SHOW_CCD_TI50: - host_shell_show = SHELL_SHOW_SERIAL_NUMBER; + host_exec_show = SHELL_SHOW_SERIAL_NUMBER; return strdup("ccd_ti50"); case SHELL_SHOW_C2D2: - host_shell_show = SHELL_SHOW_SERIAL_NUMBER; + host_exec_show = SHELL_SHOW_SERIAL_NUMBER; return strdup("c2d2"); case SHELL_SHOW_UNKNOWN: - host_shell_show = SHELL_SHOW_SERIAL_NUMBER; + host_exec_show = SHELL_SHOW_SERIAL_NUMBER; return strdup("<unknown>"); case SHELL_SHOW_INVALID: - host_shell_show = SHELL_SHOW_EMPTY; + host_exec_show = SHELL_SHOW_EMPTY; return strdup("<invalid>"); case SHELL_SHOW_EMPTY: return strdup(""); @@ -67,58 +67,62 @@ setenv(ENV_SERVOD_PORT, "1234", 1); setenv(ENV_SERVOD_NAME, "some-servo-name", 1); - host_shell_show = SHELL_SHOW_SERVO_V2; + host_exec_show = SHELL_SHOW_SERVO_V2; prog = host_detect_servo(&ctrl); TEST_EQ(strcmp(prog, "ft2232_spi:type=google-servo-v2,serial=serial-number") || strcmp(ctrl, "cpu_fw_spi"), 0, "Servo servo_v2"); - host_shell_show = SHELL_SHOW_SERVO_MICRO; + host_exec_show = SHELL_SHOW_SERVO_MICRO; prog = host_detect_servo(&ctrl); TEST_EQ(strcmp(prog, "raiden_debug_spi:serial=serial-number") || strcmp(ctrl, "cpu_fw_spi"), 0, "Servo servo_micro"); - host_shell_show = SHELL_SHOW_CCD_CR50; + host_exec_show = SHELL_SHOW_CCD_CR50; prog = host_detect_servo(&ctrl); TEST_EQ(strcmp(prog, "raiden_debug_spi:target=AP,custom_rst=true,serial=serial-number") || strcmp(ctrl, "ccd_cpu_fw_spi"), 0, "Servo ccd_cr50"); - host_shell_show = SHELL_SHOW_CCD_GSC; + host_exec_show = SHELL_SHOW_CCD_GSC; prog = host_detect_servo(&ctrl); TEST_EQ(strcmp(prog, "raiden_debug_spi:target=AP,custom_rst=true,serial=serial-number") || strcmp(ctrl, "ccd_cpu_fw_spi"), 0, "Servo ccd_gsc"); - host_shell_show = SHELL_SHOW_CCD_TI50; + host_exec_show = SHELL_SHOW_CCD_TI50; prog = host_detect_servo(&ctrl); TEST_EQ(strcmp(prog, "raiden_debug_spi:target=AP,custom_rst=true,serial=serial-number") || strcmp(ctrl, "ccd_cpu_fw_spi"), 0, "Servo ccd_ti50"); - host_shell_show = SHELL_SHOW_C2D2; + host_exec_show = SHELL_SHOW_C2D2; prog = host_detect_servo(&ctrl); TEST_EQ(strcmp(prog, "raiden_debug_spi:serial=serial-number") || strcmp(ctrl, "cpu_fw_spi"), 0, "Servo c2d2"); - host_shell_show = SHELL_SHOW_UNKNOWN; + host_exec_show = SHELL_SHOW_UNKNOWN; prog = host_detect_servo(&ctrl); TEST_EQ(strcmp(prog, "raiden_debug_spi:serial=serial-number") || strcmp(ctrl, "cpu_fw_spi"), 0, "Servo unknown"); - host_shell_show = SHELL_SHOW_INVALID; + host_exec_show = SHELL_SHOW_INVALID; prog = host_detect_servo(&ctrl); TEST_EQ(prog == NULL && ctrl == NULL, 1, "Servo invalid"); + host_exec_show = SHELL_SHOW_DEFAULT; + prog = host_detect_servo(&ctrl); + TEST_EQ(prog == NULL && ctrl == NULL, 1, "Servo null"); + setenv(ENV_SERVOD_PORT, "", 1); setenv(ENV_SERVOD_NAME, "some-servo-name", 1); - host_shell_show = SHELL_SHOW_SERVO_V2; + host_exec_show = SHELL_SHOW_SERVO_V2; prog = host_detect_servo(&ctrl); TEST_EQ(strcmp(prog, "ft2232_spi:type=google-servo-v2,serial=serial-number") || strcmp(ctrl, "cpu_fw_spi"), @@ -126,7 +130,7 @@ setenv(ENV_SERVOD_PORT, "1234", 1); setenv(ENV_SERVOD_NAME, "", 1); - host_shell_show = SHELL_SHOW_SERVO_V2; + host_exec_show = SHELL_SHOW_SERVO_V2; prog = host_detect_servo(&ctrl); TEST_EQ(strcmp(prog, "ft2232_spi:type=google-servo-v2,serial=serial-number") || strcmp(ctrl, "cpu_fw_spi"), @@ -134,7 +138,7 @@ setenv(ENV_SERVOD_PORT, "", 1); setenv(ENV_SERVOD_NAME, "", 1); - host_shell_show = SHELL_SHOW_UNKNOWN; + host_exec_show = SHELL_SHOW_UNKNOWN; prog = host_detect_servo(&ctrl); TEST_EQ(strcmp(prog, "raiden_debug_spi:serial=serial-number") || strcmp(ctrl, "cpu_fw_spi"),