Repository navigation
Several bugfixes found by Claude - #42
Open
martinjaeger wants to merge 6 commits into
Open
martinjaeger wants to merge 6 commits into
martinjaeger wants to merge 6 commits into
Conversation
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
martinjaeger
force-pushed
the
claude-bugfixes
branch
from
November 2, 2026 10:46
61ff879 to
f0f5769
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR contains a couple of bugfixes identified with Claude Opus 5.