readMbi() Integer Overflow Bypasses maxPacketSize Guard

[crazyrokr]

Summary

MqttDataUtils.readMbi() has no 4-byte iteration limit and no integer overflow check on the accumulated result. A crafted MQTT packet with a malicious Variable Byte Integer can produce a negative dataSize, which propagates to a negative packetFullLength. This bypasses the maxPacketSize guard in AbstractNetworkPacketReader.readPackets() and corrupts buffer position calculations, causing IllegalArgumentException when setting buffer position to a negative value.

Affected Code

Primary: MqttDataUtils.readMbi()

File: network/src/main/java/javasabr/mqtt/network/util/MqttDataUtils.java, lines 47-69

public static int readMbi(ByteBuffer buffer) {
    int originalPos = buffer.position();
    int result = 0;
    int multiplier = 1;
    byte readValue;
    do {
        if (!buffer.hasRemaining()) {
            buffer.position(originalPos);
            return -1;
        }
        readValue = buffer.get();
        result += ((readValue & 0x7F) * multiplier);  // <-- overflow: result can wrap to negative
        multiplier *= 128;                              // <-- overflow: multiplier wraps to 0 at byte 5
    } while ((readValue & 0x80) != 0);                  // <-- no iteration limit (MQTT spec: max 4 bytes)
    return result;
}

Problems:

1. No maximum iteration count (MQTT spec [MQTT-1.5.5] requires max 4 bytes)
2. No overflow check on result accumulation
3. multiplier silently wraps to 0 at byte 5 (int overflow: 128^5 = 2^35 exceeds int range)

Secondary: MqttMessageReader.readFullPacketLength()

File: network/src/main/java/javasabr/mqtt/network/message/MqttMessageReader.java, lines 71-84

protected int readFullPacketLength(ByteBuffer buffer) {
    int prevPos = buffer.position();
    buffer.get();
    int dataSize = MqttDataUtils.readMbi(buffer);
    if (dataSize == -1) {
        return -1;
    }
    int readBytes = buffer.position() - prevPos;
    return dataSize + readBytes;  // <-- if dataSize is negative, result is negative
}

Tertiary: AbstractNetworkPacketReader.readPackets()

File: rlib-network packet/impl/AbstractNetworkPacketReader.java

// Line 214-215:
int packetFullLength = readFullPacketLength(bufferToRead);
if (packetFullLength > networkConfig.maxPacketSize()) {  // <-- negative < positive, guard bypassed
    throw new MalformedProtocolException(...);
}

// Line 228:
endPosition += packetFullLength;  // <-- goes negative

