fix: guard against OOB read in FlutterStandardCodecHelper string decode path (iOS/macOS)#183996
fix: guard against OOB read in FlutterStandardCodecHelper string decode path (iOS/macOS)#183996artahir-dev wants to merge 1 commit intoflutter:masterfrom
Conversation
…wrapping CFData ReadDataNoCopy was calling CFDataCreateWithBytesNoCopy with the raw wire-claimed length without validating that enough bytes remain in the buffer. A crafted codec message with an inflated string size field (e.g. 0xFFFFFFFF) would produce a CFData "view" into unowned memory, causing CFStringCreateFromExternalRepresentation to read far beyond the actual buffer — OOB read and crash. The fix adds a remaining-bytes check in ReadDataNoCopy and makes FlutterStandardCodecHelperReadUTF8 treat a nil return as a decode failure rather than passing it to CFStringCreateFromExternalRepresentation. Three regression tests are added to flutter_standard_codec_unittest.mm: inflated size field, normally-encoded string truncated by one byte, and the message codec layer decode path.
github-actions
bot
added
platform-ios
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a critical out-of-bounds read vulnerability in FlutterStandardCodecHelper by adding necessary bounds checking. The changes are logical and well-supported by new regression tests. My feedback includes a suggestion to improve the readability and robustness of the bounds check and to strengthen the assertions in the new tests for better precision.
| CFIndex data_length = CFDataGetLength(data); | ||
| if ((CFIndex)*location > data_length || | ||
| length > (unsigned long)(data_length - (CFIndex)*location)) { | ||
| *location += length; | ||
| return nil; | ||
| } |
There was a problem hiding this comment.
The bounds check logic is correct, but it involves mixed signed/unsigned arithmetic and multiple casts which can be hard to reason about. Consider simplifying this by performing the check using unsigned integers. This improves readability and makes the logic less prone to subtle errors related to integer promotion and conversion rules.
const unsigned long data_length = CFDataGetLength(data);
if (*location >= data_length || length > data_length - *location) {
*location += length;
return nil;
}References
- Code should be optimized for readability, as it is read more often than it is written. The suggested change simplifies the conditional logic, making it easier to understand. (link)
| FlutterStandardReader* reader = | ||
| [[FlutterStandardReader alloc] initWithData:data]; | ||
| id result = [reader readValue]; | ||
| ASSERT_TRUE(result == nil || [result isKindOfClass:[NSString class]]); |
There was a problem hiding this comment.
The assertion is correct but could be more specific. For a malformed input that causes a decoding error, the expected outcome is nil. Strengthening the assertion to check specifically for nil makes the test's intent clearer and more robust against future changes that might incorrectly return a different value (like an empty string) on error.
ASSERT_TRUE(result == nil);
| NSData* message = [NSData dataWithBytes:bytes length:sizeof(bytes)]; | ||
| FlutterStandardMessageCodec* codec = [FlutterStandardMessageCodec sharedInstance]; | ||
| id decoded = [codec decode:message]; | ||
| ASSERT_TRUE(decoded == nil || [decoded isKindOfClass:[NSString class]]); |
There was a problem hiding this comment.
The assertion is correct but could be more specific. For a malformed input that causes a decoding error, the expected outcome is nil. Strengthening the assertion to check specifically for nil makes the test's intent clearer and more robust against future changes that might incorrectly return a different value on error.
ASSERT_TRUE(decoded == nil);
| NSData* full = [codec encode:@"hello"]; | ||
| NSData* truncated = [full subdataWithRange:NSMakeRange(0, full.length - 1)]; | ||
| id decoded = [codec decode:truncated]; | ||
| ASSERT_TRUE(decoded == nil || [decoded isKindOfClass:[NSString class]]); |
There was a problem hiding this comment.
The assertion is correct but could be more specific. For a malformed input that causes a decoding error, the expected outcome is nil. Strengthening the assertion to check specifically for nil makes the test's intent clearer and more robust against future changes that might incorrectly return a different value on error.
ASSERT_TRUE(decoded == nil);
1 participant
Fixes a crash in
FlutterStandardCodecHelperon iOS and macOS where a crafted platform channel message could cause the codec to read memory beyond the end of the buffer.ReadDataNoCopywas callingCFDataCreateWithBytesNoCopyusing the length field directly from the wire without first checking that enough bytes remain. That function doesn't validate the pointer arithmetic, so a message with an inflated size — for example akStringtype byte followed by0xFF 0xFF 0xFF 0xFF 0xFFencoding a claimed length of 4 GiB — would wrap a huge range of unowned heap memory in aCFData. WhenCFStringCreateFromExternalRepresentationthen tried to scan that "string" it would read far past the actual buffer and crash.The typed-data decode path already had this issue documented and worked around: there's a comment in
FlutterStandardCodec.mm(aroundReadTypedDataOfType) that saysCFDataCreateBytesNoCopy crashesand usessubdataWithRange:instead. That fix was never applied to the string path (ReadDataNoCopy/FlutterStandardCodecHelperReadUTF8), which is what this PR addresses.The change is small — a bounds check at the top of
ReadDataNoCopyand a nil guard inFlutterStandardCodecHelperReadUTF8. Three regression tests are added covering the inflated-size case, a normally-encoded string truncated by one byte, and theFlutterStandardMessageCodeclayer decode.Pre-launch Checklist
///).