Skip to content

mqtt: verify that CONNACK has Remaining Length set to 2#20513

Closed
bagder wants to merge 7 commits intomasterfrom
bagder/mqtt-remlen
Closed

mqtt: verify that CONNACK has Remaining Length set to 2#20513
bagder wants to merge 7 commits intomasterfrom
bagder/mqtt-remlen

Conversation

@bagder
Copy link
Member

@bagder bagder commented Feb 4, 2026

Alternative take to #20503

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request adds validation to ensure MQTT CONNACK packets have a Remaining Length field set to 2, as required by the MQTT v3.1.1 specification. This is an alternative implementation to PR #20503.

Changes:

  • Added validation in mqtt_verify_connack() to verify CONNACK Remaining Length equals 2
  • Simplified error handling flow by replacing goto statements with early returns
  • Added test infrastructure to allow test server to send malformed CONNACK packets
  • Added test case to verify client correctly rejects CONNACK with incorrect Remaining Length

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
lib/mqtt.c Adds validation that CONNACK Remaining Length is 2 and simplifies error handling with early returns
tests/server/mqttd.c Adds remlen_connack configuration option to test server for sending malformed CONNACK packets; reorders struct fields cosmetically
tests/data/test1132 New test case verifying client rejects CONNACK with Remaining Length of 3
tests/data/Makefile.am Adds test1132 to the test suite

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bagder bagder requested a review from Copilot February 4, 2026 10:12
@bagder bagder marked this pull request as ready for review February 4, 2026 10:13
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@JimFuller-RedHat JimFuller-RedHat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (and good to see new test).

@bagder
Copy link
Member Author

bagder commented Feb 4, 2026

augment review

@augmentcode
Copy link

augmentcode bot commented Feb 4, 2026

🤖 Augment PR Summary

Summary: Tightens MQTT protocol validation around CONNACK handling, with additional test coverage for malformed packets.

Changes:

  • In lib/mqtt.c, verify CONNACK Remaining Length is exactly 2 and simplify error returns.
  • Add a SUBACK Remaining Length sanity check before parsing the SUBACK payload.
  • Extend the MQTT test server (tests/server/mqttd.c) with a remlen-CONNACK knob to emit malformed CONNACK packets.
  • Add new test tests/data/test1132 and wire it into the test list to validate rejection of bad CONNACK Remaining Length.

🤖 Was this summary useful? React with 👍 or 👎

Copy link

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

@bagder
Copy link
Member Author

bagder commented Feb 4, 2026

@aisle-analyzer what do you think?

@aisle-research-bot
Copy link

aisle-research-bot bot commented Feb 4, 2026

🔒 Aisle Security Analysis

✅ We scanned this PR and did not find any security vulnerabilities.
Aisle supplements but does not replace security review.


Analyzed PR: #20513 at commit 42fbd1a

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

Development

Successfully merging this pull request may close these issues.

3 participants