ruby-mqtt
ruby-mqtt copied to clipboard
njh
[Security] Handle instantaneous and unsolicited PUBACKs
Prior to this commit, it was possible to crash the read thread by sending a PUBACK with an unexpected ID, e.g. an ID the client had not used before, or had already deleted the queue for. These packages will now simple be ignored.
Furthermore, with QoS 1 or 2, the PUBACK queue for a package was only created after the package had already been published. If the PUBACK arrived within this tiny gap, the read thread would crash. We fix this by creating the queue before starting to publish.
Thank you for this PR. I am wondering if this is the correct behaviour - to ignore invalid PUBACK identifiers. Under what situation would you expect this to occur? Due to a faulty server? Or due to a bad actor? I think if you are connected to a server that is trying to do harm to its clients, you might have bigger problems.
I haven't read the specification in a long time. But typically in the MQTT protocol, if something goes wrong, the action is to disconnect. I did a quick scan and I don't think this scenario is documented in version 3.1.1. of the specification.
Interested in your thoughts.
@njh Malicious actors aside, the current code will first send a package and then add it to the list of "awaiting PUBACK". This allows for a race condition: If the PUBACK is processed before you can register that you are actually waiting for it, then the code will break. This PR essentially suggests to first register your expectation for a PUBACK before publishing, thereby eliminating the race condition.
Yeah, ran into this issue too. The race condition mentioned is very real and I hit it quite often when I use QoS 1. I am not a fan of ignoring unknown packets but the part of the patch which fixes the race condition is something which is useful.
