MQTT functionality has a vulnerablity
Frank-Bemelman opened this issue · 6 comments
Thought I played a bit with the MQTT functionality. I run a Homeassistant setup in my office, to automate things. This has a MQTT broker and tried that.
It all works, but if you send the clock a message without tokens, it crashes the clock. For instance, it the setup is like this:
#define MQTT_CLIENT "ELEKSTUBE"
..and I send it a MQTT message ELEKSTUBE, it will crash.
..and when I send it ELEKSTUBE/directive/powerState OFF - it behaves perfect, turns off the clock
..and when I send it ELEKSTUBE/directive/powerState ON - it behaves perfect, turns on the clock
Problem is in file Mqtt_client_ips.cpp.
The problem is that if you don't provide enough tokens, in the MQTT message sent to the clock, the array with tokens[] pointers only gets initialized for the first Topic token. The strcmp's in the callback function crash on these unintialized wild pointers.
So I changed:
splitTopic(topic, tokens, tokensNumber);
into:
tokensNumber = splitTopic(topic, tokens, tokensNumber);
So that I know how many / seperated tokens there were in the message.
And before the compare of the tokens start, right after the Serial.print stuff:
if(tokensNumber<3)return;
This works, but maybe bit quick and dirty..
That's exactly the right way to handle that. If the message is malformed, just ignore it. MAYBE send back a "huh?" message, but honestly that can cause problems too, so you'd want to wrap rate limiting code around sending the response. I think just ignoring it is a valid response.
...pull request? :-)
Will make a PR request tomorrow. I'm on my mobile phone right now, and too much hassle to work on a small screen. Just wanted some feedback on the idea. It's just a minor thing and it doesn't break anything 😉
Interesting find. MQTT code was taken from SmartNest.cz website (I am using their public broker and app) and if you find any of their repositories here, you can send a PR to them as well.
Hmm. Just had a look at:
https://github.com/aososam/Smartnest
There are quite a few repositories there, and many of them use the same callback with the strcmp that will go wrong when the received mqtt message does not have the topic/subtopic/subtopic format. I have to admit that under normal conditions it is not a big issue, I guess, but still sort of undesirable feature to be able to crash/reboot a device. Will send him a message about it. Thanks for mentioning.
...pull request? :-)
Just made that ;-)
Closed.