Fix cluster id over-read past the controller id in Metadata response - #5545
Draft
pranav shah (prashah-confluent) wants to merge 2 commits into
Draft
Fix cluster id over-read past the controller id in Metadata response#5545pranav shah (prashah-confluent) wants to merge 2 commits into
pranav shah (prashah-confluent) wants to merge 2 commits into
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
3 tasks
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.
Summary
The Metadata response parser copied the
cluster_idstring usingstrlen(),even though the source is not nul-terminated. This causes a read past the end
of the string into the following
controller_idfield, producing a corrupted(over-long) cluster id and, in the worst case, a heap over-read past the receive
buffer.
Root cause
In
rd_kafka_parse_Metadata0()(src/rdkafka_metadata.c),cluster_idis readwith
rd_kafka_buf_read_str(), which setscluster_id.strto point into thereceive buffer with a known
cluster_id.lenbut no nul terminator. Thecopy into the internal metadata struct was done via
rd_tmpabuf_write_str(&tbuf, cluster_id.str), which internally doesstrlen(cluster_id.str) + 1.On the wire,
cluster_idis immediately followed by thecontroller_idint32.So
strlen()walks past the string into thecontroller_idbytes until ithappens to hit a
0x00. It only stops correctly by luck - when the leading(big-endian, i.e. network-order) byte of
controller_idis0x00, which is thecase for typical small broker ids. It breaks when that byte is non-zero, e.g.:
controller_id = -1(0xFFFFFFFF) — a valid "no controller" value, orcontroller_id >= 2^24.In those cases the copied cluster id is too long/corrupted, and if no
0x00appears before the end of the buffer the
strlen()reads out of bounds.Impact
mdi->cluster_id) is corrupted. This issurfaced to applications via the Admin
DescribeClusterAPI(
rd_kafka_DescribeCluster_result_cluster_id()).contain no
0x00.rd_kafka_clusterid()is not affected — the cachedrk_clusteridis built with
RD_KAFKAP_STR_DUP(length-based) and remains correct.Fix
Copy exactly
cluster_id.lenbytes and write our own nul terminator, instead ofrelying on
strlen():Testing
Compatibility
No public API/ABI changes to the client library; the fix is internal to metadata
parsing. The only added symbol is the additive mock helper
rd_kafka_mock_set_controller_id().