Adafruit_MQTT::publishPacket: Protect against memory corruption. #166

flavio-fernandes · 2019-12-24T03:27:33Z

Fixes #109
Fixes #122

After spending a long time pulling my hair to understand why I was
hitting a panic when attempting to read from my registered
subscriptions, I found out that the subscriptions member of the
Adafruit_MQTT instance was corrupted. :(

Turns out the memory corruption was caused by my publish
call, where the payload I was providing was bigger than the
allocated space in the buffer for construction of the packet
(see buffer[MAXBUFFERSIZE]).

To protect myself from ever making this mistake again, I am
proposing a simple logic in publishPacket where instead of
silently corrupting memory, the code uses as much payload
as it can fit in the available space. By seeing the
truncated payload, user can decide whether he/she should
1)break it up into different topics, 2) put the payload on
a diet, or 3) increase MAXBUFFERSIZE.

====

Describe the scope of your change--i.e. what the change does and what parts
of the code were modified. This will help us understand any risks of integrating
the code.

This pull requests improves the handling of buffer for packet creation of
Adafruit_MQTT::publishPacket. It adds argument maxPacketLen to keep track
of how much space is available in buffer.

Describe any known limitations with your change. For example if the change
doesn't apply to a supported platform of the library please mention it.

This change is not modifying the sizes of any buffers. Instead, it truncates
the payload to avoid memory corruption of the class Adafruit_MQTT by keeping
packet creation from going beyond MAXBUFFERSIZE.

Please run any tests or examples that can exercise your modified code. We
strive to not break users of the code and running tests/examples helps with this
process.

See this gist with an example code where a packet has a payload that exceeds
MAXBUFFERSIZE and corrupts packet_id_counter and subscriptions of the
Adafruit_MQTT instance.

See comments below that gist with the crash and stack trace of code above, where memory is
corrupted and the LoadProhibited exception takes place.

Lastly, see the comment that follows with the proposed modification, where the same code
simply gets the payload truncated instead of allowing memory to get messed up.

Avoid memory corruption from happening when data payload provided in Adafruit_MQTT::publishPacket is greater than MAXBUFFERSIZE. In order to do that, a helper function is being added to calculate how much space is available for the payload after subtracting what is used as header. Pull request adafruit#166 Fixes adafruit#109 Fixes adafruit#122 Signed-off-by: Flavio Fernandes <[email protected]>

Avoid memory corruption from happening when data payload provided in Adafruit_MQTT::publishPacket is greater than MAXBUFFERSIZE. In order to do that, a helper function is being added to calculate how much space is available for the payload after subtracting what is used as the header. Pull request adafruit#166 Fixes adafruit#109 Fixes adafruit#122 Signed-off-by: Flavio Fernandes <[email protected]>

BrunoMartins11 · 2021-03-12T17:03:52Z

It's 2021 and I still struggled to find that this is the reason why my program is crashing. Please review this PR asap for the sake of our mental health.

Duckle29 · 2021-03-28T19:34:03Z

