Buffer overflow in publishPacket

[pakokol]

The function uint16_t Adafruit_MQTT::publishPacket has buffer overflow if you send a packet that is larger than the MAXBUFFERSIZE (topic len + data len).

// as per http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc398718040
uint16_t Adafruit_MQTT::publishPacket(uint8_t *packet, const char *topic,
                                     uint8_t *data, uint16_t bLen, uint8_t qos) {
  uint8_t *p = packet;
  uint16_t len=0;

  // calc length of non-header data
  len += 2;               // two bytes to set the topic size
  len += strlen(topic); // topic length
  if(qos > 0) { 
    len += 2; // qos packet id
  }
  len += bLen; // payload length

  // Now you can start generating the packet!
  p[0] = MQTT_CTRL_PUBLISH << 4 | qos << 1;
  p++;

  // fill in packet[1] last
  do {
    uint8_t encodedByte = len % 128;
    len /= 128;
    // if there are more data to encode, set the top bit of this byte
    if ( len > 0 ) {
      encodedByte |= 0x80;
    }
    p[0] = encodedByte;
    p++;
  } while ( len > 0 );

  // topic comes before packet identifier
  p = stringprint(p, topic);

  // add packet identifier. used for checking PUBACK in QOS > 0
  if(qos > 0) {
    p[0] = (packet_id_counter >> 8) & 0xFF;
    p[1] = packet_id_counter & 0xFF;
    p+=2;

    // increment the packet id
    packet_id_counter++;
  }

  memmove(p, data, bLen);
  p+= bLen;
  len = p - packet;
  DEBUG_PRINTLN(F("MQTT publish packet:"));
  DEBUG_PRINTBUFFER(buffer, len);
  return len;
}

As you can see from the code there isn't any check if we already went past the packet size and there fore we write over the boundaries.

[brentru]

@pakokol Would you want to implement this? If not, I'll implement it in a new pull request.

[pakokol]

@brentru sorry for the late response I didn't got any notification that you answered here on github maybe I have accidentally turned off the notifications. I suppose I could implement this if you prefer me to do it.

[brentru]

@pakokol No problem, still up for it?

[pakokol]

@brentru sure why not. Just tell me how is the policity in fixing bugs. It is my first time to collaborate in a project on github :)

[S2Doc]

I edited MAXBUFFERSIZE to increase it to 1000.

Adafruit - A nice way to implement this might be: #if !defined(MAXBUFFERSIZE) #define MAXBUFFERSIZE 150 #endif

That would leave things as they are, but allow a user to change the buffer in their own code without editing the header file.

[flavio-fernandes]

@brentru and @pakokol : Please take a look at https://github.com/adafruit/Adafruit_MQTT_Library/pull/166 and see if that addresses your issue.

[jshep321]

I have an issue that might be related? If I publish an outgoing (client to adafruit.io) mqtt message that is >128 characters, it sends ok (as does follow-on outgoing messages). However the next incoming subscriber message causes coredump. My array is 256 characters, fyi. I am seeing coredumps after a certain amount of time with this, which I think is related.

127 character message is fine.

Note the length of the topic seems not relevant in my case.

[flavio-fernandes]

I have an issue that might be related? If I publish an outgoing (client to adafruit.io) mqtt message that is >128 characters, it sends ok (as does follow-on outgoing messages). However the next incoming subscriber message causes coredump. My array is 256 characters, fyi. I am seeing coredumps after a certain amount of time with this, which I think is related.

127 character message is fine.

Note the length of the topic seems not relevant in my case.

Yes, that sounds extremely familiar. Try using my fix to see if it helps with the crash. The packet will be truncated to the max length, but at least you should stop crashing:

https://github.com/adafruit/Adafruit_MQTT_Library/pull/166

Best,

-- flaviof

[jshep321]

Thanks! I took the dead simple approach in my outgoing mqtt publish function and implemented this to "fix" it: strncpy (messageToSend, messageIWantToSend, 127);