Add validation that the remaining length is less than 2^28

[63n0]

Fixed it to throw a ValueError if the remaining length is too large, and removed the FIXME comment.

Closes: #900

[JamesParrott]

Thanks for this PR. But the equivalent PR in the Go Paho.mqtt library that inspired this, also fixed the decoding code for remaining length as well as the encoding code.

As I understand it, Client._pack_remaining_length only gets called when publishing (i.e. by the attacker). While it's fine to clear things up, make behaviour explicit, and comply with the spec, this PR does nothing to add security for users (other than add a trivial inconvenience for attackers, who can run any other code they desire, or simply use an older version of paho.mqtt).

@PierreF As I understand it, and the code base (possibly mistakenly, I admit) This PR misunderstands the issue and is even Security Theatre. Personally I'd close it to maintain high standards for the code, and avoid setting a bad precedent.

The decoding code in Paho.mqtt that calculates remaining_length is this while loop:

        if self._in_packet['have_remaining'] == 0:
            # Read remaining
            # Algorithm for decoding taken from pseudo code at
            # http://publib.boulder.ibm.com/infocenter/wmbhelp/v6r0m0/topic/com.ibm.etools.mft.doc/ac10870_.htm
            while True:
                try:
                    byte = self._sock_recv(1)
                except BlockingIOError:
                    return MQTTErrorCode.MQTT_ERR_AGAIN
                except OSError as err:
                    self._easy_log(
                        MQTT_LOG_ERR, 'failed to receive on socket: %s', err)
                    return MQTTErrorCode.MQTT_ERR_CONN_LOST
                else:
                    if len(byte) == 0:
                        return MQTTErrorCode.MQTT_ERR_CONN_LOST
                    byte_value = byte[0]
                    self._in_packet['remaining_count'].append(byte_value)
                    # Max 4 bytes length for remaining length as defined by protocol.
                    # Anything more likely means a broken/malicious client.
                    if len(self._in_packet['remaining_count']) > 4:
                        return MQTTErrorCode.MQTT_ERR_PROTOCOL

                    self._in_packet['remaining_length'] += (
                        byte_value & 127) * self._in_packet['remaining_mult']
                    self._in_packet['remaining_mult'] = self._in_packet['remaining_mult'] * 128

                if (byte_value & 128) == 0:
                    break

[63n0]

Thank you for the thorough review. I'd like to clarify a few points.

---

On the paho.mqtt.golang issue

I think there may be a slight misunderstanding about the threat model in eclipse-paho/paho.mqtt.golang#737.

That issue assumes an attacker who can control input to the client application but has no direct access to the MQTT broker. The concern is that if such an attacker feeds a sufficiently large value into the application, paho.mqtt.golang could generate a packet with a Remaining Length exceeding 2²⁸ — which is outside the MQTT specification — and that some brokers may not handle such packets gracefully. For example, Apache ActiveMQ < 6.2.4 suffered an integer overflow when presented with a Remaining Length exceeding 2³², causing a single packet to be misinterpreted as multiple MQTT control packets (CVE-2025-66168).

---

On the decoding side

I noticed that the existing decode loop already handles this correctly — it returns MQTT_ERR_PROTOCOL if remaining_count exceeds 4 bytes, which is exactly what the spec requires. That is precisely why this PR focuses only on the encoding path (_pack_remaining_length), where no such guard currently exists.

---

On paho.mqtt.python specifically

I agree that this is not a security fix, and I want to be clear on that point. As noted in the issue, the realistic security impact is extremely low. Unlike the golang case, it is not realistically possible to set a Remaining Length as large as 2³¹ or 2³² through this library's API.

Theoretically, if a broker were to stop reading at the 4th byte regardless of the MSB and treat only those bytes as the length, the attack could succeed — but among the 23 brokers I surveyed, none exhibited such behavior. And even if such a broker existed, the truncation would occur on the broker side, which I think should reasonably be considered the broker's responsibility rather than the client's.

---

Why I still submitted this PR

Although the security impact appears minimal in practice, I noticed that depending on how an application using this library is implemented, there could be a code path that sends a spec-violating packet without the developer intending to do so. That felt worth addressing.

I'm aware that my implementation may not meet the project's coding standards, and I'm very open to feedback or suggestions on how to improve it. If there's a cleaner approach you'd prefer, I'm happy to revise accordingly.

[JamesParrott]

@63n0 Thanks. LGTM then. The change does ensure we play nicely (in accordance with the spec) with brokers, both good and bad. Also it rules out a potential bug, closes the FIXME, and makes the codebase more robust.

Also the code quality is fine. My reference about "quality" was intended towards avoiding adding PRs for the sake of it, with no real benefit.