From aa922032158910489e22c420a0b51eca67e243da Mon Sep 17 00:00:00 2001 From: Petr Ledvina Date: Tue, 10 Sep 2024 18:15:52 +0200 Subject: [PATCH] rc-modes bugfix - prevent buffer overflow when serializing box names (#13880) --- src/main/cli/settings.c | 8 ++++---- src/main/fc/rc_modes.c | 9 +-------- src/main/fc/rc_modes.h | 8 +++----- src/main/msp/msp_box.c | 44 +++++++++++++++++++++++++++-------------- src/main/msp/msp_box.h | 6 +++--- 5 files changed, 40 insertions(+), 35 deletions(-) diff --git a/src/main/cli/settings.c b/src/main/cli/settings.c index 6a334f675a..2e0f524222 100644 --- a/src/main/cli/settings.c +++ b/src/main/cli/settings.c @@ -1835,10 +1835,10 @@ const clivalue_t valueTable[] = { // PG_MODE_ACTIVATION_CONFIG #if defined(USE_CUSTOM_BOX_NAMES) - { "box_user_1_name", VAR_UINT8 | HARDWARE_VALUE | MODE_STRING, .config.string = { 1, MAX_BOX_USER_NAME_LENGTH, STRING_FLAGS_NONE }, PG_MODE_ACTIVATION_CONFIG, offsetof(modeActivationConfig_t, box_user_1_name) }, - { "box_user_2_name", VAR_UINT8 | HARDWARE_VALUE | MODE_STRING, .config.string = { 1, MAX_BOX_USER_NAME_LENGTH, STRING_FLAGS_NONE }, PG_MODE_ACTIVATION_CONFIG, offsetof(modeActivationConfig_t, box_user_2_name) }, - { "box_user_3_name", VAR_UINT8 | HARDWARE_VALUE | MODE_STRING, .config.string = { 1, MAX_BOX_USER_NAME_LENGTH, STRING_FLAGS_NONE }, PG_MODE_ACTIVATION_CONFIG, offsetof(modeActivationConfig_t, box_user_3_name) }, - { "box_user_4_name", VAR_UINT8 | HARDWARE_VALUE | MODE_STRING, .config.string = { 1, MAX_BOX_USER_NAME_LENGTH, STRING_FLAGS_NONE }, PG_MODE_ACTIVATION_CONFIG, offsetof(modeActivationConfig_t, box_user_4_name) }, + { "box_user_1_name", VAR_UINT8 | HARDWARE_VALUE | MODE_STRING, .config.string = { 1, MAX_BOX_USER_NAME_LENGTH, STRING_FLAGS_NONE }, PG_MODE_ACTIVATION_CONFIG, offsetof(modeActivationConfig_t, box_user_names[0]) }, + { "box_user_2_name", VAR_UINT8 | HARDWARE_VALUE | MODE_STRING, .config.string = { 1, MAX_BOX_USER_NAME_LENGTH, STRING_FLAGS_NONE }, PG_MODE_ACTIVATION_CONFIG, offsetof(modeActivationConfig_t, box_user_names[1]) }, + { "box_user_3_name", VAR_UINT8 | HARDWARE_VALUE | MODE_STRING, .config.string = { 1, MAX_BOX_USER_NAME_LENGTH, STRING_FLAGS_NONE }, PG_MODE_ACTIVATION_CONFIG, offsetof(modeActivationConfig_t, box_user_names[2]) }, + { "box_user_4_name", VAR_UINT8 | HARDWARE_VALUE | MODE_STRING, .config.string = { 1, MAX_BOX_USER_NAME_LENGTH, STRING_FLAGS_NONE }, PG_MODE_ACTIVATION_CONFIG, offsetof(modeActivationConfig_t, box_user_names[3]) }, #endif }; diff --git a/src/main/fc/rc_modes.c b/src/main/fc/rc_modes.c index 94ed8507d7..ce2bc54ac4 100644 --- a/src/main/fc/rc_modes.c +++ b/src/main/fc/rc_modes.c @@ -59,14 +59,7 @@ static uint8_t activeLinkedMacArray[MAX_MODE_ACTIVATION_CONDITION_COUNT]; PG_REGISTER_ARRAY(modeActivationCondition_t, MAX_MODE_ACTIVATION_CONDITION_COUNT, modeActivationConditions, PG_MODE_ACTIVATION_PROFILE, 3); #if defined(USE_CUSTOM_BOX_NAMES) -PG_REGISTER_WITH_RESET_TEMPLATE(modeActivationConfig_t, modeActivationConfig, PG_MODE_ACTIVATION_CONFIG, 0); - -PG_RESET_TEMPLATE(modeActivationConfig_t, modeActivationConfig, - .box_user_1_name = { 0 }, - .box_user_2_name = { 0 }, - .box_user_3_name = { 0 }, - .box_user_4_name = { 0 }, -); +PG_REGISTER(modeActivationConfig_t, modeActivationConfig, PG_MODE_ACTIVATION_CONFIG, 0); #endif bool IS_RC_MODE_ACTIVE(boxId_e boxId) diff --git a/src/main/fc/rc_modes.h b/src/main/fc/rc_modes.h index e6282fcee4..dc0a4c126e 100644 --- a/src/main/fc/rc_modes.h +++ b/src/main/fc/rc_modes.h @@ -124,12 +124,10 @@ PG_DECLARE_ARRAY(modeActivationCondition_t, MAX_MODE_ACTIVATION_CONDITION_COUNT, #if defined(USE_CUSTOM_BOX_NAMES) #define MAX_BOX_USER_NAME_LENGTH 16 - +#define BOX_USER_NAME_COUNT 4 +STATIC_ASSERT(BOXUSER4 + 1 - BOXUSER1 == BOX_USER_NAME_COUNT, "Invalid BOX_USER_NAME_COUNT"); typedef struct modeActivationConfig_s { - char box_user_1_name[MAX_BOX_USER_NAME_LENGTH]; - char box_user_2_name[MAX_BOX_USER_NAME_LENGTH]; - char box_user_3_name[MAX_BOX_USER_NAME_LENGTH]; - char box_user_4_name[MAX_BOX_USER_NAME_LENGTH]; + char box_user_names[BOX_USER_NAME_COUNT][MAX_BOX_USER_NAME_LENGTH]; } modeActivationConfig_t; PG_DECLARE(modeActivationConfig_t, modeActivationConfig); diff --git a/src/main/msp/msp_box.c b/src/main/msp/msp_box.c index 37f99b1022..0b41ac1e11 100644 --- a/src/main/msp/msp_box.c +++ b/src/main/msp/msp_box.c @@ -137,28 +137,39 @@ static bool activeBoxIdGet(boxId_e boxId) return bitArrayGet(&activeBoxIds, boxId); } -void serializeBoxNameFn(sbuf_t *dst, const box_t *box) +int serializeBoxNameFn(sbuf_t *dst, const box_t *box) { + const char* name = NULL; + int len; #if defined(USE_CUSTOM_BOX_NAMES) - if (box->boxId == BOXUSER1 && strlen(modeActivationConfig()->box_user_1_name) > 0) { - sbufWriteString(dst, modeActivationConfig()->box_user_1_name); - } else if (box->boxId == BOXUSER2 && strlen(modeActivationConfig()->box_user_2_name) > 0) { - sbufWriteString(dst, modeActivationConfig()->box_user_2_name); - } else if (box->boxId == BOXUSER3 && strlen(modeActivationConfig()->box_user_3_name) > 0) { - sbufWriteString(dst, modeActivationConfig()->box_user_3_name); - } else if (box->boxId == BOXUSER4 && strlen(modeActivationConfig()->box_user_4_name) > 0) { - sbufWriteString(dst, modeActivationConfig()->box_user_4_name); - } else -#endif - { - sbufWriteString(dst, box->boxName); + if (name == NULL + && box->boxId >= BOXUSER1 && box->boxId <= BOXUSER4) { + const int n = box->boxId - BOXUSER1; + name = modeActivationConfig()->box_user_names[n]; + // possibly there is no '\0' in boxname + len = strnlen(name, sizeof(modeActivationConfig()->box_user_names[0])); } +#endif + if (name == NULL) { + name = box->boxName; + len = strlen(name); + } + if (sbufBytesRemaining(dst) < len + 1) { + // boxname or separator won't fit + return -1; + } + sbufWriteData(dst, name, len); sbufWriteU8(dst, ';'); + return len + 1; } -void serializeBoxPermanentIdFn(sbuf_t *dst, const box_t *box) +int serializeBoxPermanentIdFn(sbuf_t *dst, const box_t *box) { + if (sbufBytesRemaining(dst) < 1) { + return -1; + } sbufWriteU8(dst, box->permanentId); + return 1; } // serialize 'page' of boxNames. @@ -171,7 +182,10 @@ void serializeBoxReply(sbuf_t *dst, int page, serializeBoxFn *serializeBox) for (boxId_e id = 0; id < CHECKBOX_ITEM_COUNT; id++) { if (activeBoxIdGet(id)) { if (boxIdx >= pageStart && boxIdx < pageEnd) { - (*serializeBox)(dst, findBoxByBoxId(id)); + if ((*serializeBox)(dst, findBoxByBoxId(id)) < 0) { + // failed to serialize, abort + return; + } } boxIdx++; // count active boxes } diff --git a/src/main/msp/msp_box.h b/src/main/msp/msp_box.h index 5008a48b3b..525e5db322 100644 --- a/src/main/msp/msp_box.h +++ b/src/main/msp/msp_box.h @@ -36,9 +36,9 @@ const box_t *findBoxByPermanentId(uint8_t permanentId); struct boxBitmask_s; int packFlightModeFlags(struct boxBitmask_s *mspFlightModeFlags); struct sbuf_s; -void serializeBoxNameFn(struct sbuf_s *dst, const box_t *box); -void serializeBoxPermanentIdFn(struct sbuf_s *dst, const box_t *box); -typedef void serializeBoxFn(struct sbuf_s *dst, const box_t *box); +int serializeBoxNameFn(struct sbuf_s *dst, const box_t *box); +int serializeBoxPermanentIdFn(struct sbuf_s *dst, const box_t *box); +typedef int serializeBoxFn(struct sbuf_s *dst, const box_t *box); void serializeBoxReply(struct sbuf_s *dst, int page, serializeBoxFn *serializeBox); void initActiveBoxIds(void); bool getBoxIdState(boxId_e boxid);