Error decoding binary message (Not enough bits)

[philipparndt]

Hello,

when a binary with the binary content 0x78, 0x9C, 0x8D message is published to the topic test, this results in a crash in MqttDecodePublish. The error message is Swift/Integers.swift:3447: Fatal error: Not enough bits to represent the passed value

Version 2.0.2-beta4 as I like to start supporting MQTT5

Test Case:

import XCTest
import CocoaMQTT

class CocoaMQTTRegressionTests: XCTestCase {
    func testDecodeBinary789c8d() {
        let publish = MqttDecodePublish()
        publish.decodePublish(fixedHeader: 49, publishData:
                                [0, 4, 116, 101, 115, 116,
                                 0x78, 0x9C, 0x8D])
    }
}

[leeway1208]

hi @philipparndt thank you for your report. I will check the problem soon. If you have some good ideas, please let me know.😄

[philipparndt]

hi @leeway1208 thanks for having a look on this. I do not know the whole specification of MQTT, but what I see is that that the first two bytes of the message payload are interpreted as property name / Payload Format Indicator.

This will most likely result in a invalid property name but in some cases (for the example data) it not only results in a invalid property name but in a "property index" that does not match in UInt8 which causes the crash.

A quick and dirty fix would be to check the range and break early e.g.:

let propertyNameByte = resVariableByteInteger.res
if (propertyNameByte > 255) {
  break
}
guard let propertyName = CocoaMQTTPropertyName(rawValue: UInt8(propertyNameByte)) else {
    break
}

But the main question to me is, why is the payload interpreted as header? And how do we know that the header is present? Otherwise the payload could be interpreted as some other content in case it randomly matches a property name 😄

[leeway1208]

@philipparndt Hi . Thanks for your so detailed description. after we review the code. we find you lost the PUBLISH Variable Header (3.3.2) in test case. If the variable header dose not have any property, you should add 0 in it. So the right test case may like this:

import XCTest
import CocoaMQTT

class CocoaMQTTRegressionTests: XCTestCase {
    func testDecodeBinary789c8d() {
        let publish = MqttDecodePublish()
        publish.decodePublish(fixedHeader: 49, publishData:
                                [0, 4, 116, 101, 115, 116,
                                 0, 0x78, 0x9C, 0x8D])
    }
}

Here is the link for your reference (https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901119)

[philipparndt]

Hi @leeway1208

unfortunately the test case is correct. Given a binary file with three bytes and the content 0x78 0x9C 0x8D When I call mosquitto_pub -h localhost -t test -u admin -P password -f ./extracted_0-2.bin

I get the following result:

(lldb) po publishData
▿ 9 elements
  - 0 : 0
  - 1 : 4
  - 2 : 116
  - 3 : 101
  - 4 : 115
  - 5 : 116
  - 6 : 120
  - 7 : 156
  - 8 : 141

So there is no header in this case.

When I execute mosquitto_pub -h localhost -t test -m test the header is missing as well:

(lldb) po publishData
▿ 10 elements
  - 0 : 0
  - 1 : 4
  - 2 : 116
  - 3 : 101
  - 4 : 115
  - 5 : 116
  - 6 : 116
  - 7 : 101
  - 8 : 115
  - 9 : 116

[philipparndt]

Ah okay I think I got it 😄 the properties are just for MQTT5, in this case MQTT3.x and 5 are get mixed up. I've debugged the same message in MQTT.js and at the same point there is the following section:

  // ... parse topic ...

    // Properties mqtt 5
    if (this.settings.protocolVersion === 5) {
      const properties = this._parseProperties()
      if (Object.getOwnPropertyNames(properties).length) {
        packet.properties = properties
      }
    }

  // ... use payload ...

see: https://github.com/mqttjs/mqtt-packet/blob/23774e79e0ca83a718b95d254414964e7a421b1f/parser.js#L316

[leeway1208]

You are right. MQTT3.x and 5 are a little different on properties. hahaha.

[philipparndt]

Do you have a look at this, or shall I dive a little bit deeper and try to create a PR on this? I like to support both protocol versions in the MQTTAnalyzer App, so going back to 1.x would be a temporary fix but not a long-term goal.

[leeway1208]

@philipparndt Hi. Sorry for the late reply. I did get mixed up with different mqtt versions and I will fix it tonight.😭😭

[philipparndt]

@leeway1208 thanks for the fast fix. My manual tests and the regression test are both "green". So I think you can close this ticket.