Skip to content

Commit fc9c0c3

Browse files
noahpnordicjm
authored andcommitted
net: lib: nrf_cloud: mqtt: Fix integer underflow in topic prefix parsing
Fixed a signed integer underflow in nct_set_topic_prefix() where a malformed topic prefix (e.g. "stage/" with no tenant segment) caused len to compute as -1. Due to implicit signed-to-unsigned conversion, the bounds check was bypassed and memcpy copied 63 bytes of memory beyond the end of the input string into the tenant[] buffer. Fixed by using size_t for all length variables and validating that the topic prefix contains a non-empty tenant segment before computing tenant_len. Ref: SI-539 Signed-off-by: Noah Pendleton <noah.pendleton@nordicsemi.no>
1 parent 7067226 commit fc9c0c3

1 file changed

Lines changed: 32 additions & 22 deletions

File tree

subsys/net/lib/nrf_cloud/mqtt/src/nrf_cloud_transport.c

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -277,29 +277,39 @@ int nct_tenant_id_get(char *cur_tenant, const int cur_tenant_len)
277277

278278
void nct_set_topic_prefix(const char *topic_prefix)
279279
{
280-
char *end_of_stage = strchr(topic_prefix, '/');
281-
int len;
282-
283-
if (end_of_stage) {
284-
len = end_of_stage - topic_prefix;
285-
if (len >= sizeof(stage)) {
286-
LOG_WRN("Truncating copy of stage string length "
287-
"from %d to %zd",
288-
len, sizeof(stage));
289-
len = sizeof(stage) - 1;
290-
}
291-
memcpy(stage, topic_prefix, len);
292-
stage[len] = '\0';
293-
len = strlen(topic_prefix) - len - 2; /* skip both / */
294-
if (len >= sizeof(tenant)) {
295-
LOG_WRN("Truncating copy of tenant id string length "
296-
"from %d to %zd",
297-
len, sizeof(tenant));
298-
len = sizeof(tenant) - 1;
299-
}
300-
memcpy(tenant, end_of_stage + 1, len);
301-
tenant[len] = '\0';
280+
const char *end_of_stage = strchr(topic_prefix, '/');
281+
size_t total_len;
282+
size_t stage_len;
283+
size_t tenant_len;
284+
285+
if (!end_of_stage) {
286+
return;
287+
}
288+
289+
total_len = strlen(topic_prefix);
290+
stage_len = (size_t)(end_of_stage - topic_prefix);
291+
292+
if (total_len < stage_len + 2) {
293+
LOG_ERR("Malformed topic prefix");
294+
return;
295+
}
296+
297+
if (stage_len >= sizeof(stage)) {
298+
LOG_WRN("Truncating copy of stage string length from %zu to %zu",
299+
stage_len, sizeof(stage) - 1);
300+
stage_len = sizeof(stage) - 1;
301+
}
302+
memcpy(stage, topic_prefix, stage_len);
303+
stage[stage_len] = '\0';
304+
305+
tenant_len = total_len - stage_len - 2;
306+
if (tenant_len >= sizeof(tenant)) {
307+
LOG_WRN("Truncating copy of tenant id string length from %zu to %zu",
308+
tenant_len, sizeof(tenant) - 1);
309+
tenant_len = sizeof(tenant) - 1;
302310
}
311+
memcpy(tenant, end_of_stage + 1, tenant_len);
312+
tenant[tenant_len] = '\0';
303313
}
304314

305315
static int allocate_and_format_topic(struct mqtt_utf8 *const topic,

0 commit comments

Comments
 (0)