Skip to content

Commit a5aee42

Browse files
icraggsicraggs
committed
Fix V5 properties reading of incorrect packets
1 parent f74ecbc commit a5aee42

3 files changed

Lines changed: 70 additions & 34 deletions

File tree

‎src/MQTTPacket.c‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1059,7 +1059,7 @@ int MQTTPacket_VBIlen(int rem_len)
10591059
* @param value the decoded length returned
10601060
* @return the number of bytes read from the socket
10611061
*/
1062-
int MQTTPacket_VBIdecode(int (*getcharfn)(char*, int), unsigned int* value)
1062+
int MQTTPacket_VBIdecode(int (*getcharfn)(char*, char*), char* enddata, unsigned int* value)
10631063
{
10641064
char c;
10651065
int multiplier = 1;
@@ -1073,12 +1073,15 @@ int MQTTPacket_VBIdecode(int (*getcharfn)(char*, int), unsigned int* value)
10731073

10741074
if (++len > MAX_NO_OF_REMAINING_LENGTH_BYTES)
10751075
{
1076-
rc = MQTTPACKET_READ_ERROR; /* bad data */
1076+
len = MQTTPACKET_READ_ERROR; /* bad data */
10771077
goto exit;
10781078
}
1079-
rc = (*getcharfn)(&c, 1);
1079+
rc = (*getcharfn)(&c, enddata);
10801080
if (rc != 1)
1081+
{
1082+
len = MQTTPACKET_READ_ERROR; /* bad data */
10811083
goto exit;
1084+
}
10821085
*value += (c & 127) * multiplier;
10831086
multiplier *= 128;
10841087
} while ((c & 128) != 0);
@@ -1089,19 +1092,21 @@ int MQTTPacket_VBIdecode(int (*getcharfn)(char*, int), unsigned int* value)
10891092

10901093
static char* bufptr;
10911094

1092-
int bufchar(char* c, int count)
1095+
int bufchar(char* c, char* enddata)
10931096
{
1094-
int i;
1097+
int rc = 1;
10951098

1096-
for (i = 0; i < count; ++i)
1099+
if (enddata - bufptr > 0)
10971100
*c = *bufptr++;
1098-
return count;
1101+
else
1102+
rc = -1;
1103+
return rc; /* 1 is good, any other value is an error */
10991104
}
11001105

11011106

1102-
int MQTTPacket_decodeBuf(char* buf, unsigned int* value)
1107+
int MQTTPacket_decodeBuf(char* buf, char* enddata, unsigned int* value)
11031108
{
11041109
bufptr = buf;
1105-
return MQTTPacket_VBIdecode(bufchar, value);
1110+
return MQTTPacket_VBIdecode(bufchar, enddata, value);
11061111
}
11071112

‎src/MQTTPacket.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,7 @@ unsigned int readInt4(char** pptr);
263263
void writeMQTTLenString(char** pptr, MQTTLenString lenstring);
264264
int MQTTLenStringRead(MQTTLenString* lenstring, char** pptr, char* enddata);
265265
int MQTTPacket_VBIlen(int rem_len);
266-
int MQTTPacket_decodeBuf(char* buf, unsigned int* value);
266+
int MQTTPacket_decodeBuf(char* buf, char* enddata, unsigned int* value);
267267

268268
#include "MQTTPacketOut.h"
269269

‎src/MQTTProperties.c‎

Lines changed: 55 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*******************************************************************************
2-
* Copyright (c) 2017, 2024 IBM Corp. and others
2+
* Copyright (c) 2017, 2026 IBM Corp. and others
33
*
44
* All rights reserved. This program and the accompanying materials
55
* are made available under the terms of the Eclipse Public License v2.0
@@ -248,33 +248,54 @@ int MQTTProperties_write(char** pptr, const MQTTProperties* properties)
248248
return rc;
249249
}
250250

251+
/**
252+
* Is there enough data remaining to read the supplied number of bytes
253+
* @param pptr current read position
254+
* @param enddata pointer to the last byte of the buffer
255+
* @param len number of bytes to be read
256+
* @return 1 if the data can be read, 0 if the buffer is too short
257+
*/
258+
static int is_enough_data(char** pptr, char* enddata, int len)
259+
{
260+
return (enddata - (*pptr)) >= len;
261+
}
251262

