Skip to content

in_collectd: validate value part length before reading count#11951

Open
saddamr3e wants to merge 1 commit into
fluent:masterfrom
saddamr3e:collectd-value-part-length
Open

in_collectd: validate value part length before reading count#11951
saddamr3e wants to merge 1 commit into
fluent:masterfrom
saddamr3e:collectd-value-part-length

Conversation

@saddamr3e

@saddamr3e saddamr3e commented Jun 16, 2026

Copy link
Copy Markdown
  1. netprot_pack_value() reads the 2-byte value count via be16read(ptr) without checking the part actually carries those bytes.
  2. a PART_VALUE with a declared length of 4 or 5 (payload size 0 or 1) makes that read run past the part, and past the received datagram when it is the last part, reachable from a single collectd UDP packet.
  3. the time/interval parts already gate their 8-byte read with size < sizeof(uint64_t) in netprot_to_msgpack(); the value part had no equivalent.

Added the matching length check before reading the count.


Testing

  • Example configuration file for the change
  • [N/A] Debug log output from testing the change
  • Attached AddressSanitizer output that shows the out-of-bounds read before the fix and a clean run after

Config used to reach the parser:

[INPUT]
    name    collectd
    listen  0.0.0.0
    port    25826

Trigger: a collectd UDP datagram ending with a PART_VALUE part of declared length 4 (00 06 00 04), preceded by any PART_TYPE part. Exercising netprot_to_msgpack() on a buffer sized to the datagram under AddressSanitizer:

Before:

ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 2 ... 0 bytes after 10-byte region
  #0 netprot_pack_value  (count = be16read(ptr))

After: the short value part is rejected with data truncated and the read stays in bounds.

  • [N/A] Run local packaging test
  • [N/A] Set ok-package-test label

Documentation

  • [N/A] Documentation required for this feature

Backporting

  • Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

Summary by CodeRabbit

  • Bug Fixes
    • Improved validation of network protocol data to properly detect and handle truncated payloads, preventing potential system crashes or errors when processing incomplete network data.

Signed-off-by: saddamr3e <saddamr3e@gmail.com>
@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 154303c8-4c12-48d1-8ae6-00b99f464467

📥 Commits

Reviewing files that changed from the base of the PR and between 4ddcb11 and 9a0d792.

📒 Files selected for processing (1)
  • plugins/in_collectd/netprot.c

📝 Walkthrough

Walkthrough

A defensive bounds check is added in netprot_pack_value() in plugins/in_collectd/netprot.c. Before reading the 2-byte value-count field from the PART_VALUE payload, the function now validates that size >= sizeof(uint16_t). If the buffer is too small, it logs a "data truncated" error and returns -1.

Changes

Bounds check for PART_VALUE count field

Layer / File(s) Summary
Size validation before reading value-count header
plugins/in_collectd/netprot.c
Adds a size < sizeof(uint16_t) guard in netprot_pack_value(); on truncation, logs an error and returns -1 instead of performing an out-of-bounds read.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested labels

backport to v4.2.x

Suggested reviewers

  • cosmo0920

Poem

🐇 A tiny buffer, a dangerous read,
The rabbit hopped in to plant a seed.
Six lines of caution, a check so small,
No truncated data shall cause a fall.
Bounds checked, log written, return minus one—
The collectd plugin is safely done! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title directly and clearly describes the main change: adding a validation check for the value part length before reading the count field, which matches the core purpose of addressing the buffer overflow vulnerability.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant