Skip to content

Commit 578a488

Browse files
pwithnallpwithnall
committed
gdbusmessage: Fix types of integer arithmetic in message length calculation
This fixes the addition happening with type `uint32_t`, rather than the expected `gssize`. Because of the narrower type, this could have resulted in integer overflow for large header lengths, bypassing the message length check lower down in the function. This is not exploitable in practice unless communicating peer-to-peer with a malicious D-Bus peer, as invalid messages like this would be filtered out by the message bus (such as `dbus-daemon`). Rework the arithmetic to use checked arithmetic functions into a `size_t` (largely as suggested by Simon McVittie), and add some unit tests. Simply doing the arithmetic in the `gssize` domain would not fix the issue on 32-bit systems. Based on a report by Koda Reef, albeit reverse engineered as the report did not contain a proof of concept of the vulnerability. Signed-off-by: Philip Withnall <pwithnall@gnome.org> Fixes: #3933
1 parent 1676bde commit 578a488

2 files changed

Lines changed: 36 additions & 18 deletions

File tree

‎gio/gdbusmessage.c‎

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2273,50 +2273,52 @@ g_dbus_message_bytes_needed (guchar *blob,
22732273
gsize blob_len,
22742274
GError **error)
22752275
{
2276-
gssize ret;
2277-
2278-
ret = -1;
2276+
uint32_t header_len, body_len;
2277+
size_t ret;
22792278

22802279
g_return_val_if_fail (blob != NULL, -1);
22812280
g_return_val_if_fail (error == NULL || *error == NULL, -1);
22822281
g_return_val_if_fail (blob_len >= 16, -1);
22832282

22842283
if (blob[0] == 'l')
22852284
{
2286-
/* core header (12 bytes) + ARRAY of STRUCT of (BYTE,VARIANT) */
2287-
ret = 12 + 4 + GUINT32_FROM_LE (((guint32 *) blob)[3]);
2288-
/* round up so it's a multiple of 8 */
2289-
ret = 8 * ((ret + 7)/8);
2290-
/* finally add the body size */
2291-
ret += GUINT32_FROM_LE (((guint32 *) blob)[1]);
2285+
header_len = GUINT32_FROM_LE (((guint32 *) blob)[3]);
2286+
body_len = GUINT32_FROM_LE (((guint32 *) blob)[1]);
22922287
}
22932288
else if (blob[0] == 'B')
22942289
{
2295-
/* core header (12 bytes) + ARRAY of STRUCT of (BYTE,VARIANT) */
2296-
ret = 12 + 4 + GUINT32_FROM_BE (((guint32 *) blob)[3]);
2297-
/* round up so it's a multiple of 8 */
2298-
ret = 8 * ((ret + 7)/8);
2299-
/* finally add the body size */
2300-
ret += GUINT32_FROM_BE (((guint32 *) blob)[1]);
2290+
header_len = GUINT32_FROM_BE (((guint32 *) blob)[3]);
2291+
body_len = GUINT32_FROM_BE (((guint32 *) blob)[1]);
23012292
}
23022293
else
23032294
{
23042295
g_set_error (error,
23052296
G_IO_ERROR,
23062297
G_IO_ERROR_INVALID_ARGUMENT,
23072298
"Unable to determine message blob length - given blob is malformed");
2299+
return -1;
23082300
}
23092301

2310-
if (ret > (1<<27))
2302+
/* core header (12 bytes) + array length (4 bytes) + length of array.
2303+
* Array elements are tuples: (byte, variant) */
2304+
ret = 0;
2305+
if (!g_size_checked_add (&ret, 12 + 4, header_len) ||
2306+
/* round up so it's a multiple of 8: ret = 8 * ((ret + 7)/8) */
2307+
!g_size_checked_add (&ret, ret, 7) ||
2308+
!g_size_checked_mul (&ret, 8, ret/8) ||
2309+
/* finally add the body size */
2310+
!g_size_checked_add (&ret, ret, body_len) ||
2311+
ret > (1 << 27))
23112312
{
23122313
g_set_error (error,
23132314
G_IO_ERROR,
23142315
G_IO_ERROR_INVALID_ARGUMENT,
23152316
"Blob indicates that message exceeds maximum message length (128MiB)");
2316-
ret = -1;
2317+
return -1;
23172318
}
23182319

2319-
return ret;
2320+
g_assert (ret <= G_MAXSSIZE);
2321+
return (gssize) ret;
23202322
}
23212323

23222324
/* ---------------------------------------------------------------------------------------------------- */

‎gio/tests/gdbus-message.c‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,22 @@ message_bytes_needed (void)
185185
0, 0, 0, 0x08, /* body length (128MiB) */
186186
1, 0, 0, 0, /* message serial */
187187
7, 0, 0, 0 /* header length */}, -1 },
188+
{ { 'B', 0, 0, 1, /* endianness, message type, flags, protocol version */
189+
0, 0, 0, 0, /* body length (empty) */
190+
1, 0, 0, 0, /* message serial */
191+
0xff, 0xff, 0xff, 0xf0 /* header length (overflow) */}, -1 },
192+
{ { 'l', 0, 0, 1, /* endianness, message type, flags, protocol version */
193+
0, 0, 0, 0, /* body length (empty) */
194+
1, 0, 0, 0, /* message serial */
195+
0xf0, 0xff, 0xff, 0xff /* header length (overflow) */}, -1 },
196+
{ { 'B', 0, 0, 1, /* endianness, message type, flags, protocol version */
197+
0, 0, 0, 1, /* body length (short) */
198+
1, 0, 0, 0, /* message serial */
199+
0xff, 0xff, 0xff, 0xef /* header length (overflow once body is added) */}, -1 },
200+
{ { 'l', 0, 0, 1, /* endianness, message type, flags, protocol version */
201+
1, 0, 0, 0, /* body length (short) */
202+
1, 0, 0, 0, /* message serial */
203+
0xef, 0xff, 0xff, 0xff /* header length (overflow once body is added) */}, -1 },
188204
};
189205
gsize i;
190206

0 commit comments

Comments
 (0)