252263
int MQTTProperty_read(MQTTProperty* prop, char** pptr, char* enddata)
253264
{
254265
int type = -1,
255266
len = -1;
256267

257268
memset(prop, 0, sizeof(MQTTProperty));
269+
if (!is_enough_data(pptr, enddata, 1))
270+
goto exit;
258271
prop->identifier = readChar(pptr);
259272
type = MQTTProperty_getType(prop->identifier);
260273
if (type >= MQTTPROPERTY_TYPE_BYTE && type <= MQTTPROPERTY_TYPE_UTF_8_STRING_PAIR)
261274
{
262275
switch (type)
263276
{
264277
case MQTTPROPERTY_TYPE_BYTE:
278+
if (!is_enough_data(pptr, enddata, 1))
279+
goto exit;
265280
prop->value.byte = readChar(pptr);
266281
len = 1;
267282
break;
268283
case MQTTPROPERTY_TYPE_TWO_BYTE_INTEGER:
284+
if (!is_enough_data(pptr, enddata, 2))
285+
goto exit;
269286
prop->value.integer2 = readInt(pptr);
270287
len = 2;
271288
break;
272289
case MQTTPROPERTY_TYPE_FOUR_BYTE_INTEGER:
290+
if (!is_enough_data(pptr, enddata, 4))
291+
goto exit;
273292
prop->value.integer4 = readInt4(pptr);
274293
len = 4;
275294
break;
276295
case MQTTPROPERTY_TYPE_VARIABLE_BYTE_INTEGER:
277-
len = MQTTPacket_decodeBuf(*pptr, &prop->value.integer4);
296+
len = MQTTPacket_decodeBuf(*pptr, enddata, &prop->value.integer4);
297+
if (len <= 0)
298+
goto exit;
278299
*pptr += len;
279300
break;
280301
case MQTTPROPERTY_TYPE_BINARY_DATA:
@@ -308,61 +329,71 @@ int MQTTProperty_read(MQTTProperty* prop, char** pptr, char* enddata)
308329
break;
309330
}
310331
}
311-
return (len == -1) ? -1 : len + 1; /* 1 byte for identifier */
332+
exit:
333+
return (len == -1) ? -1 : len + 1; /* 1 byte for identifier */
312334
}
313335

314336

315-
int MQTTProperties_read(MQTTProperties* properties, char** pptr, char* enddata)
337+
int MQTTProperties_read(MQTTProperties *properties, char **pptr, char *enddata)
316338
{
317339
int rc = 0;
318-
unsigned int remlength = 0;
340+
int remlength = 0;
319341

320342
FUNC_ENTRY;
321343
/* we assume an initialized properties structure */
322344
if (enddata - (*pptr) > 0) /* enough length to read the VBI? */
323345
{
324346
int proplen = 0;
325347

326-
*pptr += MQTTPacket_decodeBuf(*pptr, &remlength);
348+
rc = MQTTPacket_decodeBuf(*pptr, enddata, (unsigned int*)&remlength);
349+
if (rc > 0)
350+
*pptr += rc;
351+
else
352+
{
353+
rc = MQTTPACKET_BUFFER_TOO_SHORT;
354+
goto exit;
355+
}
327356
properties->length = remlength;
328357
while (remlength > 0)
329358
{
330359
if (properties->count == properties->max_count)
331360
{
332-
properties->max_count += 10;
333-
if (properties->max_count == 10)
334-
properties->array = malloc(sizeof(MQTTProperty) * properties->max_count);
335-
else
336-
{
337-
void* newPtr = realloc(properties->array, sizeof(MQTTProperty) * properties->max_count);
338-
if (newPtr == NULL)
339-
{
340-
free(properties->array);
341-
properties->array = NULL;
342-
}
361+
properties->max_count += 10;
362+
if (properties->max_count == 10)
363+
properties->array = malloc(sizeof(MQTTProperty) * properties->max_count);
343364
else
344365
{
345-
properties->array = newPtr;
366+
void *newPtr = realloc(properties->array, sizeof(MQTTProperty) * properties->max_count);
367+
if (newPtr == NULL)
368+
{
369+
free(properties->array);
370+
properties->array = NULL;
371+
} else
372+
{
373+
properties->array = newPtr;
374+
}
346375
}
347376
}
348-
}
349377
if (properties->array == NULL)
350378
{
351-
rc = PAHO_MEMORY_ERROR;
379+
rc = PAHO_MEMORY_ERROR;
352380
goto exit;
353381
}
354382
if ((proplen = MQTTProperty_read(&properties->array[properties->count], pptr, enddata)) > 0)
355-
remlength -= proplen;
383+
remlength -= proplen;
356384
else
357-
break;
385+
{
386+
rc = MQTTPACKET_BAD;
387+
break;
388+
}
358389
properties->count++;
359390
}
360391
if (remlength == 0)
361-
rc = 1; /* data read successfully */
392+
rc = 1; /* data read successfully */
362393
}
363394

364395
if (rc != 1 && properties->array != NULL)
365-
MQTTProperties_free(properties);
396+
MQTTProperties_free(properties);
366397

367398
exit:
368399
FUNC_EXIT_RC(rc);

0 commit comments

Comments
 (0)