From c7fec71193a19f6be1a2adca8cf7753cd7103c78 Mon Sep 17 00:00:00 2001 From: Siddharth Chandrasekaran Date: Mon, 20 Jun 2022 22:40:44 +0200 Subject: [PATCH] mgmt/osdp: Add length checks for commands and replies For all commands and replies, the buffer length needed to build or the length of data needed to decode needs to be checked and asserted. Right now we do this by ad-hoc if-s. Add macros that do this at a common location. Signed-off-by: Siddharth Chandrasekaran --- subsys/mgmt/osdp/src/osdp_cp.c | 74 +++++++++++++++++++--------------- subsys/mgmt/osdp/src/osdp_pd.c | 70 +++++++++----------------------- 2 files changed, 61 insertions(+), 83 deletions(-) diff --git a/subsys/mgmt/osdp/src/osdp_cp.c b/subsys/mgmt/osdp/src/osdp_cp.c index 3c8cfc2e8cc..e21747821de 100644 --- a/subsys/mgmt/osdp/src/osdp_cp.c +++ b/subsys/mgmt/osdp/src/osdp_cp.c @@ -80,6 +80,12 @@ 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) +{ + __ASSERT(need < have, "OOM at build command: need:%d have:%d", + need, have); +} + static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) { struct osdp_cmd *cmd = NULL; @@ -97,45 +103,50 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) switch (pd->cmd_id) { case CMD_POLL: - __fallthrough; + assert_buf_len(CMD_POLL_LEN, max_len); + buf[len++] = pd->cmd_id; + ret = 0; + break; case CMD_LSTAT: - __fallthrough; + assert_buf_len(CMD_LSTAT_LEN, max_len); + buf[len++] = pd->cmd_id; + ret = 0; + break; case CMD_ISTAT: - __fallthrough; + assert_buf_len(CMD_ISTAT_LEN, max_len); + buf[len++] = pd->cmd_id; + ret = 0; + break; case CMD_OSTAT: - __fallthrough; + assert_buf_len(CMD_OSTAT_LEN, max_len); + buf[len++] = pd->cmd_id; + ret = 0; + break; case CMD_RSTAT: + assert_buf_len(CMD_RSTAT_LEN, max_len); buf[len++] = pd->cmd_id; ret = 0; break; case CMD_ID: - if (max_len < CMD_ID_LEN) { - break; - } + assert_buf_len(CMD_ID_LEN, max_len); buf[len++] = pd->cmd_id; buf[len++] = 0x00; ret = 0; break; case CMD_CAP: - if (max_len < CMD_CAP_LEN) { - break; - } + assert_buf_len(CMD_CAP_LEN, max_len); buf[len++] = pd->cmd_id; buf[len++] = 0x00; ret = 0; break; case CMD_DIAG: - if (max_len < CMD_DIAG_LEN) { - break; - } + assert_buf_len(CMD_DIAG_LEN, max_len); buf[len++] = pd->cmd_id; buf[len++] = 0x00; ret = 0; break; case CMD_OUT: - if (max_len < CMD_OUT_LEN) { - break; - } + assert_buf_len(CMD_OUT_LEN, max_len); cmd = (struct osdp_cmd *)pd->cmd_data; buf[len++] = pd->cmd_id; buf[len++] = cmd->output.output_no; @@ -145,9 +156,7 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) ret = 0; break; case CMD_LED: - if (max_len < CMD_LED_LEN) { - break; - } + assert_buf_len(CMD_LED_LEN, max_len); cmd = (struct osdp_cmd *)pd->cmd_data; buf[len++] = pd->cmd_id; buf[len++] = cmd->led.reader; @@ -169,9 +178,7 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) ret = 0; break; case CMD_BUZ: - if (max_len < CMD_BUZ_LEN) { - break; - } + assert_buf_len(CMD_BUZ_LEN, max_len); cmd = (struct osdp_cmd *)pd->cmd_data; buf[len++] = pd->cmd_id; buf[len++] = cmd->buzzer.reader; @@ -183,9 +190,7 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) break; case CMD_TEXT: cmd = (struct osdp_cmd *)pd->cmd_data; - if (max_len < (CMD_TEXT_LEN + cmd->text.length)) { - break; - } + assert_buf_len(CMD_TEXT_LEN + cmd->text.length, max_len); buf[len++] = pd->cmd_id; buf[len++] = cmd->text.reader; buf[len++] = cmd->text.control_code; @@ -199,9 +204,7 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) ret = 0; break; case CMD_COMSET: - if (max_len < CMD_COMSET_LEN) { - break; - } + assert_buf_len(CMD_COMSET_LEN, max_len); cmd = (struct osdp_cmd *)pd->cmd_data; buf[len++] = pd->cmd_id; buf[len++] = cmd->comset.address; @@ -217,9 +220,7 @@ 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; } - if (max_len < CMD_KEYSET_LEN) { - break; - } + assert_buf_len(CMD_KEYSET_LEN, max_len); buf[len++] = pd->cmd_id; buf[len++] = 1; /* key type (1: SCBK) */ buf[len++] = 16; /* key length in bytes */ @@ -228,7 +229,8 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) ret = 0; break; case CMD_CHLNG: - if (smb == NULL || max_len < CMD_CHLNG_LEN) { + assert_buf_len(CMD_CHLNG_LEN, max_len); + if (smb == NULL) { break; } osdp_fill_random(pd->sc.cp_random, 8); @@ -242,7 +244,8 @@ static int cp_build_command(struct osdp_pd *pd, uint8_t *buf, int max_len) ret = 0; break; case CMD_SCRYPT: - if (smb == NULL || max_len < CMD_SCRYPT_LEN) { + assert_buf_len(CMD_SCRYPT_LEN, max_len); + if (smb == NULL) { break; } osdp_compute_cp_cryptogram(pd); @@ -489,6 +492,11 @@ static int cp_decode_response(struct osdp_pd *pd, uint8_t *buf, int len) return OSDP_CP_ERR_UNKNOWN; } + if (ret != OSDP_CP_ERR_NONE) { + LOG_ERR("Failed to decode response: REPLY(%02x) for CMD(%02x)", + pd->cmd_id, pd->reply_id); + } + if (pd->cmd_id != CMD_POLL) { LOG_DBG("CMD(%02x) REPLY(%02x)", pd->cmd_id, pd->reply_id); } diff --git a/subsys/mgmt/osdp/src/osdp_pd.c b/subsys/mgmt/osdp/src/osdp_pd.c index 228cf525f4c..f28f345ea19 100644 --- a/subsys/mgmt/osdp/src/osdp_pd.c +++ b/subsys/mgmt/osdp/src/osdp_pd.c @@ -299,7 +299,6 @@ static int pd_decode_command(struct osdp_pd *pd, uint8_t *buf, int len) #ifdef CONFIG_OSDP_SC_ENABLED case CMD_KEYSET: if (len != CMD_KEYSET_DATA_LEN) { - LOG_ERR("CMD_KEYSET length mismatch! %d/18", len); break; } /** @@ -342,7 +341,6 @@ static int pd_decode_command(struct osdp_pd *pd, uint8_t *buf, int len) break; } if (len != CMD_CHLNG_DATA_LEN) { - LOG_ERR("CMD_CHLNG length mismatch! %d/8", len); break; } osdp_sc_init(pd); @@ -356,7 +354,6 @@ static int pd_decode_command(struct osdp_pd *pd, uint8_t *buf, int len) } case CMD_SCRYPT: if (len != CMD_SCRYPT_DATA_LEN) { - LOG_ERR("CMD_SCRYPT length mismatch! %d/16", len); break; } for (i = 0; i < 16; i++) { @@ -373,11 +370,12 @@ static int pd_decode_command(struct osdp_pd *pd, uint8_t *buf, int len) return OSDP_PD_ERR_REPLY; } - if (ret != 0) { - LOG_ERR("Invalid command structure. CMD: %02x, Len: %d", - pd->cmd_id, len); + if (ret == OSDP_PD_ERR_GENERIC) { + LOG_ERR("Failed to decode command: CMD(%02x) Len:%d ret:%d", + pd->cmd_id, len, ret); pd->reply_id = REPLY_NAK; pd->cmd_data[0] = OSDP_PD_NAK_CMD_LEN; + ret = OSDP_PD_ERR_REPLY; } if (pd->cmd_id != CMD_POLL) { @@ -387,6 +385,12 @@ 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) +{ + __ASSERT(need < have, "OOM at build command: need:%d have:%d", + need, have); +} + /** * Returns: * +ve: length of command @@ -404,25 +408,15 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) #endif buf += data_off; max_len -= data_off; - if (max_len <= 0) { - LOG_ERR("Out of buffer space!"); - return -1; - } switch (pd->reply_id) { case REPLY_ACK: - if (max_len < REPLY_ACK_LEN) { - LOG_ERR("Out of buffer space!"); - break; - } + assert_buf_len(REPLY_ACK_LEN, max_len); buf[len++] = pd->reply_id; ret = OSDP_PD_ERR_NONE; break; case REPLY_PDID: - if (max_len < REPLY_PDID_LEN) { - LOG_ERR("Out of buffer space!"); - break; - } + assert_buf_len(REPLY_PDID_LEN, max_len); buf[len++] = pd->reply_id; buf[len++] = BYTE_0(pd->id.vendor_code); @@ -443,10 +437,7 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) ret = OSDP_PD_ERR_NONE; break; case REPLY_PDCAP: - if (max_len < REPLY_PDCAP_LEN) { - LOG_ERR("Out of buffer space!"); - break; - } + assert_buf_len(REPLY_PDCAP_LEN, max_len); buf[len++] = pd->reply_id; for (i = 0; i < OSDP_PD_CAP_SENTINEL; i++) { if (pd->cap[i].function_code != i) { @@ -464,29 +455,20 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) ret = OSDP_PD_ERR_NONE; break; case REPLY_LSTATR: - if (max_len < REPLY_LSTATR_LEN) { - LOG_ERR("Out of buffer space!"); - break; - } + assert_buf_len(REPLY_LSTATR_LEN, max_len); 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: - if (max_len < REPLY_RSTATR_LEN) { - LOG_ERR("Out of buffer space!"); - break; - } + assert_buf_len(REPLY_RSTATR_LEN, max_len); buf[len++] = pd->reply_id; buf[len++] = ISSET_FLAG(pd, PD_FLAG_R_TAMPER); ret = OSDP_PD_ERR_NONE; break; case REPLY_COM: - if (max_len < REPLY_COM_LEN) { - LOG_ERR("Out of buffer space!"); - break; - } + assert_buf_len(REPLY_COM_LEN, max_len); /** * 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 @@ -517,10 +499,7 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) ret = OSDP_PD_ERR_NONE; break; case REPLY_NAK: - if (max_len < REPLY_NAK_LEN) { - LOG_ERR("Fatal: insufficient space for sending NAK"); - return -1; - } + assert_buf_len(REPLY_NAK_LEN, max_len); buf[len++] = pd->reply_id; buf[len++] = pd->cmd_data[0]; ret = OSDP_PD_ERR_NONE; @@ -530,10 +509,7 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) if (smb == NULL) { break; } - if (max_len < REPLY_CCRYPT_LEN) { - LOG_ERR("Out of buffer space!"); - return -1; - } + assert_buf_len(REPLY_CCRYPT_LEN, max_len); osdp_fill_random(pd->sc.pd_random, 8); osdp_compute_session_keys(pd); osdp_compute_pd_cryptogram(pd); @@ -556,10 +532,7 @@ static int pd_build_reply(struct osdp_pd *pd, uint8_t *buf, int max_len) if (smb == NULL) { break; } - if (max_len < REPLY_RMAC_I_LEN) { - LOG_ERR("Out of buffer space!"); - return -1; - } + assert_buf_len(REPLY_RMAC_I_LEN, max_len); osdp_compute_rmac_i(pd); buf[len++] = pd->reply_id; for (i = 0; i < 16; i++) { @@ -595,10 +568,7 @@ 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); - if (max_len < REPLY_NAK_LEN) { - LOG_ERR("Fatal: insufficient space for sending NAK"); - return -1; - } + assert_buf_len(REPLY_NAK_LEN, max_len); buf[0] = REPLY_NAK; buf[1] = OSDP_PD_NAK_RECORD; len = 2;