Skip to content

Commit 3b13a4f

Browse files
DTW-ThalionDTW-Thalion
authored
Fix unsigned integer overflow in binary plist offset-table bounds check (#596)
* Fix unsigned integer overflow in binary plist offset-table bounds check * Source/NSPropertyList.m: Reformulate the offset-table sanity check in GSBinaryPLParser -initWithData:mutability: to avoid unsigned integer overflow in the multiplication. The prior form `table_start + object_count * offset_size > _length` overflows on 32-bit unsigned whenever object_count * offset_size crosses 2^32; with offset_size already bounded to 1..4 by the guard above, an attacker-controlled object_count near 2^30 wraps the product past 2^32 and lets a malformed trailer slip through, after which -offsetForIndex: reads past _bytes. Rewritten as `table_start > _length || object_count > (_length - table_start) / offset_size` which never forms the product and keeps the subtraction from underflowing at the check site, so correctness no longer depends on the ordering of sibling branches. * Tests/base/NSPropertyList/TestInfo: New test group marker. * Tests/base/NSPropertyList/bplist-overflow-bounds.m: New regression test. Four assertions: two positive-control round-trips of a legitimately serialized bplist, plus two crafted trailers with object_count=0x40000000 and 0xffffffff against offset_size=4 that wrap the pre-fix product and would have been accepted. Written in plain main() with conditions inline in PASS(); uses NS_DURING only around the parser call itself because the bplist branch of +propertyListWithData:options:format:error: raises NSException on malformed input rather than populating the error out-parameter. Validated on Ubuntu 24.04 with clang 18 + libobjc2. With the fix applied, all 4 assertions pass. With the source change reverted but the test left in place, the 2 overflow-attack assertions fail (the pre-fix sum check accepts both wrapped trailers) while both positive-control assertions still pass, confirming each assertion actually discriminates the fixed and unfixed builds. * Apply review feedback from PR #596 * Source/NSPropertyList.m: Shorten the block comment above the rewritten bounds check to a note about the unsigned-overflow hazard and a pointer to the regression test. * Tests/base/NSPropertyList/bplist-overflow-bounds.m: Use PASS_EXCEPTION(NSGenericException) instead of hand-rolled NS_DURING/NS_HANDLER around the parser call, and PASS_EQUAL for the positive-control round-trip. PASS already catches stray exceptions, so neither helper function needs a try/catch wrapper. No change to the fix itself. Re-validated on Ubuntu 24.04 + clang 18: 4/4 pass with fix, 2/4 pass without (both overflow attacks fail as designed, both positive controls still pass).
1 parent 8d5bc8f commit 3b13a4f

4 files changed

Lines changed: 170 additions & 1 deletion

File tree

‎ChangeLog‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,26 @@
1+
2026-04-14 Todd White <todd.white@thalion.global>
2+
3+
* Source/NSPropertyList.m: Fix unsigned integer overflow in the
4+
binary property list offset-table bounds check. The old form
5+
`table_start + object_count * offset_size > _length` overflows
6+
on 32-bit unsigned for attacker-controlled object_count near
7+
2^30 (offset_size is bounded to 1..4 by the preceding guard),
8+
wrapping the product past 2^32 so the sum lands below _length
9+
even when the offset table runs past the end of the buffer;
10+
offsetForIndex: then reads past _bytes. Reformulated as
11+
`table_start > _length
12+
|| object_count > (_length - table_start) / offset_size`
13+
which is overflow-free and self-contained (the leading
14+
table_start > _length test guards the subtraction from
15+
underflowing, removing the previous implicit dependence on the
16+
next-in-chain table_start > _length - 32 branch).
17+
* Tests/base/NSPropertyList/bplist-overflow-bounds.m: New
18+
regression test that crafts malformed bplist trailers directly
19+
and verifies they are rejected, with a positive control that
20+
round-trips a legitimately serialized bplist.
21+
Analysis and code from Todd White <todd.white@thalion.global>
22+
using Claude.
23+
124
2026-04-14 Richard Frith-Macdonald <rfm@gnu.org>
225

326
* Source/NSJSONSerialization.m: Implementation of nesting limit when

‎Source/NSPropertyList.m‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3085,7 +3085,13 @@ - (id) initWithData: (NSData*)plData
30853085
[NSException raise: NSGenericException
30863086
format: @"Unknown table size %d", saved];
30873087
}
3088-
else if (table_start + object_count * offset_size > _length)
3088+
/* The obvious form of the bound,
3089+
* table_start + object_count * offset_size > _length,
3090+
* overflows on unsigned multiplication; take care when editing.
3091+
* See Tests/base/NSPropertyList/bplist-overflow-bounds.m.
3092+
*/
3093+
else if (table_start > _length
3094+
|| object_count > (_length - table_start) / offset_size)
30893095
{
30903096
DESTROY(self); // Bad format
30913097
[NSException raise: NSGenericException

‎Tests/base/NSPropertyList/TestInfo‎

Whitespace-only changes.

‎Tests/base/NSPropertyList/bplist-overflow-bounds.m‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
/*
2+
* bplist-overflow-bounds.m - regression test for the binary property
3+
* list offset-table bounds check.
4+
*
5+
* GSBinaryPLParser must confirm that the offset table does not run
6+
* past the end of the supplied data before it calls -offsetForIndex:.
7+
* The obvious bound,
8+
*
9+
* table_start + object_count * offset_size > _length
10+
*
11+
* wraps on 32-bit unsigned for any attacker-controlled object_count
12+
* whose product with offset_size crosses 2^32. Because offset_size
13+
* is already bounded to 1..4 by an earlier guard, object_counts near
14+
* or above 2^30 are enough to wrap the sum back below _length, so a
15+
* crafted trailer that declares billions of entries over a handful
16+
* of bytes slips through the check and -offsetForIndex: then reads
17+
* past the end of the buffer. The rewritten bound divides the
18+
* non-negative difference (_length - table_start) by offset_size and
19+
* so never forms the overflowing product.
20+
*
21+
* These trailers cannot be produced by +dataWithPropertyList:; the
22+
* test assembles them directly as raw bytes.
23+
*/
24+
25+
#import <Foundation/Foundation.h>
26+
#import "ObjectTesting.h"
27+
28+
/* Build a raw binary-plist buffer with a caller-specified trailer.
29+
* Byte 8 holds a single bplist-encoded YES so that the root index
30+
* points at a nominal object; the rest of the body is zero fill.
31+
* GSBinaryPLParser reads object_count / root_index / table_start
32+
* from the lower four bytes of each 8-byte trailer field, so the
33+
* helper only threads 32-bit values through.
34+
*/
35+
static NSData *
36+
craftBplist(uint32_t object_count,
37+
uint8_t offset_size,
38+
uint8_t index_size,
39+
uint32_t root_index,
40+
uint32_t table_start,
41+
unsigned total_length)
42+
{
43+
NSMutableData *d;
44+
unsigned char *b;
45+
unsigned char postfix[32];
46+
47+
if (total_length < 33)
48+
{
49+
total_length = 33;
50+
}
51+
d = [NSMutableData dataWithLength: total_length];
52+
b = (unsigned char *)[d mutableBytes];
53+
54+
memcpy(b, "bplist00", 8);
55+
b[8] = 0x09;
56+
57+
memset(postfix, 0, sizeof(postfix));
58+
postfix[6] = offset_size;
59+
postfix[7] = index_size;
60+
postfix[12] = (unsigned char)((object_count >> 24) & 0xff);
61+
postfix[13] = (unsigned char)((object_count >> 16) & 0xff);
62+
postfix[14] = (unsigned char)((object_count >> 8) & 0xff);
63+
postfix[15] = (unsigned char)( object_count & 0xff);
64+
postfix[20] = (unsigned char)((root_index >> 24) & 0xff);
65+
postfix[21] = (unsigned char)((root_index >> 16) & 0xff);
66+
postfix[22] = (unsigned char)((root_index >> 8) & 0xff);
67+
postfix[23] = (unsigned char)( root_index & 0xff);
68+
postfix[28] = (unsigned char)((table_start >> 24) & 0xff);
69+
postfix[29] = (unsigned char)((table_start >> 16) & 0xff);
70+
postfix[30] = (unsigned char)((table_start >> 8) & 0xff);
71+
postfix[31] = (unsigned char)( table_start & 0xff);
72+
73+
memcpy(b + total_length - 32, postfix, 32);
74+
return d;
75+
}
76+
77+
int
78+
main(int argc, char *argv[])
79+
{
80+
START_SET("NSPropertyList binary plist offset-table bounds")
81+
NSDictionary *valid;
82+
NSData *serialized;
83+
NSData *crafted;
84+
85+
/* Positive control: a legitimately serialized bplist must still
86+
* round-trip. Guards against a fix that tightens the check too
87+
* far and starts rejecting valid input.
88+
*/
89+
valid = [NSDictionary dictionaryWithObjectsAndKeys:
90+
@"value", @"key", nil];
91+
serialized = [NSPropertyListSerialization
92+
dataWithPropertyList: valid
93+
format: NSPropertyListBinaryFormat_v1_0
94+
options: 0
95+
error: NULL];
96+
PASS(serialized != nil, "valid dictionary serialized as bplist")
97+
PASS_EQUAL([NSPropertyListSerialization
98+
propertyListWithData: serialized
99+
options: NSPropertyListImmutable
100+
format: NULL
101+
error: NULL],
102+
valid,
103+
"valid bplist round-trips through the parser")
104+
105+
/* Attack 1: object_count = 0x40000000, offset_size = 4. On 32-bit
106+
* unsigned the product wraps to zero, which would defeat the
107+
* pre-fix naive sum check. The buffer is intentionally short so
108+
* that even a one-entry table walk is out of bounds.
109+
*/
110+
crafted = craftBplist(0x40000000u, 4, 1,
111+
/*root_index*/ 0,
112+
/*table_start*/ 9,
113+
/*total_length*/ 64);
114+
PASS_EXCEPTION(([NSPropertyListSerialization
115+
propertyListWithData: crafted
116+
options: NSPropertyListImmutable
117+
format: NULL
118+
error: NULL]),
119+
NSGenericException,
120+
"object_count=0x40000000 offset_size=4 (product wraps to 0) rejected")
121+
122+
/* Attack 2: the maximum 32-bit object_count with the same
123+
* offset_size. The product wraps to 4 * 2^32 - 4, a small value
124+
* that again defeats the naive sum check.
125+
*/
126+
crafted = craftBplist(0xffffffffu, 4, 1,
127+
/*root_index*/ 0,
128+
/*table_start*/ 9,
129+
/*total_length*/ 64);
130+
PASS_EXCEPTION(([NSPropertyListSerialization
131+
propertyListWithData: crafted
132+
options: NSPropertyListImmutable
133+
format: NULL
134+
error: NULL]),
135+
NSGenericException,
136+
"object_count=0xffffffff offset_size=4 rejected")
137+
138+
END_SET("NSPropertyList binary plist offset-table bounds")
139+
return 0;
140+
}

0 commit comments

Comments
 (0)