From 3a16e932797e84f7cb64de19c9c8a7e6f18441fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20J=C3=A4ger?= Date: Tue, 22 Sep 2026 06:14:49 +0200 Subject: [PATCH 1/6] Binary mode: Only accept record items belonging to the record When deserializing records, the ID of every item inside a record map was looked up globally with thingset_get_object_by_id() without checking that the returned object actually belongs to the record being written. The data union of a data object stores an offset into the record for record items, but a pointer to the variable itself for any other object. Passing the ID of an ordinary data item therefore caused its pointer value to be used as an offset, which was added to the record base address and written through. This turned a record update into an almost arbitrary write with a CBOR-supplied value: AddressSanitizer: SEGV on unknown address 0x103505b8 in thingset_bin_test_import_record_foreign_id The type of the foreign object was not validated either, so the width and interpretation of the write were attacker-chosen as well. The issue is reachable via UPDATE requests, thingset_import_data() and thingset_import_record() for any records object with write access. Deserializing records requires a CBOR nesting depth of 3, so it only applies to configurations with CONFIG_THINGSET_BINARY_MAX_DEPTH >= 5. That includes applications serializing records in reports, for which the help text of CONFIG_THINGSET_REPORT_RECORD_SERIALIZATION recommends raising the maximum depth. Fixed by skipping items whose parent_id does not match the record object, consistent with how unknown IDs were already handled. Record deserialization was not covered by any test before. The report tests do exercise records at a sufficient nesting depth, but only through the serializer. A thingset.protocol.nested_records scenario is therefore added to run the new tests with CONFIG_THINGSET_BINARY_MAX_DEPTH=5. Co-Authored-By: Claude Opus 5 (1M context) --- src/thingset_bin.c | 20 ++++++----- tests/protocol/src/bin.c | 70 ++++++++++++++++++++++++++++++++++++ tests/protocol/testcase.yaml | 8 +++++ 3 files changed, 89 insertions(+), 9 deletions(-) diff --git a/src/thingset_bin.c b/src/thingset_bin.c index 0a8c401..9d26eac 100644 --- a/src/thingset_bin.c +++ b/src/thingset_bin.c @@ -113,8 +113,8 @@ static int bin_serialize_simple_value(zcbor_state_t *encoder, union thingset_dat success = zcbor_int32_put(encoder, *data.i8); break; case THINGSET_TYPE_F32: - if (IS_ENABLED(CONFIG_THINGSET_ENCODE_ZERO_DECIMAL_FLOATS_AS_INTEGERS) && detail == 0) - { /* round to 0 decimals: use int */ + if (IS_ENABLED(CONFIG_THINGSET_ENCODE_ZERO_DECIMAL_FLOATS_AS_INTEGERS) && detail == 0) { + /* round to 0 decimals: use int */ success = zcbor_int32_put(encoder, lroundf(*data.f32)); } else { @@ -710,10 +710,11 @@ static int bin_deserialize_value(struct thingset_context *ts, int index = 0; do { /* using uint8_t pointer for byte-wise pointer arithmetics */ - union thingset_data_pointer data = { .u8 = array->elements.u8 + index * type_size }; + union thingset_data_pointer data = { .u8 = array->elements.u8 + + index * type_size }; - err = bin_deserialize_simple_value(ts, data, array->element_type, array->decimals, - check_only); + err = bin_deserialize_simple_value(ts, data, array->element_type, + array->decimals, check_only); if (err != 0) { break; } @@ -744,15 +745,16 @@ static int bin_deserialize_value(struct thingset_context *ts, while (zcbor_uint32_decode(ts->decoder, &id) && id < UINT16_MAX) { struct thingset_data_object *element = thingset_get_object_by_id(ts, id); - if (element == NULL) { + /* only items belonging to this record may be used */ + if (element == NULL || element->parent_id != object->id) { zcbor_any_skip(ts->decoder, NULL); continue; } union thingset_data_pointer data = { .u8 = ((uint8_t *)records->records) - + (i * records->record_size) - + element->data.offset }; + + (i * records->record_size) + + element->data.offset }; err = bin_deserialize_simple_value(ts, data, element->type, element->detail, - check_only); + check_only); } success = zcbor_map_end_decode(ts->decoder); diff --git a/tests/protocol/src/bin.c b/tests/protocol/src/bin.c index 3f51e8d..909e528 100644 --- a/tests/protocol/src/bin.c +++ b/tests/protocol/src/bin.c @@ -1083,6 +1083,76 @@ ZTEST(thingset_bin, test_import_record) records[1].f32_arr[2] = 7.89F; } +/* + * Deserializing records requires a nesting depth of 3 (outer map, list of records, record map), + * which needs CONFIG_THINGSET_BINARY_MAX_DEPTH of at least 5. With fewer states zcbor cannot + * enter the record map and the tests below would only assert that the path is unreachable. + */ +#if CONFIG_THINGSET_BINARY_MAX_DEPTH >= 5 + +/* Items of a nested record must be addressed by IDs belonging to that record. */ +ZTEST(thingset_bin, test_import_record_foreign_id) +{ + struct thingset_endpoint endpoint; + uint8_t data[THINGSET_TEST_BUF_SIZE]; + int err; + + /* 0x502 is Access/wItem, a float item that does not belong to record 0x610 */ + const char data_hex[] = + "A1 " + "19 06 10 " /* Records/Nested */ + "82 " /* two records */ + "A1 19 05 02 FA 3F800000 " /* {Access/wItem: 1.0} -> must be ignored */ + "A0"; /* {} */ + int data_len = hex2bin_spaced(data_hex, data, sizeof(data)); + + err = thingset_endpoint_by_path(&ts, &endpoint, "Records/1", strlen("Records/1")); + zassert_equal(err, 0); + + uint32_t child_u32_bak = records[1].nested[0].child_u32; + float child_f32_bak = records[1].nested[0].child_f32; + + err = thingset_import_record(&ts, data, data_len, &endpoint, THINGSET_BIN_IDS_VALUES); + zassert_equal(err, 0, "act: 0x%X", -err); + + /* the foreign item must have been skipped without touching the record */ + zassert_equal(records[1].nested[0].child_u32, child_u32_bak); + zassert_equal(records[1].nested[0].child_f32, child_f32_bak); +} + +/* Items that do belong to the nested record must still be written. */ +ZTEST(thingset_bin, test_import_record_nested) +{ + struct thingset_endpoint endpoint; + uint8_t data[THINGSET_TEST_BUF_SIZE]; + int err; + + const char data_hex[] = + "A1 " + "19 06 10 " /* Records/Nested */ + "82 " /* two records */ + "A1 19 06 11 0A " /* {Nested/wU32: 10} */ + "A1 19 06 11 14"; /* {Nested/wU32: 20} */ + int data_len = hex2bin_spaced(data_hex, data, sizeof(data)); + + err = thingset_endpoint_by_path(&ts, &endpoint, "Records/1", strlen("Records/1")); + zassert_equal(err, 0); + + uint32_t bak0 = records[1].nested[0].child_u32; + uint32_t bak1 = records[1].nested[1].child_u32; + + err = thingset_import_record(&ts, data, data_len, &endpoint, THINGSET_BIN_IDS_VALUES); + zassert_equal(err, 0, "act: 0x%X", -err); + + zassert_equal(records[1].nested[0].child_u32, 10); + zassert_equal(records[1].nested[1].child_u32, 20); + + records[1].nested[0].child_u32 = bak0; + records[1].nested[1].child_u32 = bak1; +} + +#endif /* CONFIG_THINGSET_BINARY_MAX_DEPTH >= 5 */ + static void *thingset_setup(void) { thingset_init_global(&ts); diff --git a/tests/protocol/testcase.yaml b/tests/protocol/testcase.yaml index 8b81d8a..14a7f02 100644 --- a/tests/protocol/testcase.yaml +++ b/tests/protocol/testcase.yaml @@ -20,3 +20,11 @@ tests: extra_args: EXTRA_CFLAGS=-Werror extra_configs: - CONFIG_THINGSET_OBJECT_LOOKUP_MAP=y + # deserializing records needs a nesting depth of 3, which the default max depth cannot provide + thingset.protocol.nested_records: + integration_platforms: + - native_sim + - native_sim/native/64 + extra_args: EXTRA_CFLAGS=-Werror + extra_configs: + - CONFIG_THINGSET_BINARY_MAX_DEPTH=5 From ffbdc0c915cbb861020a1db11d8e06f57fa1b014 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20J=C3=A4ger?= Date: Tue, 22 Sep 2026 08:57:34 +0200 Subject: [PATCH 2/6] Don't use the dummy check buffer for types described by a struct The first pass of an UPDATE request validates the format of each value by deserializing it into a dummy object backed by an 8 byte stack buffer. Only THINGSET_TYPE_BYTES and THINGSET_TYPE_ARRAY were excluded from that, so THINGSET_TYPE_RECORDS used the dummy buffer as well. Because the data union is read back as a struct thingset_records pointer, the deserializer interpreted that 8 byte buffer as a 32 byte struct (16 bytes on 32-bit targets). Reading record_size and num_records therefore accessed uninitialized memory past the end of the buffer, and the garbage values were used to calculate the address of the value to write: AddressSanitizer: stack-buffer-overflow in bin_deserialize_value at src/thingset_bin.c:739 Unlike the record item lookup fixed in the previous commit, this does not depend on CONFIG_THINGSET_BINARY_MAX_DEPTH: num_records is read before the record map is entered, so the out-of-bounds read happens with the default maximum depth as well. Only affects binary mode, as the text mode deserializer does not handle THINGSET_TYPE_RECORDS at all. Fixed by also using the data pointer of the actual object for records, as already done for buffers and arrays. The records object of the test data is made writable to be able to reach the UPDATE path for records at all, which was not covered before. Co-Authored-By: Claude Opus 5 (1M context) --- src/thingset_common.c | 4 +++- tests/common/data.c | 6 ++++-- tests/protocol/src/bin.c | 33 ++++++++++++++++++++++++++++++--- 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/src/thingset_common.c b/src/thingset_common.c index 1da132f..f7de2f2 100644 --- a/src/thingset_common.c +++ b/src/thingset_common.c @@ -328,10 +328,12 @@ int thingset_common_update(struct thingset_context *ts) /* * Test format of simple data types (up to 64-bit) by deserializing the value into a dummy * object of the same type. For string and byte buffers only the size of the buffers is - * checked. + * checked. Types described by a struct don't fit into the dummy buffer, so the data + * pointer of the actual object is used for them. */ uint8_t dummy_data[8]; uint8_t *data = object->type == THINGSET_TYPE_BYTES || object->type == THINGSET_TYPE_ARRAY + || object->type == THINGSET_TYPE_RECORDS ? object->data.u8 : dummy_data; struct thingset_data_object dummy_object = { diff --git a/tests/common/data.c b/tests/common/data.c index dbfacd8..8b6c0e7 100644 --- a/tests/common/data.c +++ b/tests/common/data.c @@ -266,7 +266,8 @@ THINGSET_ADD_ITEM_FLOAT(0x500, 0x503, "wMfrOnly", &access_item, 2, THINGSET_ANY_ 0); /* Records */ -THINGSET_ADD_RECORDS(THINGSET_ID_ROOT, 0x600, "Records", &records_obj, THINGSET_ANY_R, SUBSET_LIVE); +THINGSET_ADD_RECORDS(THINGSET_ID_ROOT, 0x600, "Records", &records_obj, THINGSET_ANY_RW, + SUBSET_LIVE); THINGSET_ADD_RECORD_ITEM_UINT32(0x600, 0x601, "t_s", struct test_struct, timestamp); THINGSET_ADD_RECORD_ITEM_BOOL(0x600, 0x602, "wBool", struct test_struct, b); THINGSET_ADD_RECORD_ITEM_UINT8(0x600, 0x603, "wU8", struct test_struct, u8); @@ -379,7 +380,8 @@ struct thingset_data_object data_objects[] = { 0), /* Records */ - THINGSET_RECORDS(THINGSET_ID_ROOT, 0x600, "Records", &records_obj, THINGSET_ANY_R, SUBSET_LIVE), + THINGSET_RECORDS(THINGSET_ID_ROOT, 0x600, "Records", &records_obj, THINGSET_ANY_RW, + SUBSET_LIVE), THINGSET_RECORD_ITEM_UINT32(0x600, 0x601, "t_s", struct test_struct, timestamp), THINGSET_RECORD_ITEM_BOOL(0x600, 0x602, "wBool", struct test_struct, b), THINGSET_RECORD_ITEM_UINT8(0x600, 0x603, "wU8", struct test_struct, u8), diff --git a/tests/protocol/src/bin.c b/tests/protocol/src/bin.c index 909e528..38eb4ab 100644 --- a/tests/protocol/src/bin.c +++ b/tests/protocol/src/bin.c @@ -1083,13 +1083,40 @@ ZTEST(thingset_bin, test_import_record) records[1].f32_arr[2] = 7.89F; } +/* An UPDATE of a whole records object must not treat the dummy check buffer as a record struct. */ +ZTEST(thingset_bin, test_update_records_empty) +{ + /* = {0x600: [{},{}]} */ +#if CONFIG_THINGSET_BINARY_MAX_DEPTH >= 5 + THINGSET_ASSERT_REQUEST_HEX("07 00 A1 19 0600 82 A0 A0", "84 F6 F6"); +#else + /* the record map cannot be entered with the default nesting depth */ + THINGSET_ASSERT_REQUEST_HEX("07 00 A1 19 0600 82 A0 A0", "AF F6 F6"); +#endif + + zassert_equal(records[0].timestamp, 1); + zassert_equal(records[1].timestamp, 2); +} + /* - * Deserializing records requires a nesting depth of 3 (outer map, list of records, record map), - * which needs CONFIG_THINGSET_BINARY_MAX_DEPTH of at least 5. With fewer states zcbor cannot - * enter the record map and the tests below would only assert that the path is unreachable. + * The following tests deserialize the content of a record map, which requires a nesting depth of + * 3 (outer map, list of records, record map) and thus CONFIG_THINGSET_BINARY_MAX_DEPTH >= 5. */ #if CONFIG_THINGSET_BINARY_MAX_DEPTH >= 5 +/* An UPDATE of a whole records object must write through the actual records descriptor. */ +ZTEST(thingset_bin, test_update_records_values) +{ + /* = {0x600: [{0x601: 5},{0x601: 6}]} */ + THINGSET_ASSERT_REQUEST_HEX("07 00 A1 19 0600 82 A1 19 0601 05 A1 19 0601 06", "84 F6 F6"); + + zassert_equal(records[0].timestamp, 5); + zassert_equal(records[1].timestamp, 6); + + records[0].timestamp = 1; + records[1].timestamp = 2; +} + /* Items of a nested record must be addressed by IDs belonging to that record. */ ZTEST(thingset_bin, test_import_record_foreign_id) { From 25511ec0d77dcc27d34ae618b801f1cdee339013 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20J=C3=A4ger?= Date: Wed, 23 Sep 2026 19:35:33 +0200 Subject: [PATCH 3/6] Text mode: Check buffer bounds when serializing values The text serializer tracked the write position in a local variable that was advanced by the return value of snprintf, which is the length the output would have had if the buffer were large enough. As soon as a value did not fit, the position moved past the end of the buffer and the following "size - pos" underflowed, so that subsequent calls were told that an almost unlimited amount of space was available. The array branch was worse: it added the return value of json_serialize_simple_value without checking it, and that function returns a negative ThingSet response code if the value does not fit. The position went negative and the buffer was written before its start. The subsets, functions and records branches additionally wrote quotes and commas without any bounds check at all. Serializing a record into a report calculated the position of the separating comma from ts->rsp_pos, which is an absolute offset, while the buffer pointer was already offset by the position at function entry. The comma was therefore written at roughly twice the intended offset, beyond the end of a buffer that is only slightly larger than the report. It was also redundant, because the map end already appends a comma. Fixed by adding a json_append() helper which checks the position before writing, rejects output that does not fit and passes negative positions through, so that errors propagate to the single check at the end. Also fixed along the way, because the new tests would trip over them: - The string escaping branch wrote the closing quote and comma after the loop had already moved the position past the end of the buffer. - base64_encode() was called with "size - 4" without checking that the buffer holds at least 4 bytes, underflowing the size argument. - txt_serialize_subsets() added the return code of serialize_key_value() to rsp_pos, although that function advances rsp_pos itself, and checked the remaining space only after writing. The check itself could underflow for very small buffers. - txt_serialize_report_header() compared the result of snprintf with ">" instead of ">=", accepting a truncated header, and tested the unsigned rsp_pos for being negative. - txt_serialize_finish() decremented rsp_pos without checking it for zero. The serialize functions set rsp_pos to 0 on error and thingset_report_path() and thingset_export_*() call serialize_finish() unconditionally, so the unsigned position wrapped around and the string was terminated outside of the buffer. The existing tests always passed a response buffer of 1024 bytes, which is large enough for every response of the test data. The new tests serialize into buffers from 4 bytes upwards and verify that a guard pattern behind the buffer stays intact, as address sanitizer cannot detect writes into the unused part of a buffer. Co-Authored-By: Claude Opus 5 (1M context) --- src/thingset_txt.c | 171 ++++++++++++++++++++++++++++---------- tests/protocol/src/txt.c | 45 ++++++++++ tests/report/src/report.c | 23 +++++ 3 files changed, 194 insertions(+), 45 deletions(-) diff --git a/src/thingset_txt.c b/src/thingset_txt.c index 6cae964..f7ededa 100644 --- a/src/thingset_txt.c +++ b/src/thingset_txt.c @@ -100,6 +100,36 @@ static int txt_serialize_response(struct thingset_context *ts, uint8_t code, con return 0; } +/** + * Append a formatted string at the given position of the buffer. + * + * A negative position is passed through unchanged, so that errors propagate through consecutive + * calls and only have to be checked once at the end. + * + * @returns New position or negative ThingSet reponse code in case of error + */ +static int json_append(char *buf, size_t size, int pos, const char *format, ...) +{ + va_list vargs; + + if (pos < 0) { + return pos; + } + else if ((size_t)pos >= size) { + return -THINGSET_ERR_RESPONSE_TOO_LARGE; + } + + va_start(vargs, format); + int len = vsnprintf(buf + pos, size - pos, format, vargs); + va_end(vargs); + + if (len < 0 || (size_t)len >= size - pos) { + return -THINGSET_ERR_RESPONSE_TOO_LARGE; + } + + return pos + len; +} + /** * @returns Number of serialized bytes or negative ThingSet reponse code in case of error */ @@ -156,6 +186,9 @@ static int json_serialize_simple_value(char *buf, size_t size, union thingset_da break; case THINGSET_TYPE_STRING: #if CONFIG_THINGSET_JSON_STRING_ESCAPING + if (size < 1) { + return -THINGSET_ERR_RESPONSE_TOO_LARGE; + } buf[0] = '"'; pos = 1; for (int data_pos = 0; data_pos < detail; data_pos++) { @@ -203,8 +236,14 @@ static int json_serialize_simple_value(char *buf, size_t size, union thingset_da buf[pos++] = data.str[data_pos]; } } - buf[pos++] = '"'; - buf[pos++] = ','; + if ((size_t)pos + 2 < size) { + buf[pos++] = '"'; + buf[pos++] = ','; + } + else { + /* indicate that the buffer is too small (similar to snprintf) */ + pos += 2; + } #else pos = snprintf(buf, size, "\"%s\",", data.str); #endif @@ -212,6 +251,9 @@ static int json_serialize_simple_value(char *buf, size_t size, union thingset_da #if CONFIG_THINGSET_BYTES_TYPE_SUPPORT case THINGSET_TYPE_BYTES: { size_t strlen; + if (size < 4) { + return -THINGSET_ERR_RESPONSE_TOO_LARGE; + } int err = base64_encode((uint8_t *)buf + 1, size - 4, &strlen, data.bytes->bytes, data.bytes->num_bytes); if (err == 0) { @@ -251,79 +293,88 @@ static int txt_serialize_value(struct thingset_context *ts, if (pos == -THINGSET_ERR_UNSUPPORTED_FORMAT) { /* not a simple value */ if (object->type == THINGSET_TYPE_GROUP) { - pos = snprintf(buf, size, "null,"); + pos = json_append(buf, size, 0, "null,"); } else if (object->type == THINGSET_TYPE_RECORDS) { if (IS_ENABLED(CONFIG_THINGSET_REPORT_RECORD_SERIALIZATION) && ts->rsp[0] == THINGSET_TXT_REPORT) { - pos = snprintf(buf, size, "["); - ts->rsp_pos++; + pos = json_append(buf, size, 0, "["); + if (pos < 0) { + ts->rsp_pos = 0; + return pos; + } + ts->rsp_pos += pos; + + /* the records are serialized directly into the response buffer, each of them + already terminated with a comma by the map end */ for (unsigned int i = 0; i < object->data.records->num_records; i++) { ret = thingset_common_serialize_record(ts, object, i); - pos = ts->rsp_pos; - buf[pos++] = ','; + if (ret != 0) { + ts->rsp_pos = 0; + return ret; + } } - /* thingset_common_serialize_record has already incremeneted rsp_pos, so we reset - the length of pos here, so that when we increment at the end, we are only - incrementing the small delta below */ - buf = ts->rsp + ts->rsp_pos; - pos = 0; + if (object->data.records->num_records > 0) { - pos--; /* remove trailing comma */ + ts->rsp_pos--; /* overwrite trailing comma */ } - pos += snprintf(buf + pos, size - pos, "],"); + buf = ts->rsp + ts->rsp_pos; + size = ts->rsp_size - ts->rsp_pos; + pos = json_append(buf, size, 0, "],"); } else { - pos = snprintf(buf, size, "%d,", object->data.records->num_records); + pos = json_append(buf, size, 0, "%d,", object->data.records->num_records); } } else if (object->type == THINGSET_TYPE_FN_VOID || object->type == THINGSET_TYPE_FN_I32) { - pos = snprintf(buf, size, "["); + pos = json_append(buf, size, 0, "["); for (unsigned int i = 0; i < ts->num_objects; i++) { if (ts->data_objects[i].parent_id == object->id) { - pos += snprintf(buf + pos, size - pos, "\"%s\",", ts->data_objects[i].name); + pos = json_append(buf, size, pos, "\"%s\",", ts->data_objects[i].name); } } if (pos > 1) { pos--; /* remove trailing comma */ } - pos += snprintf(buf + pos, size - pos, "],"); + pos = json_append(buf, size, pos, "],"); } else if (object->type == THINGSET_TYPE_SUBSET) { - pos = snprintf(buf, size, "["); + pos = json_append(buf, size, 0, "["); for (unsigned int i = 0; i < ts->num_objects; i++) { if (ts->data_objects[i].subsets & object->data.subset) { - buf[pos++] = '"'; + pos = json_append(buf, size, pos, "\""); + if (pos < 0) { + break; + } ret = thingset_get_path(ts, buf + pos, size - pos, &ts->data_objects[i]); if (ret <= 0) { ts->rsp_pos = 0; return ret; } - pos += ret; - buf[pos++] = '"'; - buf[pos++] = ','; + pos = json_append(buf, size, pos + ret, "\","); } } if (pos > 1) { pos--; /* remove trailing comma */ } - pos += snprintf(buf + pos, size - pos, "],"); + pos = json_append(buf, size, pos, "],"); } else if (object->type == THINGSET_TYPE_ARRAY && object->data.array != NULL) { struct thingset_array *array = object->data.array; - pos = snprintf(buf, size, "["); + pos = json_append(buf, size, 0, "["); size_t type_size = thingset_type_size(array->element_type); - for (int i = 0; i < array->num_elements; i++) { + for (int i = 0; i < array->num_elements && pos >= 0; i++) { /* using uint8_t pointer for byte-wise pointer arithmetics */ union thingset_data_pointer data = { .u8 = array->elements.u8 + i * type_size }; - pos += json_serialize_simple_value(buf + pos, size - pos, data, array->element_type, - array->decimals); + ret = json_serialize_simple_value(buf + pos, size - pos, data, array->element_type, + array->decimals); + pos = ret < 0 ? ret : pos + ret; } - if (array->num_elements > 0) { + if (pos > 1) { pos--; /* remove trailing comma */ } - pos += snprintf(buf + pos, size - pos, "],"); + pos = json_append(buf, size, pos, "],"); } else { ts->rsp_pos = 0; @@ -424,8 +475,12 @@ static int txt_serialize_name_value(struct thingset_context *ts, static void txt_serialize_finish(struct thingset_context *ts) { /* remove the trailing comma or space (in case of no payload) and terminate string */ - ts->rsp_pos--; - ts->rsp[ts->rsp_pos] = '\0'; + if (ts->rsp_pos > 0) { + ts->rsp_pos--; + } + if (ts->rsp_pos < ts->rsp_size) { + ts->rsp[ts->rsp_pos] = '\0'; + } } /** @@ -697,8 +752,13 @@ static int txt_serialize_subsets(struct thingset_context *ts, uint16_t subsets) { struct thingset_data_object *ancestors[2]; int depth = 0; + int pos; - ts->rsp[ts->rsp_pos++] = '{'; + pos = json_append(ts->rsp, ts->rsp_size, ts->rsp_pos, "{"); + if (pos < 0) { + return pos; + } + ts->rsp_pos = pos; for (unsigned int i = 0; i < ts->num_objects; i++) { if (ts->data_objects[i].subsets & subsets) { @@ -720,7 +780,11 @@ static int txt_serialize_subsets(struct thingset_context *ts, uint16_t subsets) || parent_id == 0)) /* return to root */ { ts->rsp[ts->rsp_pos - 1] = '}'; /* overwrite comma */ - ts->rsp[ts->rsp_pos++] = ','; + pos = json_append(ts->rsp, ts->rsp_size, ts->rsp_pos, ","); + if (pos < 0) { + return pos; + } + ts->rsp_pos = pos; depth--; } @@ -729,25 +793,40 @@ static int txt_serialize_subsets(struct thingset_context *ts, uint16_t subsets) struct thingset_data_object *grandparent = thingset_get_object_by_id(ts, parent->parent_id); if (grandparent != NULL) { - ts->rsp_pos += snprintf(ts->rsp + ts->rsp_pos, ts->rsp_size - ts->rsp_pos, - "\"%s\":{", grandparent->name); + pos = json_append(ts->rsp, ts->rsp_size, ts->rsp_pos, "\"%s\":{", + grandparent->name); + if (pos < 0) { + return pos; + } + ts->rsp_pos = pos; ancestors[depth++] = grandparent; } } - ts->rsp_pos += snprintf(ts->rsp + ts->rsp_pos, ts->rsp_size - ts->rsp_pos, - "\"%s\":{", parent->name); + pos = json_append(ts->rsp, ts->rsp_size, ts->rsp_pos, "\"%s\":{", parent->name); + if (pos < 0) { + return pos; + } + ts->rsp_pos = pos; ancestors[depth++] = parent; } else if (depth > 0 && parent_id != ancestors[depth - 1]->id) { if (parent != NULL) { - ts->rsp_pos += snprintf(ts->rsp + ts->rsp_pos, ts->rsp_size - ts->rsp_pos, - "\"%s\":{", parent->name); + pos = json_append(ts->rsp, ts->rsp_size, ts->rsp_pos, "\"%s\":{", parent->name); + if (pos < 0) { + return pos; + } + ts->rsp_pos = pos; ancestors[depth++] = parent; } } - ts->rsp_pos += ts->api->serialize_key_value(ts, &ts->data_objects[i]); + + /* serialize_key_value advances rsp_pos itself */ + int err = ts->api->serialize_key_value(ts, &ts->data_objects[i]); + if (err != 0) { + return err; + } } - if (ts->rsp_pos >= ts->rsp_size - 1 - depth) { + if (ts->rsp_pos + 1 + depth >= ts->rsp_size) { return -THINGSET_ERR_RESPONSE_TOO_LARGE; } } @@ -766,11 +845,13 @@ static int txt_serialize_subsets(struct thingset_context *ts, uint16_t subsets) static int txt_serialize_report_header(struct thingset_context *ts, const char *path) { - ts->rsp_pos = snprintf(ts->rsp, ts->rsp_size, "#%s ", path); - if (ts->rsp_pos < 0 || ts->rsp_pos > ts->rsp_size) { - return -THINGSET_ERR_RESPONSE_TOO_LARGE; + int pos = json_append(ts->rsp, ts->rsp_size, 0, "#%s ", path); + if (pos < 0) { + ts->rsp_pos = 0; + return pos; } else { + ts->rsp_pos = pos; return 0; } } diff --git a/tests/protocol/src/txt.c b/tests/protocol/src/txt.c index 00b4d77..923a376 100644 --- a/tests/protocol/src/txt.c +++ b/tests/protocol/src/txt.c @@ -294,6 +294,51 @@ ZTEST(thingset_txt, test_update_escaped_string) #endif /* CONFIG_THINGSET_JSON_STRING_ESCAPING */ +/* + * Process the request with a response buffer of the given size, which is followed by a guard + * pattern to detect writes beyond the end of the buffer. + */ +static void assert_response_within_buffer(const char *req, size_t rsp_size) +{ + uint8_t buf[THINGSET_TEST_BUF_SIZE]; + const uint8_t guard = 0xAA; + + memset(buf, guard, sizeof(buf)); + + int len = thingset_process_message(&ts, req, strlen(req), buf, rsp_size); + + for (size_t i = rsp_size; i < sizeof(buf); i++) { + if (buf[i] != guard) { + zassert_unreachable("req \"%s\" with buffer size %u wrote %u bytes beyond the end", req, + (unsigned int)rsp_size, (unsigned int)(i - rsp_size + 1)); + } + } + + zassert_true(len <= (int)rsp_size, "req \"%s\" returned %d for a buffer size of %u", req, len, + (unsigned int)rsp_size); +} + +/* Serializing into a buffer that is too small must not write beyond the end of the buffer. */ +ZTEST(thingset_txt, test_serialize_buffer_too_small) +{ + static const char *const reqs[] = { + "?Arrays/wF32", /* array */ + "?mLive", /* subset of paths */ + "?Exec", /* group containing functions */ + "?Types/wString", /* string */ + "?Types", /* group of simple values */ + "?Records/1", /* record */ + "?", /* whole node */ + }; + + for (unsigned int i = 0; i < ARRAY_SIZE(reqs); i++) { + /* 4 bytes is the minimum size accepted by thingset_process_message() */ + for (size_t size = 4; size <= 128; size++) { + assert_response_within_buffer(reqs[i], size); + } + } +} + ZTEST(thingset_txt, test_update_timestamp_zero) { THINGSET_ASSERT_REQUEST_TXT("= {\"t_s\":0}", ":84"); diff --git a/tests/report/src/report.c b/tests/report/src/report.c index 3715303..1265368 100644 --- a/tests/report/src/report.c +++ b/tests/report/src/report.c @@ -140,6 +140,29 @@ ZTEST(thingset_report, test_report_txt) THINGSET_ASSERT_REPORT_TXT("mLive", rpt_exp, strlen(rpt_exp)); } +/* Generating a report into a buffer that is too small must not write beyond the end. */ +ZTEST(thingset_report, test_report_buffer_too_small) +{ + uint8_t buf[THINGSET_TEST_BUF_SIZE]; + const uint8_t guard = 0xAA; + + for (size_t rpt_size = 1; rpt_size <= 256; rpt_size++) { + memset(buf, guard, sizeof(buf)); + + int len = thingset_report_path(&ts, buf, rpt_size, "mLive", THINGSET_TXT_NAMES_VALUES); + + for (size_t i = rpt_size; i < sizeof(buf); i++) { + if (buf[i] != guard) { + zassert_unreachable("buffer size %u: wrote %u bytes beyond the end", + (unsigned int)rpt_size, (unsigned int)(i - rpt_size + 1)); + } + } + + zassert_true(len <= (int)rpt_size, "buffer size %u: returned %d", (unsigned int)rpt_size, + len); + } +} + static void *thingset_setup(void) { thingset_init_global(&ts); From 59c188fda5cad105efce9adc9eecdb7a80474e63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20J=C3=A4ger?= Date: Wed, 23 Sep 2026 19:45:36 +0200 Subject: [PATCH 4/6] Release context lock when rejecting an unsupported export format thingset_export_subsets() returned directly from the default case of the format switch, without releasing the context lock taken at the beginning of the function. A single call with an unsupported format therefore left the context locked forever: every following call had to wait for the full THINGSET_CONTEXT_LOCK_TIMEOUT_MS and then failed with an internal server error, which also affects processing of incoming messages. Fixed by jumping to the existing k_sem_give() at the end of the function, as thingset_export_item() already does. Co-Authored-By: Claude Opus 5 (1M context) --- src/thingset.c | 4 +++- tests/protocol/src/common.c | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/thingset.c b/src/thingset.c index dad32ce..9246611 100644 --- a/src/thingset.c +++ b/src/thingset.c @@ -194,7 +194,8 @@ int thingset_export_subsets(struct thingset_context *ts, uint8_t *buf, size_t bu thingset_bin_setup(ts, 0); break; default: - return -THINGSET_ERR_NOT_IMPLEMENTED; + ret = -THINGSET_ERR_NOT_IMPLEMENTED; + goto out; } ret = ts->api->serialize_subsets(ts, subsets); @@ -205,6 +206,7 @@ int thingset_export_subsets(struct thingset_context *ts, uint8_t *buf, size_t bu ret = ts->rsp_pos; } +out: k_sem_give(&ts->lock); return ret; diff --git a/tests/protocol/src/common.c b/tests/protocol/src/common.c index c26dfa6..3d56c49 100644 --- a/tests/protocol/src/common.c +++ b/tests/protocol/src/common.c @@ -10,6 +10,7 @@ #include "../../src/thingset_internal.h" +#include "data.h" #include "test_utils.h" static struct thingset_context ts; @@ -114,6 +115,20 @@ ZTEST(thingset_common, test_serialize_path) zassert_mem_equal(buf, "Nested/Obj2/rItem1_V", len); } +/* Rejecting an unsupported format must not leave the context locked. */ +ZTEST(thingset_common, test_export_subsets_unsupported_format) +{ + uint8_t buf[THINGSET_TEST_BUF_SIZE]; + int err; + + err = thingset_export_subsets(&ts, buf, sizeof(buf), SUBSET_LIVE, THINGSET_BIN_IDS_ONLY); + zassert_equal(err, -THINGSET_ERR_NOT_IMPLEMENTED, "act: 0x%X", -err); + + /* a leaked lock would make this time out and fail with an internal server error */ + err = thingset_export_subsets(&ts, buf, sizeof(buf), SUBSET_LIVE, THINGSET_BIN_IDS_VALUES); + zassert_true(err > 0, "act: 0x%X", -err); +} + static void *thingset_setup(void) { thingset_init_global(&ts); From f6f88f38a969c8af8b754f3079a35dddcae6d1c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20J=C3=A4ger?= Date: Wed, 23 Sep 2026 20:07:28 +0200 Subject: [PATCH 5/6] Limit path parsing to the declared path length thingset_get_object_by_path() used strchr() to find the next separator, although the path is not null-terminated: it points directly into the received message for both binary and text mode, and into a zero-copy string for CREATE and DELETE requests. The result was only checked against the end of the path afterwards, so the search itself already read past the end of the message buffer and only stopped at the next separator or zero byte that happened to follow it: AddressSanitizer: global-buffer-overflow READ of size 6 ... 0 bytes after global variable of size 11 in thingset_get_object_by_path The comment about strtoul() and atoi() not working on strings that are not null-terminated was already there for the index parsing below. Fixed by using memchr() limited to the remaining length. The explicit check for the found separator being outside of the path can be dropped, as memchr() cannot return such a position. A path length of zero is rejected as well, because the first character was dereferenced before the length was taken into account. This can be reached through the metadata endpoint with an empty name. Co-Authored-By: Claude Opus 5 (1M context) --- src/thingset.c | 13 +++++++++++-- tests/protocol/src/common.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/thingset.c b/src/thingset.c index 9246611..cc5f0cc 100644 --- a/src/thingset.c +++ b/src/thingset.c @@ -13,6 +13,7 @@ #include #include +#include LOG_MODULE_REGISTER(thingset, CONFIG_THINGSET_LOG_LEVEL); @@ -623,10 +624,18 @@ struct thingset_data_object *thingset_get_object_by_path(struct thingset_context const char *end; uint16_t parent = 0; + if (path_len == 0) { + return NULL; + } + /* maximum depth of 10 assumed */ for (int i = 0; i < 10; i++) { - end = strchr(start, '/'); - if (end == NULL || end >= path + path_len) { + /* + * The path may point directly into a received message and is not null-terminated, so the + * search for the separator has to be limited to the remaining length. + */ + end = memchr(start, '/', path + path_len - start); + if (end == NULL) { /* reached at the end of the path */ if (object != NULL && object->type == THINGSET_TYPE_RECORDS && *start >= '0' && *start <= '9') diff --git a/tests/protocol/src/common.c b/tests/protocol/src/common.c index 3d56c49..5599b96 100644 --- a/tests/protocol/src/common.c +++ b/tests/protocol/src/common.c @@ -115,6 +115,34 @@ ZTEST(thingset_common, test_serialize_path) zassert_mem_equal(buf, "Nested/Obj2/rItem1_V", len); } +/* Paths are not null-terminated, so parsing must not read beyond the given length. */ +static char path_without_termination[] = { 'T', 'y', 'p', 'e', 's', '/', 'w', 'B', 'o', 'o', 'l' }; + +ZTEST(thingset_common, test_endpoint_from_path_without_termination) +{ + struct thingset_endpoint endpoint; + int err; + + err = thingset_endpoint_by_path(&ts, &endpoint, path_without_termination, + sizeof(path_without_termination)); + zassert_equal(err, 0, "act: 0x%X", -err); + zassert_equal(endpoint.object->id, 0x201); + + /* a shorter length must select the group instead of the item */ + err = thingset_endpoint_by_path(&ts, &endpoint, path_without_termination, 5); + zassert_equal(err, 0, "act: 0x%X", -err); + zassert_equal(endpoint.object->id, 0x200); + + /* an empty path selects the root object */ + err = thingset_endpoint_by_path(&ts, &endpoint, path_without_termination, 0); + zassert_equal(err, 0, "act: 0x%X", -err); + zassert_equal(endpoint.object->id, 0); + + /* an empty path must not be dereferenced when looked up directly */ + int index; + zassert_is_null(thingset_get_object_by_path(&ts, path_without_termination, 0, &index)); +} + /* Rejecting an unsupported format must not leave the context locked. */ ZTEST(thingset_common, test_export_subsets_unsupported_format) { From f0f576936ece4b6d431bf89e0cb51b6602696255 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20J=C3=A4ger?= Date: Wed, 23 Sep 2026 20:20:28 +0200 Subject: [PATCH 6/6] Validate record indices before using them as an offset The record index of a path was parsed digit by digit without any limit, so a path like "Records/99999999999999999999" overflowed the signed int used for the index, which is undefined behaviour and can result in a negative index. thingset_common_serialize_record() only checked the index against the number of records, and the comparison is signed, so a negative index passed the check and was multiplied with the record size to get a large negative offset. Binary mode already rejects negative indices while parsing the endpoint, text mode did not. thingset_import_record() did not check the index at all. Since the endpoint is provided by the caller, an index taken from a received path was used to calculate the write offset directly. The type of the endpoint object was not verified either, so the data union of an object that is not a records object would have been used as a records descriptor. The same function also ignored THINGSET_DETAIL_DYN_RECORDS. Dynamic records keep only a single record in memory, with num_records being the number of virtually available records, so importing into "DynRecords/7" wrote seven records beyond the end of that buffer: AddressSanitizer: global-buffer-overflow in txt_deserialize_simple_value Fixed by limiting the parsed index to the range of uint16_t, which is the type of num_records, by rejecting negative indices where records are serialized, and by validating the endpoint and honouring dynamic records where they are imported. The dynamic record variable of the test data is exported to be able to verify that the import writes to the correct location. Co-Authored-By: Claude Opus 5 (1M context) --- src/thingset.c | 22 +++++++++-- src/thingset_common.c | 2 +- tests/common/data.c | 2 +- tests/common/data.h | 2 + tests/protocol/src/common.c | 79 +++++++++++++++++++++++++++++++++++++ 5 files changed, 102 insertions(+), 5 deletions(-) diff --git a/src/thingset.c b/src/thingset.c index cc5f0cc..417453b 100644 --- a/src/thingset.c +++ b/src/thingset.c @@ -418,6 +418,14 @@ int thingset_import_record(struct thingset_context *ts, const uint8_t *data, siz ts->endpoint = *endpoint; + if (ts->endpoint.object == NULL || ts->endpoint.object->type != THINGSET_TYPE_RECORDS + || ts->endpoint.index < 0 + || ts->endpoint.index >= ts->endpoint.object->data.records->num_records) + { + err = -THINGSET_ERR_NOT_FOUND; + goto out; + } + switch (format) { #ifdef CONFIG_THINGSET_TEXT_MODE case THINGSET_TXT_NAMES_VALUES: @@ -442,6 +450,12 @@ int thingset_import_record(struct thingset_context *ts, const uint8_t *data, siz goto out; } + /* dynamic records keep the data of only one record in memory */ + struct thingset_records *records = ts->endpoint.object->data.records; + size_t record_offset = ts->endpoint.object->detail == THINGSET_DETAIL_DYN_RECORDS + ? 0 + : ts->endpoint.index * records->record_size; + const struct thingset_data_object *item; while ((err = ts->api->deserialize_child(ts, &item)) != -THINGSET_ERR_DESERIALIZATION_FINISHED) { @@ -454,9 +468,7 @@ int thingset_import_record(struct thingset_context *ts, const uint8_t *data, siz goto out; } - struct thingset_records *records = ts->endpoint.object->data.records; - uint8_t *record_ptr = - (uint8_t *)records->records + ts->endpoint.index * records->record_size; + uint8_t *record_ptr = (uint8_t *)records->records + record_offset; err = thingset_common_prepare_record_element(ts, item, record_ptr, deserialize_value_callback); @@ -650,6 +662,10 @@ struct thingset_data_object *thingset_get_object_by_path(struct thingset_context do { if (*start >= '0' && *start <= '9') { *index = (*index) * 10 + *start - '0'; + if (*index > UINT16_MAX) { + /* record indices are limited to the range of uint16_t */ + return NULL; + } start++; } else { diff --git a/src/thingset_common.c b/src/thingset_common.c index f7de2f2..bdcf410 100644 --- a/src/thingset_common.c +++ b/src/thingset_common.c @@ -111,7 +111,7 @@ int thingset_common_serialize_record(struct thingset_context *ts, size_t record_offset; int err; - if (record_index >= records->num_records) { + if (record_index < 0 || record_index >= records->num_records) { return -THINGSET_ERR_NOT_FOUND; } diff --git a/tests/common/data.c b/tests/common/data.c index 8b6c0e7..00f6e78 100644 --- a/tests/common/data.c +++ b/tests/common/data.c @@ -158,7 +158,7 @@ THINGSET_DEFINE_RECORD_FLOAT_ARRAY(f32_array_record, 1, struct test_struct, f32_ THINGSET_DEFINE_RECORD_RECORDS(nested_records_obj, struct test_struct, nested); /* Dynamic record */ -static struct test_dyn_struct dyn_records = { +struct test_dyn_struct dyn_records = { .index = 0, }; diff --git a/tests/common/data.h b/tests/common/data.h index 1b90c6a..c1adf31 100644 --- a/tests/common/data.h +++ b/tests/common/data.h @@ -105,4 +105,6 @@ struct test_dyn_struct uint32_t index; }; +extern struct test_dyn_struct dyn_records; + #endif /* TESTS_COMMON_DATA_H_ */ diff --git a/tests/protocol/src/common.c b/tests/protocol/src/common.c index 5599b96..8ca02ea 100644 --- a/tests/protocol/src/common.c +++ b/tests/protocol/src/common.c @@ -143,6 +143,85 @@ ZTEST(thingset_common, test_endpoint_from_path_without_termination) zassert_is_null(thingset_get_object_by_path(&ts, path_without_termination, 0, &index)); } +/* A record index in a path must be limited to the range of valid indices. */ +ZTEST(thingset_common, test_endpoint_from_path_record_index) +{ + struct thingset_endpoint endpoint; + char *path; + int err; + + path = "Records/1"; + err = thingset_endpoint_by_path(&ts, &endpoint, path, strlen(path)); + zassert_equal(err, 0, "act: 0x%X", -err); + zassert_equal(endpoint.index, 1); + + path = "Records/65535"; + err = thingset_endpoint_by_path(&ts, &endpoint, path, strlen(path)); + zassert_equal(err, 0, "act: 0x%X", -err); + zassert_equal(endpoint.index, 65535); + + /* an index above the range of uint16_t must be rejected instead of overflowing */ + path = "Records/65536"; + err = thingset_endpoint_by_path(&ts, &endpoint, path, strlen(path)); + zassert_equal(err, -THINGSET_ERR_NOT_FOUND, "act: 0x%X", -err); + + path = "Records/99999999999999999999"; + err = thingset_endpoint_by_path(&ts, &endpoint, path, strlen(path)); + zassert_equal(err, -THINGSET_ERR_NOT_FOUND, "act: 0x%X", -err); +} + +/* Importing a record must reject indices outside of the records array. */ +ZTEST(thingset_common, test_import_record_invalid_index) +{ + struct thingset_endpoint endpoint; + uint8_t data[THINGSET_TEST_BUF_SIZE]; + int err; + + /* {0x602: false} */ + int data_len = hex2bin_spaced("A1 19 06 02 F4", data, sizeof(data)); + + err = thingset_endpoint_by_path(&ts, &endpoint, "Records/1", strlen("Records/1")); + zassert_equal(err, 0, "act: 0x%X", -err); + + /* only two records exist */ + endpoint.index = 2; + err = thingset_import_record(&ts, data, data_len, &endpoint, THINGSET_BIN_IDS_VALUES); + zassert_equal(err, -THINGSET_ERR_NOT_FOUND, "act: 0x%X", -err); + + endpoint.index = THINGSET_ENDPOINT_INDEX_NONE; + err = thingset_import_record(&ts, data, data_len, &endpoint, THINGSET_BIN_IDS_VALUES); + zassert_equal(err, -THINGSET_ERR_NOT_FOUND, "act: 0x%X", -err); + + /* an endpoint that is not a records object must be rejected as well */ + err = thingset_endpoint_by_path(&ts, &endpoint, "Types", strlen("Types")); + zassert_equal(err, 0, "act: 0x%X", -err); + endpoint.index = 0; + err = thingset_import_record(&ts, data, data_len, &endpoint, THINGSET_BIN_IDS_VALUES); + zassert_equal(err, -THINGSET_ERR_NOT_FOUND, "act: 0x%X", -err); +} + +/* Dynamic records keep only one record in memory, so the index must not be used as an offset. */ +ZTEST(thingset_common, test_import_dyn_record) +{ + struct thingset_endpoint endpoint; + uint8_t data[THINGSET_TEST_BUF_SIZE]; + int err; + + /* {0x681: 42} */ + int data_len = hex2bin_spaced("A1 19 06 81 18 2A", data, sizeof(data)); + + err = thingset_endpoint_by_path(&ts, &endpoint, "DynRecords/7", strlen("DynRecords/7")); + zassert_equal(err, 0, "act: 0x%X", -err); + zassert_equal(endpoint.index, 7); + + err = thingset_import_record(&ts, data, data_len, &endpoint, THINGSET_BIN_IDS_VALUES); + zassert_equal(err, 0, "act: 0x%X", -err); + + zassert_equal(dyn_records.index, 42); + + dyn_records.index = 0; +} + /* Rejecting an unsupported format must not leave the context locked. */ ZTEST(thingset_common, test_export_subsets_unsupported_format) {