Skip to content

in_mqtt: fix OOB read from hardcoded remaining length overhead#11853

Open
TristanInSec wants to merge 1 commit into
fluent:masterfrom
TristanInSec:fix/mqtt-remaining-length-oob
Open

in_mqtt: fix OOB read from hardcoded remaining length overhead#11853
TristanInSec wants to merge 1 commit into
fluent:masterfrom
TristanInSec:fix/mqtt-remaining-length-oob

Conversation

@TristanInSec
Copy link
Copy Markdown

@TristanInSec TristanInSec commented May 26, 2026

The MQTT packet parser used hardcoded +2/-2 to account for the fixed
header size (1 type byte + 1 remaining-length byte). This is only
correct when the remaining length fits in a single byte (0-127).
For remaining lengths 128+, the encoding uses 2-4 bytes, making the
actual overhead 3-5 bytes. Replace the constant with the computed
header size (buf_pos - pos + 1) so the bounds checks account for the
actual number of remaining-length bytes consumed.

Summary by CodeRabbit

  • Bug Fixes
    • Improved MQTT message parsing reliability by fixing buffer-availability checks used during length decoding.
    • Prevents premature parsing of incomplete packets and ensures the parser correctly requests more data for partial messages, reducing errors and stream-processing interruptions.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: af3492c7-05af-436b-8078-80c8f3e42eb7

📥 Commits

Reviewing files that changed from the base of the PR and between a25128a and 66daf45.

📒 Files selected for processing (1)
  • plugins/in_mqtt/mqtt_prot.c
🚧 Files skipped from review as they are similar to previous changes (1)
  • plugins/in_mqtt/mqtt_prot.c

📝 Walkthrough

Walkthrough

The MQTT protocol parser refines remaining-length decoding by replacing fixed buffer-size assumptions with dynamic offset-based checks; it rewinds to the packet start and returns MQTT_MORE when the calculated remaining length exceeds currently buffered bytes.

Changes

MQTT remaining-length parsing fix

Layer / File(s) Summary
MQTT remaining-length parsing boundary checks
plugins/in_mqtt/mqtt_prot.c
Boundary checks during MQTT remaining-length decoding now use calculations based on the parser's current offset position, replacing fixed-length comparisons and ensuring MQTT_MORE is returned when remaining length exceeds buffered data.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • fluent/fluent-bit#11580: Both PRs adjust MQTT buffer-boundary handling in plugins/in_mqtt/mqtt_prot.c—the main PR fixes mqtt_prot_parser “remaining length” decoding to request more data correctly, and the retrieved PR adds early truncation checks in mqtt_handle_publish before reading topic/QoS/payload.

Suggested labels

backport to v4.2.x

Suggested reviewers

  • cosmo0920

Poem

🐰 The buffer grows byte by byte, so true,
I hop through offsets, checking what’s due.
No guesses now, the parser takes care,
Rewinds when it's short and reads once there's spare.
A rabbit nods: precise bytes, through and through!

🚥 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 title clearly and accurately describes the primary change: fixing an out-of-bounds read vulnerability caused by hardcoded remaining length overhead assumptions in the MQTT parser.
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.

@edsiper
Copy link
Copy Markdown
Member

edsiper commented May 27, 2026

@TristanInSec would you please sign off the commits ? (DCO error / git commit -s ...)

The MQTT packet parser used hardcoded +2/-2 to account for the fixed
header size (1 type byte + 1 remaining-length byte). This is only
correct when the remaining length fits in a single byte (0-127).
For remaining lengths 128+, the encoding uses 2-4 bytes, making the
actual overhead 3-5 bytes. Replace the constant with the computed
header size (buf_pos - pos + 1) so the bounds checks account for the
actual number of remaining-length bytes consumed.

Signed-off-by: Tristan <tristan@talencesecurity.com>
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.

3 participants