espressif / esp-mqtt

ESP32 mqtt component
Apache License 2.0
603 stars 255 forks source link

Thingsboard invalid headers - not to spec (IDFGH-1942) #134

Closed Kiogora closed 4 years ago

Kiogora commented 5 years ago

For those using thingsboard, the implementation of the broker does not follow the 3.1.1 spec with regard to acknowledgement flags thus:

Though wireshark does identify the header - Maybe it doesnt check flags on some commands for proper identification?

You may edit the mqtt_message.c to edit the expected flags check in mqtt_has_valid_msg_hdr() function as below:

int mqtt_has_valid_msg_hdr(uint8_t *buffer, uint16_t length)
{
    int qos, dup;

    if (length < 1) {
        return 0;
    }
    switch (mqtt_get_type(buffer)) {
    case MQTT_MSG_TYPE_CONNECT:
    case MQTT_MSG_TYPE_CONNACK:
    case MQTT_MSG_TYPE_PUBREC:
    case MQTT_MSG_TYPE_PUBCOMP:
    case MQTT_MSG_TYPE_UNSUBACK:
    case MQTT_MSG_TYPE_PINGREQ:
    case MQTT_MSG_TYPE_PINGRESP:
    case MQTT_MSG_TYPE_DISCONNECT:
        return (buffer[0] & 0x0f) == 0;  /* all flag bits are 0 */
    case MQTT_MSG_TYPE_PUBREL:
    case MQTT_MSG_TYPE_SUBSCRIBE:
    case MQTT_MSG_TYPE_UNSUBSCRIBE:
        return (buffer[0] & 0x0f) == 0x02;  /* only bit 1 is set */
    case MQTT_MSG_TYPE_SUBACK:
    case MQTT_MSG_TYPE_PUBACK:
        return (((buffer[0] & 0x0f) == 0x02)||((buffer[0] & 0x0f) == 0));  /* Break mqtt rules by accepting all flag bits 0 or bit 1 is set for thingsboard*/
    case MQTT_MSG_TYPE_PUBLISH:
        qos = mqtt_get_qos(buffer);
        dup = mqtt_get_dup(buffer);
        /*
         * there is no qos=3  [MQTT-3.3.1-4]
         * dup flag must be set to 0 for all qos=0 messages [MQTT-3.3.1-2]
         */
        return (qos < 3) && ((qos > 0) || (dup == 0));
    default:
        return 0;
    }
}
david-cermak commented 4 years ago

Thanks @Kiogora for sharing this. I don't think we can do anything about it in esp-mqtt, not even "tolerate" these reserved bits, as the spec says the receiver (not only server) MUST close the connection.

Kiogora commented 4 years ago

Thank you @david-cermak . This is understood. I can close this, to archive it in case anybody finds the same problem.