From 061a87aff8650ba55bc698dbe7f00a08a4abb6e5 Mon Sep 17 00:00:00 2001 From: Siddharth Chandrasekaran Date: Sat, 15 Jul 2023 03:32:29 +0200 Subject: [PATCH] mgmt/osdp: Replace __ASSERT() with an explicit if Commit c7fec71193a ("mgmt/osdp: Add length checks for commands and replies") attempted to remove code duplication by adding a macro to perform a length check. At the time, a CI linter did not like macros with control flow so the code was switched to a method which called __ASSERT() on this condition. The __ASSERT() macro is a nop if CONFIG_ASSERT=n (which is the default) and causes the buffer access to be unguarded which may lead to OOB accesses. This patch fixes the issue by reintroducing the if check. Fixes: c7fec71193a19f6be1a2adca8cf7753cd7103c78. Signed-off-by: Siddharth Chandrasekaran --- subsys/mgmt/osdp/src/osdp_cp.c | 73 +++++++++++++++++++++++++--------- subsys/mgmt/osdp/src/osdp_pd.c | 61 ++++++++++++++++++++-------- 2 files changed, 99 insertions(+), 35 deletions(-) diff --git a/subsys/mgmt/osdp/src/osdp_cp.c b/subsys/mgmt/osdp/src/osdp_cp.c index 86a1e9d20e3..e9c5d19d2eb 100644 --- a/subsys/mgmt/osdp/src/osdp_cp.c +++ b/subsys/mgmt/osdp/src/osdp_cp.c @@ -114,10 +114,13 @@ int osdp_extract_address(int *address) return (pd_offset == CONFIG_OSDP_NUM_CONNECTED_PD) ? 0 : -1; } -static inline void assert_buf_len(int need, int have) +static inline bool check_buf_len(int need, int have) { - __ASSERT(need < have, "OOM at build command: need:%d have:%d", - need, have); + if (need >= have) { + LOG_ERR("OOM at build command: need:%d have:%d", need, have); + return false; + } + return true; } static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) @@ -137,42 +140,60 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) switch (pd->cmd_id) { case CMD_POLL: - assert_buf_len(CMD_POLL_LEN, max_len); + if (!check_buf_len(CMD_POLL_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } buf[len++] = pd->cmd_id; break; case CMD_LSTAT: - assert_buf_len(CMD_LSTAT_LEN, max_len); + if (!check_buf_len(CMD_LSTAT_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } buf[len++] = pd->cmd_id; break; case CMD_ISTAT: - assert_buf_len(CMD_ISTAT_LEN, max_len); + if (!check_buf_len(CMD_ISTAT_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } buf[len++] = pd->cmd_id; break; case CMD_OSTAT: - assert_buf_len(CMD_OSTAT_LEN, max_len); + if (!check_buf_len(CMD_OSTAT_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } buf[len++] = pd->cmd_id; break; case CMD_RSTAT: - assert_buf_len(CMD_RSTAT_LEN, max_len); + if (!check_buf_len(CMD_RSTAT_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } buf[len++] = pd->cmd_id; break; case CMD_ID: - assert_buf_len(CMD_ID_LEN, max_len); + if (!check_buf_len(CMD_ID_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } buf[len++] = pd->cmd_id; buf[len++] = 0x00; break; case CMD_CAP: - assert_buf_len(CMD_CAP_LEN, max_len); + if (!check_buf_len(CMD_CAP_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } buf[len++] = pd->cmd_id; buf[len++] = 0x00; break; case CMD_DIAG: - assert_buf_len(CMD_DIAG_LEN, max_len); + if (!check_buf_len(CMD_DIAG_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } buf[len++] = pd->cmd_id; buf[len++] = 0x00; break; case CMD_OUT: - assert_buf_len(CMD_OUT_LEN, max_len); + if (!check_buf_len(CMD_OUT_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } cmd = (struct osdp_cmd *)pd->ephemeral_data; buf[len++] = pd->cmd_id; buf[len++] = cmd->output.output_no; @@ -181,7 +202,9 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) buf[len++] = BYTE_1(cmd->output.timer_count); break; case CMD_LED: - assert_buf_len(CMD_LED_LEN, max_len); + if (!check_buf_len(CMD_LED_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } cmd = (struct osdp_cmd *)pd->ephemeral_data; buf[len++] = pd->cmd_id; buf[len++] = cmd->led.reader; @@ -202,7 +225,9 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) buf[len++] = cmd->led.permanent.off_color; break; case CMD_BUZ: - assert_buf_len(CMD_BUZ_LEN, max_len); + if (!check_buf_len(CMD_BUZ_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } cmd = (struct osdp_cmd *)pd->ephemeral_data; buf[len++] = pd->cmd_id; buf[len++] = cmd->buzzer.reader; @@ -213,7 +238,9 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) break; case CMD_TEXT: cmd = (struct osdp_cmd *)pd->ephemeral_data; - assert_buf_len(CMD_TEXT_LEN + cmd->text.length, max_len); + if (!check_buf_len(CMD_TEXT_LEN + cmd->text.length, max_len)) { + return OSDP_CP_ERR_GENERIC; + } buf[len++] = pd->cmd_id; buf[len++] = cmd->text.reader; buf[len++] = cmd->text.control_code; @@ -225,7 +252,9 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) len += cmd->text.length; break; case CMD_COMSET: - assert_buf_len(CMD_COMSET_LEN, max_len); + if (!check_buf_len(CMD_COMSET_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } cmd = (struct osdp_cmd *)pd->ephemeral_data; buf[len++] = pd->cmd_id; buf[len++] = cmd->comset.address; @@ -240,7 +269,9 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) LOG_ERR("Cannot perform KEYSET without SC!"); return OSDP_CP_ERR_GENERIC; } - assert_buf_len(CMD_KEYSET_LEN, max_len); + if (!check_buf_len(CMD_KEYSET_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } cmd = (struct osdp_cmd *)pd->ephemeral_data; if (cmd->keyset.length != 16) { LOG_ERR("Invalid key length"); @@ -260,7 +291,9 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) len += 16; break; case CMD_CHLNG: - assert_buf_len(CMD_CHLNG_LEN, max_len); + if (!check_buf_len(CMD_CHLNG_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } if (smb == NULL) { LOG_ERR("Invalid secure message block!"); return -1; @@ -273,7 +306,9 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) len += 8; break; case CMD_SCRYPT: - assert_buf_len(CMD_SCRYPT_LEN, max_len); + if (!check_buf_len(CMD_SCRYPT_LEN, max_len)) { + return OSDP_CP_ERR_GENERIC; + } if (smb == NULL) { LOG_ERR("Invalid secure message block!"); return -1; diff --git a/subsys/mgmt/osdp/src/osdp_pd.c b/subsys/mgmt/osdp/src/osdp_pd.c index ceb17c2d1a9..9f3e7d42892 100644 --- a/subsys/mgmt/osdp/src/osdp_pd.c +++ b/subsys/mgmt/osdp/src/osdp_pd.c @@ -556,10 +556,13 @@ static int pd_decode_command(struct osdp_pd *pd, uint8_t *buf, int len) return ret; } -static inline void assert_buf_len(int need, int have) +static inline bool check_buf_len(int need, int have) { - __ASSERT(need < have, "OOM at build command: need:%d have:%d", - need, have); + if (need >= have) { + LOG_ERR("OOM at build reply: need:%d have:%d", need, have); + return false; + } + return true; } /** @@ -582,12 +585,16 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) switch (pd->reply_id) { case REPLY_ACK: - assert_buf_len(REPLY_ACK_LEN, max_len); + if (!check_buf_len(REPLY_ACK_LEN, max_len)) { + return OSDP_PD_ERR_GENERIC; + } buf[len++] = pd->reply_id; ret = OSDP_PD_ERR_NONE; break; case REPLY_PDID: - assert_buf_len(REPLY_PDID_LEN, max_len); + if (!check_buf_len(REPLY_PDID_LEN, max_len)) { + return OSDP_PD_ERR_GENERIC; + } buf[len++] = pd->reply_id; buf[len++] = BYTE_0(pd->id.vendor_code); @@ -608,7 +615,9 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) ret = OSDP_PD_ERR_NONE; break; case REPLY_PDCAP: - assert_buf_len(REPLY_PDCAP_LEN, max_len); + if (!check_buf_len(REPLY_PDCAP_LEN, max_len)) { + return OSDP_PD_ERR_GENERIC; + } buf[len++] = pd->reply_id; for (i = 1; i < OSDP_PD_CAP_SENTINEL; i++) { if (pd->cap[i].function_code != i) { @@ -626,21 +635,27 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) ret = OSDP_PD_ERR_NONE; break; case REPLY_LSTATR: - assert_buf_len(REPLY_LSTATR_LEN, max_len); + if (!check_buf_len(REPLY_LSTATR_LEN, max_len)) { + return OSDP_PD_ERR_GENERIC; + } buf[len++] = pd->reply_id; buf[len++] = ISSET_FLAG(pd, PD_FLAG_TAMPER); buf[len++] = ISSET_FLAG(pd, PD_FLAG_POWER); ret = OSDP_PD_ERR_NONE; break; case REPLY_RSTATR: - assert_buf_len(REPLY_RSTATR_LEN, max_len); + if (!check_buf_len(REPLY_RSTATR_LEN, max_len)) { + return OSDP_PD_ERR_GENERIC; + } buf[len++] = pd->reply_id; buf[len++] = ISSET_FLAG(pd, PD_FLAG_R_TAMPER); ret = OSDP_PD_ERR_NONE; break; case REPLY_KEYPPAD: event = (struct osdp_event *)pd->ephemeral_data; - assert_buf_len(REPLY_KEYPAD_LEN + event->keypress.length, max_len); + if (!check_buf_len(REPLY_KEYPAD_LEN + event->keypress.length, max_len)) { + return OSDP_PD_ERR_GENERIC; + } buf[len++] = pd->reply_id; buf[len++] = (uint8_t)event->keypress.reader_no; buf[len++] = (uint8_t)event->keypress.length; @@ -653,7 +668,9 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) event = (struct osdp_event *)pd->ephemeral_data; len_bytes = (event->cardread.length + 7) / 8; - assert_buf_len(REPLY_RAW_LEN + len_bytes, max_len); + if (!check_buf_len(REPLY_RAW_LEN + len_bytes, max_len)) { + return OSDP_PD_ERR_GENERIC; + } buf[len++] = pd->reply_id; buf[len++] = (uint8_t)event->cardread.reader_no; buf[len++] = (uint8_t)event->cardread.format; @@ -666,7 +683,9 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) } case REPLY_FMT: event = (struct osdp_event *)pd->ephemeral_data; - assert_buf_len(REPLY_FMT_LEN + event->cardread.length, max_len); + if (!check_buf_len(REPLY_FMT_LEN + event->cardread.length, max_len)) { + return OSDP_PD_ERR_GENERIC; + } buf[len++] = pd->reply_id; buf[len++] = (uint8_t)event->cardread.reader_no; buf[len++] = (uint8_t)event->cardread.direction; @@ -676,7 +695,9 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) ret = OSDP_PD_ERR_NONE; break; case REPLY_COM: - assert_buf_len(REPLY_COM_LEN, max_len); + if (!check_buf_len(REPLY_COM_LEN, max_len)) { + return OSDP_PD_ERR_GENERIC; + } /** * If COMSET succeeds, the PD must reply with the old params and * then switch to the new params from then then on. We have the @@ -702,7 +723,9 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) ret = OSDP_PD_ERR_NONE; break; case REPLY_NAK: - assert_buf_len(REPLY_NAK_LEN, max_len); + if (!check_buf_len(REPLY_NAK_LEN, max_len)) { + return OSDP_PD_ERR_GENERIC; + } buf[len++] = pd->reply_id; buf[len++] = pd->ephemeral_data[0]; ret = OSDP_PD_ERR_NONE; @@ -712,7 +735,9 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) if (smb == NULL) { break; } - assert_buf_len(REPLY_CCRYPT_LEN, max_len); + if (!check_buf_len(REPLY_CCRYPT_LEN, max_len)) { + return OSDP_PD_ERR_GENERIC; + } osdp_fill_random(pd->sc.pd_random, 8); osdp_compute_session_keys(pd); osdp_compute_pd_cryptogram(pd); @@ -730,7 +755,9 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) if (smb == NULL) { break; } - assert_buf_len(REPLY_RMAC_I_LEN, max_len); + if (!check_buf_len(REPLY_RMAC_I_LEN, max_len)) { + return OSDP_PD_ERR_GENERIC; + } osdp_compute_rmac_i(pd); buf[len++] = pd->reply_id; memcpy(buf + len, pd->sc.r_mac, 16); @@ -766,7 +793,9 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) /* catch all errors and report it as a RECORD error to CP */ LOG_ERR("Failed to build REPLY(%02x); Sending NAK instead!", pd->reply_id); - assert_buf_len(REPLY_NAK_LEN, max_len); + if (!check_buf_len(REPLY_NAK_LEN, max_len)) { + return OSDP_PD_ERR_GENERIC; + } buf[0] = REPLY_NAK; buf[1] = OSDP_PD_NAK_RECORD; len = 2;