Commit 6e92c053e655e865994a582d10bcd3f491c0deaa
1 parent
0586e380
Fix packet parsing bug
When sending many 0xFF, it would overflow. Found by using afl-fuzz.
Showing
2 changed files
with
14 additions
and
2 deletions
client.cpp
| @@ -310,13 +310,16 @@ bool Client::bufferToMqttPackets(std::vector<MqttPacket> &packetQueueIn, Client_ | @@ -310,13 +310,16 @@ bool Client::bufferToMqttPackets(std::vector<MqttPacket> &packetQueueIn, Client_ | ||
| 310 | // Determine the packet length by decoding the variable length | 310 | // Determine the packet length by decoding the variable length |
| 311 | int remaining_length_i = 1; // index of 'remaining length' field is one after start. | 311 | int remaining_length_i = 1; // index of 'remaining length' field is one after start. |
| 312 | uint fixed_header_length = 1; | 312 | uint fixed_header_length = 1; |
| 313 | - int multiplier = 1; | ||
| 314 | - uint packet_length = 0; | 313 | + size_t multiplier = 1; |
| 314 | + size_t packet_length = 0; | ||
| 315 | unsigned char encodedByte = 0; | 315 | unsigned char encodedByte = 0; |
| 316 | do | 316 | do |
| 317 | { | 317 | { |
| 318 | fixed_header_length++; | 318 | fixed_header_length++; |
| 319 | 319 | ||
| 320 | + if (fixed_header_length > 5) | ||
| 321 | + throw ProtocolError("Packet signifies more than 5 bytes in variable length header. Invalid."); | ||
| 322 | + | ||
| 320 | // This happens when you only don't have all the bytes that specify the remaining length. | 323 | // This happens when you only don't have all the bytes that specify the remaining length. |
| 321 | if (fixed_header_length > readbuf.usedBytes()) | 324 | if (fixed_header_length > readbuf.usedBytes()) |
| 322 | return false; | 325 | return false; |
| @@ -335,6 +338,11 @@ bool Client::bufferToMqttPackets(std::vector<MqttPacket> &packetQueueIn, Client_ | @@ -335,6 +338,11 @@ bool Client::bufferToMqttPackets(std::vector<MqttPacket> &packetQueueIn, Client_ | ||
| 335 | throw ProtocolError("An unauthenticated client sends a packet of 1 MB or bigger? Probably it's just random bytes."); | 338 | throw ProtocolError("An unauthenticated client sends a packet of 1 MB or bigger? Probably it's just random bytes."); |
| 336 | } | 339 | } |
| 337 | 340 | ||
| 341 | + if (packet_length > ABSOLUTE_MAX_PACKET_SIZE) | ||
| 342 | + { | ||
| 343 | + throw ProtocolError("A client sends a packet claiming to be bigger than the maximum MQTT allows."); | ||
| 344 | + } | ||
| 345 | + | ||
| 338 | if (packet_length <= readbuf.usedBytes()) | 346 | if (packet_length <= readbuf.usedBytes()) |
| 339 | { | 347 | { |
| 340 | MqttPacket packet(readbuf, packet_length, fixed_header_length, sender); | 348 | MqttPacket packet(readbuf, packet_length, fixed_header_length, sender); |
mqttpacket.cpp
| @@ -17,6 +17,7 @@ MqttPacket::MqttPacket(CirBuf &buf, size_t packet_len, size_t fixed_header_lengt | @@ -17,6 +17,7 @@ MqttPacket::MqttPacket(CirBuf &buf, size_t packet_len, size_t fixed_header_lengt | ||
| 17 | fixed_header_length(fixed_header_length), | 17 | fixed_header_length(fixed_header_length), |
| 18 | sender(sender) | 18 | sender(sender) |
| 19 | { | 19 | { |
| 20 | + assert(packet_len > 0); | ||
| 20 | buf.read(bites.data(), packet_len); | 21 | buf.read(bites.data(), packet_len); |
| 21 | 22 | ||
| 22 | first_byte = bites[0]; | 23 | first_byte = bites[0]; |
| @@ -116,6 +117,9 @@ MqttPacket::MqttPacket(const PubAck &pubAck) : | @@ -116,6 +117,9 @@ MqttPacket::MqttPacket(const PubAck &pubAck) : | ||
| 116 | 117 | ||
| 117 | void MqttPacket::handle() | 118 | void MqttPacket::handle() |
| 118 | { | 119 | { |
| 120 | + if (packetType == PacketType::Reserved) | ||
| 121 | + throw ProtocolError("Packet type 0 specified, which is reserved and invalid."); | ||
| 122 | + | ||
| 119 | if (packetType != PacketType::CONNECT) | 123 | if (packetType != PacketType::CONNECT) |
| 120 | { | 124 | { |
| 121 | if (!sender->getAuthenticated()) | 125 | if (!sender->getAuthenticated()) |