Integer overflow on Arduino (ATmega) in "MqttDecode_Vbi"

[cbjornram]

After unsuccessfully trying to get wolfMQTT up and running on an Arduino Mega I found something I believe is an issue. I am relatively new to digging around in C++ libraries, so please correct me (and feel free to close the issue) if I have misunderstood.

In the following lines used when decoding incoming MQTT messages, "multiplier" is defined as an int. For regular Arduino ATmega-chipsets (Uno, Mega, etc.) integers are 16 bits, which means the part at lines 229-232 overflow since the do/while-loop is designed to run up to four times and "MQTT_PACKET_LEN_ENCODE_MASK" is defined as 0x80 in MQTT_packet.h.

wolfMQTT/src/mqtt_packet.c

Lines 217 to 240 in 456ac9e

	int MqttDecode_Vbi(byte *buf, word32 *value, word32 buf_len)
	{
	word32 rc = 0;
	int multiplier = 1;
	byte encodedByte;
	*value = 0;
	do {
	if (buf_len < rc + 1)
	return MQTT_CODE_ERROR_OUT_OF_BUFFER;
	encodedByte = *(buf++);
	*value += (encodedByte & ~MQTT_PACKET_LEN_ENCODE_MASK) * multiplier;
	multiplier *= MQTT_PACKET_LEN_ENCODE_MASK;
	if (multiplier > (MQTT_PACKET_LEN_ENCODE_MASK *
	MQTT_PACKET_LEN_ENCODE_MASK *
	MQTT_PACKET_LEN_ENCODE_MASK))
	{
	return MQTT_CODE_ERROR_MALFORMED_DATA;
	}
	rc++;
	} while ((encodedByte & MQTT_PACKET_LEN_ENCODE_MASK) != 0);
	return (int)rc;
	}

This causes an "Error (Malformed Remaining Length)" (MQTT_CODE_ERROR_MALFORMED_DATA) at runtime when trying to initialise a connection to a broker even if the received CONNACK header is correctly formatted. As a consequence, the Arduino can not make a connection to a broker at all.

Kind regards,
Christoffer

[cbjornram]

An idea I tried was changing "multiplier" and the calculations on lines 230-233 into longs instead of ints, which seems to make it work (see code snippet below). I suspect however that this solution is not desirable on non-ATmega-platforms so I have not made a PR for it. Please let me know if you want one.

All the best,
Christoffer

int MqttDecode_Vbi(byte *buf, word32 *value, word32 buf_len)
{
    word32 rc = 0;
    long multiplier = 1; // ----- Made into long -----
    long encodeMask = MQTT_PACKET_LEN_ENCODE_MASK; // ----- Made into long -----
    byte encodedByte;

    *value = 0;
    do {
        if (buf_len < rc + 1)
            return MQTT_CODE_ERROR_OUT_OF_BUFFER;
       encodedByte = *(buf++);
       *value += (encodedByte & ~MQTT_PACKET_LEN_ENCODE_MASK) * multiplier;
       multiplier *= MQTT_PACKET_LEN_ENCODE_MASK;
       if (multiplier > (encodeMask *
                         encodeMask *
                         encodeMask)) // ----- Calculations now in long -----
       {
          return MQTT_CODE_ERROR_MALFORMED_DATA;
       }
       rc++;
    } while ((encodedByte & MQTT_PACKET_LEN_ENCODE_MASK) != 0);

    return (int)rc;
}

[embhorn]

Hi @cbjornram

Thanks for your feedback. Is this the only required change you've found for wolfMQTT to work on the 16-bit platform?

[cbjornram]

Hi @embhorn,

Thank you for your quick reply. This is the only change I have found so far, but I don't have my project up and running just yet. I am currently troubleshooting publishing (that may or may not be related to the library, it's too early to say), and after that I will continue with testing subscriptions. So I'm afraid I don't know yet.

If I do find something else I will publish it in this thread, and if not I can reply again when I got it working. That could be a few weeks away though as my time for this project is limited.

[cbjornram]

A small update:
In the custom type definitions the definition for word32 should changed to unsigned long (line 162 in mqtt_types.h):

wolfMQTT/wolfmqtt/mqtt_types.h

Lines 161 to 163 in 456ac9e

	#ifndef word32
	typedef unsigned int word32;
	#endif

I am still not live with the project so I might still find more things. I'll get back again when I know more.

[orangecalculator]

Hello.

Why not use stdint.h since it contains integer types of exact specified length?

[cbjornram]

I agree @orangecalculator,that's a much better solution. If I make a PR out of this I will do that instead, and if not then maybe the wolfMQTT team could discuss that option internally moving forward.

[orangecalculator]

I think an another solution that the wolfSSL team would much more like is some existing code in their wolfssl repository.

https://github.com/wolfSSL/wolfssl/blob/0abbd9b1ec660bd006e85067fdfe12bf7ec26bf1/wolfssl/wolfcrypt/types.h#L70-L88

I think making use of a macro WC_16BIT_CPU would also be a viable solution.

[embhorn]

I think this problem is already solved. You can define WOLFMQTT_CUSTOM_TYPES and WOLFMQTT_USER_SETTINGS, then define the types for your system in a file named "user_settings.h" that is somewhere in the include path.

wolfMQTT/wolfmqtt/mqtt_types.h

Line 153 in 2801cb2

#if !defined(WOLFMQTT_CUSTOM_TYPES) && !defined(WOLF_CRYPT_TYPES_H)

[cbjornram]

Hi again,

Now I have it up and running and I didn't find anything else. I think your suggested solution @embhorn is sufficient to get it working on the more basic Arduino's such as Uno and Mega. It might be worth mentioning the custom types in the Arduino README.md, and if I get to it I can send a PR later this week for it.

Thanks for the help.

[pperzyna]

@cbjornram Your PR here could help others, including me :)