in_collectd: reject parts with length < 4 and fix null-terminator OOB#11849
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR hardens the collectd protocol parser in netprot_to_msgpack() by adding early validation for part lengths and replacing unsafe NUL-termination checks with in-bounds checks for string parts. ChangesParsing security and bounds fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@TristanInSec would you please sign off the commits ? (DCO error / git commit -s ...) |
The collectd binary protocol parser enters an infinite loop when it receives a part with part_len=0: the loop counters are not advanced and the while condition remains true forever. A single 4-byte UDP packet can permanently hang the worker thread. Additionally, the null-terminator check for string parts reads one byte past the end of the received data (ptr[size] where size = part_len - 4 accesses buf[part_len], which is past the buffer when the last part fills exactly to the end). Fix both by rejecting parts with part_len < 4 (minimum valid part is 4 bytes: 2 type + 2 length) and checking ptr[size - 1] instead of ptr[size] for the null terminator. Signed-off-by: Tristan <tristan@talencesecurity.com>
TristanInSec
force-pushed
the
fix/collectd-infinite-loop
branch
from
0b23d1d to
dab2e75
Compare
|
Hi @edsiper @cosmo0920 -- just checking in on this batch (#11849-#11856). All 7 have DCO passing and Cosmo0920's approval. Is there anything else needed from my side to get these merged? Thanks! |
3 participants
The collectd binary protocol parser enters an infinite loop when it
receives a part with part_len=0: the loop counters are not advanced
and the while condition remains true forever. A single 4-byte UDP
packet can permanently hang the worker thread.
Additionally, the null-terminator check for string parts reads one
byte past the end of the received data (ptr[size] where size =
part_len - 4 accesses buf[part_len], which is past the buffer when
the last part fills exactly to the end).
Fix both by rejecting parts with part_len < 4 (minimum valid part
is 4 bytes: 2 type + 2 length) and checking ptr[size - 1] instead
of ptr[size] for the null terminator.
Summary by CodeRabbit