adafruit-mirror/zephyr
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 9

mgmt/osdp: Replace __ASSERT() with an explicit if

Browse source 
Commit c7fec71193 ("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: c7fec71193.
Signed-off-by: Siddharth Chandrasekaran <sidcha.dev@gmail.com>
This commit is contained in:
Siddharth Chandrasekaran 2023-07-15 03:32:29 +02:00 committed by Carles Cufí
parent d2bd778bcc
commit 061a87aff8
2 changed files with 99 additions and 35 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

73
subsys/mgmt/osdp/src/osdp_cp.c
View file

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

61
subsys/mgmt/osdp/src/osdp_pd.c
View file

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