search

eclipse-mosquitto / mosquitto

Eclipse Mosquitto - An open source MQTT broker
https://mosquitto.org
Other
9.06k stars 2.39k forks source link

Behavior with malformed packets #2325

Open scompo opened 3 years ago

scompo commented 3 years ago

Hello, I was facing reconnection issues with some clients.

After analyzing the TCP traffic with tcpdump I've noticed that a client sent/or I received a malformed PINGREQ packet, it was supposed to be 0xc0 followed by 0x00 instead I'm getting 0x0a followed by 0xc0.

Right now the client doesn't get a response from it's (broken) PINGREQ and resets itself as per specification.

Is this the expected broker behavior in this case?

I think the packet it's ignored (no log mentions), should the client be disconnected or notified?

EDIT:

I'm using version 2.0.12 from snap

I looked a bit at the code and in src/read_handle.c this case should fall into the default case branch, being a 3.1.x spec client, no action is performed.

Is this correct? Should the client be disconnected?

ralight commented 3 years ago

I'm not quite clear what is happening from your description I'm afraid. If the client sends a malformed PINGREQ, I would expect it to be disconnected. If you can reproduce this or still have a pcap file, I'd suggest taking a look in wireshark - the MQTT dissectors are very good and will flag malformed packets so it is much easier to see what is going on.

scompo commented 3 years ago

From the capture I'm receiving the malformed packet on the server and no activity from the broker, just an acknowledgement of the tcp packet sent.

The broker just does nothing and leaves the connection opened, then the client receives no response and closes the connection.

From what I can see in the code a disconnect is issued only if the protocol il version 5, is this the expect behavior from the broker as specified from the protocol?

ckrey commented 5 days ago

I successfully reproduced the problem: Your are actually not sending a malformed PINGREQ packet (c000), but a reserved command (0A) with a remaining length of c0, which is an illegal length specification. The broker expects more bytes to read, but does not receive any. This is why it is waiting.

It is obviously a bug in the client to send a reserved command, but the reserved command should be rejected by the broker before waiting for a remaining length to be transmitted.

ralight commented 5 days ago

Thank you, this is now fixed for 2.0.21.

scompo commented 3 days ago

Yeah, thank you, it was a 3rd party issue with their code and we ended up telling them to fix it. Thank you for fixing the potential problem on your part too!