I share the frustration :( There seems to be a lot of nice PRs that have been sitting for more than a year. It's a shame. Thankfully platformIO lets me use git repos for lib_deps, so I forked your fix @flavio-fernandes, and used that. Thanks for the work you put in! :)

I'd switch to something like Async MQTT, but I need to use a library that uses a Client object like this, to work with my bearSSL certificate store.

flavio-fernandes · 2021-03-28T19:37:03Z

I share the frustration :( There seems to be a lot of nice PRs that have been sitting for more than a year. It's a shame. Thankfully platformIO lets me use git repos for lib_deps, so I forked your fix @flavio-fernandes, and used that. Thanks for the work you put in! :)

I'd switch to something like Async MQTT, but I need to use a library that uses a Client object like this, to work with my bearSSL certificate store.

Maybe @ladyada or @brentru can listen to our cries. I do not mind if a different PR gets used; just wish for a better experience using this awesome library.

brentru

@flavio-fernandes Made two small comments. Rest looks good and is similar to a portion of MiniMQTT which I've implemented to address a similar issue.

After you're done addressing them, please bump the patch version number in https://github.com/adafruit/Adafruit_MQTT_Library/blob/master/library.properties#L2 by 1 so we can get this picked up by the Arduino Library Manager tool.

brentru · 2021-03-29T14:46:10Z

Adafruit_MQTT.h

  uint16_t publishPacket(uint8_t *packet, const char *topic, uint8_t *payload,
-                         uint16_t bLen, uint8_t qos);
+                         uint16_t bLen, uint8_t qos, uint16_t maxPacketLen);


@flavio-fernandes is there a way to protect against memory corruption without changing the function signature here?

Not really. 😄 I think that the only way we can protect from the memory corruption is by knowing how much space is available to begin with.
This is a private member function, so there should not be a concern about changing its signature, agree?
Regardless, I made the extra parameter have the default value of 0, so caller will still work should we try to use the old function arguments.

brentru · 2021-03-29T14:46:54Z

Adafruit_MQTT.cpp

@@ -665,11 +666,22 @@ uint8_t Adafruit_MQTT::connectPacket(uint8_t *packet) {
  return len;
 }

+uint16_t Adafruit_MQTT::packetAdditionalLen(uint16_t currLen) {


could this instead be incorporated into publishPacket?

Okay. I will use that function in cpp file as a static. So this new line is now removed.

flavio-fernandes · 2021-03-29T15:51:33Z

@flavio-fernandes Made two small comments. Rest looks good and is similar to a portion of MiniMQTT which I've implemented to address a similar issue.

After you're done addressing them, please bump the patch version number in https://github.com/adafruit/Adafruit_MQTT_Library/blob/master/library.properties#L2 by 1 so we can get this picked up by the Arduino Library Manager tool.

Ack! TBH, it has been a while since I thought about this PR.
Let me think a little bit about what you said and make the changes.

brentru · 2021-03-31T15:32:16Z

@flavio-fernandes ok, let me know when its ready to review. I have a pretty large MQTT application which uses this library and I'd like to test with that.

Avoid memory corruption from happening when data payload provided in Adafruit_MQTT::publishPacket is greater than MAXBUFFERSIZE. In order to do that, a helper function is being added to calculate how much space is available for the payload after subtracting what is used as the header. Pull request adafruit#166 Fixes adafruit#109 Fixes adafruit#122 Signed-off-by: Flavio Fernandes <[email protected]>

flavio-fernandes · 2021-04-02T16:28:33Z

@flavio-fernandes ok, let me know when its ready to review. I have a pretty large MQTT application which uses this library and I'd like to test with that.

Hi @brentru ! Okay, I made the changes and pushed an update. Let me know what you think, please.
As for testing the bug, consider using this gist to easily make the corruption take place without the fix; and the non-crashing version once the pr changes are added. Lastly, I bumped the version to 2.1.1, as you requested.

Best!

brentru · 2021-05-12T21:36:29Z

@flavio-fernandes Could you bump to 2.2.0? I tested this against a SECRET project and it prevents corruption there as well.

flavio-fernandes · 2021-05-13T01:21:27Z

@flavio-fernandes Could you bump to 2.2.0? I tested this against a SECRET project and it prevents corruption there as well.

@brentru : Done bumping to 2.2.0 . Secret project? You make me curious! :)
Best,
-- flaviof

xdylanm · 2021-05-13T16:35:40Z

Adafruit_MQTT.cpp

+    return 0;
+  if (currLen < 16384)
+    return 1;
+  if (currLen < 2097151)


I think currLen (unsigned 16-bit) will always be less than 65536, so this comparison might give an unexpected result (I made the same mistake, still trying to think of a solution. I've noticed that many types in this class assume maximum lengths that might not be compatible with all message/packet lengths -- some discussion might be helpful there).

Yes, you are right! I still think that the compare should work, granted the compiler promote the type. If not, we could explicitly cast the variable to uitn32u, but that may not be doable on older arduino boards?

flavio-fernandes force-pushed the protect.buffer branch from ba0f9df to 2847f9f Compare December 24, 2019 03:29

This was referenced Dec 24, 2019

Buffer overflow in publishPacket #122

Closed

What's the max size for publish in esp8266 #109

Closed

flavio-fernandes force-pushed the protect.buffer branch from 2847f9f to 6389dc4 Compare December 25, 2019 03:07

flavio-fernandes force-pushed the protect.buffer branch from 6389dc4 to 9aedbd9 Compare December 25, 2019 03:21

flavio-fernandes force-pushed the protect.buffer branch from 9aedbd9 to 4204e1d Compare December 25, 2019 18:17

flavio-fernandes force-pushed the protect.buffer branch from 4204e1d to 6bcc819 Compare December 25, 2019 21:00

flavio-fernandes requested review from brentru, ladyada and PaintYourDragon February 24, 2020 15:41

brentru removed their request for review February 27, 2020 19:32

flavio-fernandes force-pushed the protect.buffer branch from 6bcc819 to ad914cd Compare August 29, 2020 19:58

flavio-fernandes force-pushed the protect.buffer branch from ad914cd to 3fdbdfb Compare August 29, 2020 20:40

flavio-fernandes force-pushed the protect.buffer branch from 3fdbdfb to 785b5a4 Compare August 29, 2020 20:44

brentru self-requested a review October 7, 2020 14:32

brentru requested changes Mar 29, 2021

View reviewed changes

brentru removed request for PaintYourDragon and ladyada March 29, 2021 14:49

flavio-fernandes force-pushed the protect.buffer branch from 785b5a4 to 08c6ded Compare April 2, 2021 16:03

flavio-fernandes force-pushed the protect.buffer branch from 7565b2e to 3074f51 Compare April 2, 2021 16:15

Bump to 2.2.0

fff701e

flavio-fernandes force-pushed the protect.buffer branch from 3074f51 to fff701e Compare May 13, 2021 01:19

Merge branch 'master' into protect.buffer

ce30d2a

flavio-fernandes requested a review from brentru May 13, 2021 01:26

xdylanm mentioned this pull request May 13, 2021

Support publishing and receiving large messages. #193

Merged

xdylanm reviewed May 13, 2021

View reviewed changes

brentru approved these changes May 14, 2021

View reviewed changes

brentru merged commit ca12d21 into adafruit:master May 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adafruit_MQTT::publishPacket: Protect against memory corruption. #166

Adafruit_MQTT::publishPacket: Protect against memory corruption. #166

Uh oh!

flavio-fernandes commented Dec 24, 2019 •

edited

Loading

Uh oh!

BrunoMartins11 commented Mar 12, 2021

Uh oh!

Duckle29 commented Mar 28, 2021

Uh oh!

flavio-fernandes commented Mar 28, 2021

Uh oh!

brentru left a comment

Uh oh!

brentru Mar 29, 2021

Uh oh!

flavio-fernandes Apr 2, 2021

Uh oh!

brentru Mar 29, 2021

Uh oh!

flavio-fernandes Apr 2, 2021

Uh oh!

flavio-fernandes commented Mar 29, 2021

Uh oh!

brentru commented Mar 31, 2021

Uh oh!

flavio-fernandes commented Apr 2, 2021

Uh oh!

brentru commented May 12, 2021

Uh oh!

flavio-fernandes commented May 13, 2021

Uh oh!

xdylanm May 13, 2021

Uh oh!

flavio-fernandes May 13, 2021

Uh oh!

Uh oh!

Adafruit_MQTT::publishPacket: Protect against memory corruption. #166

Adafruit_MQTT::publishPacket: Protect against memory corruption. #166

Uh oh!

Conversation

flavio-fernandes commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrunoMartins11 commented Mar 12, 2021

Uh oh!

Duckle29 commented Mar 28, 2021

Uh oh!

flavio-fernandes commented Mar 28, 2021

Uh oh!

brentru left a comment

Choose a reason for hiding this comment

Uh oh!

brentru Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

flavio-fernandes Apr 2, 2021

Choose a reason for hiding this comment

Uh oh!

brentru Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

flavio-fernandes Apr 2, 2021

Choose a reason for hiding this comment

Uh oh!

flavio-fernandes commented Mar 29, 2021

Uh oh!

brentru commented Mar 31, 2021

Uh oh!

flavio-fernandes commented Apr 2, 2021

Uh oh!

brentru commented May 12, 2021

Uh oh!

flavio-fernandes commented May 13, 2021

Uh oh!

xdylanm May 13, 2021

Choose a reason for hiding this comment

Uh oh!

flavio-fernandes May 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flavio-fernandes commented Dec 24, 2019 •

edited

Loading