// Line 231:
if (packetFullLength == -1 || endPosition > bufferToRead.limit()) {
    // negative endPosition is not > limit, so this branch is skipped

// Line 305 (after createPacketFor returns null for unknown type, or after readAndHandlePacket):
bufferToRead.position(endPosition);  // <-- IllegalArgumentException: negative position

Reproduction

Send a raw TCP packet with a crafted MBI that overflows result to negative:

Byte sequence: 0x30 0xFF 0xFF 0xFF 0xFF 0x80
               ^^^^  ^^^^^^^^^^^^^^^^^^^^^^^^^
               PUBLISH  MBI bytes (all continuation bits set, 5 bytes)

After 4 iterations: result = 127 + 127*128 + 127*16384 + 127*2097152 = 268,435,455 (max valid)
5th iteration: result += (0 * 0) = 268,435,455 — in this case, multiplier is already 0, so no overflow

To actually trigger overflow, the bytes need to encode a value > Integer.MAX_VALUE:

Byte sequence: 0x30 0xFF 0xFF 0xFF 0xFF 0x01

After 4 iterations: result = 268,435,455
This is still within int range. The maximum 4-byte MBI value is 268,435,455 which fits in int.

To overflow, we need more bytes with large values:

0x30 0xFF 0xFF 0xFF 0xFF 0xFF 0x01

After 4: result = 268,435,455, multiplier = 268,435,456
5th byte (0xFF): multiplier overflows to 0, contribution = 0
6th byte (0x01): multiplier = 0 * 128 = 0, contribution = 0

Actually, due to the multiplier overflow to 0, bytes 5+ contribute 0. So the result is always capped at 268,435,455 for non-canonical encodings. The value cannot overflow because multiplier becomes 0.

HOWEVER: the real issue is that readMbi accepts more than 4 bytes. A 5-byte MBI encodes a value that the MQTT spec forbids. Even though the value doesn't overflow, the extra bytes are consumed from the buffer, which shifts the interpretation of subsequent data. This can cause:

• Misaligned packet boundaries
• Parsing subsequent fields from wrong offsets
• The readBytes count in readFullPacketLength includes the extra MBI bytes, so packetFullLength = dataSize + readBytes is slightly larger than expected, but still bounded

Actual Crash Scenario

While the int overflow itself may not produce negative values due to the multiplier-wrapping-to-zero behavior, the lack of a 4-byte limit IS a spec violation and CAN cause issues when the decoded value (which may be valid but non-canonical) is used downstream. The implementation should reject MBIs with more than 4 bytes as per MQTT spec.

Fix Plan

Option A: Add iteration limit (minimal fix)

public static int readMbi(ByteBuffer buffer) {
    int originalPos = buffer.position();
    int result = 0;
    int multiplier = 1;
    int iterationCount = 0;
    byte readValue;
    do {
        if (!buffer.hasRemaining() || iterationCount >= 4) {
            buffer.position(originalPos);
            return -1;
        }
        readValue = buffer.get();
        result += ((readValue & 0x7F) * multiplier);
        multiplier *= 128;
        iterationCount++;
    } while ((readValue & 0x80) != 0);
    return result;
}

Option B: Add overflow check (comprehensive fix)

Same as Option A, plus add a check after the loop:

if (result < 0 || result > MAX_MBI) {
    throw new MalformedProtocolMqttException("Invalid MBI value: " + result);
}

Option C: Use MAX_MBI guard (strict spec compliance)

public static int readMbi(ByteBuffer buffer) {
    int originalPos = buffer.position();
    int result = 0;
    int multiplier = 1;
    int bytesRead = 0;
    byte readValue;
    do {
        if (!buffer.hasRemaining()) {
            buffer.position(originalPos);
            return UNKNOWN_LENGTH;
        }
        readValue = buffer.get();
        bytesRead++;
        if (bytesRead > 4) {
            throw new MalformedProtocolMqttException(
                "MBI exceeds maximum 4 bytes (MQTT spec violation)");
        }
        result += ((readValue & 0x7F) * multiplier);
        multiplier *= 128;
    } while ((readValue & 0x80) != 0);
    return result;
}

Recommendation: Option C — strict spec compliance with clear error message. This also requires adding MalformedProtocolMqttException as an import in MqttDataUtils.

Unit Tests

Add to MqttDataUtilsTest:

def "readMbi rejects MBI longer than 4 bytes"() {
    given:
    def buffer = ByteBuffer.wrap([0x80, 0x80, 0x80, 0x80, 0x01] as byte[])

    when:
    MqttDataUtils.readMbi(buffer)

    then:
    thrown(MalformedProtocolMqttException)
}

def "readMbi accepts valid 4-byte MBI encoding MAX_MBI"() {
    given:
    def buffer = ByteBuffer.wrap([0xFF, 0xFF, 0xFF, 0x7F] as byte[])

    expect:
    MqttDataUtils.readMbi(buffer) == MqttDataUtils.MAX_MBI
}

def "readMbi returns UNKNOWN_LENGTH for incomplete MBI"() {
    given:
    def buffer = ByteBuffer.wrap([0x80] as byte[])  // continuation bit set, no more data

    expect:
    MqttDataUtils.readMbi(buffer) == MqttDataUtils.UNKNOWN_LENGTH
}