fix(bplist): reject implausible element count — unbounded-allocation DoS on a crafted plist#79
Merged
Merged
Conversation
…-allocation DoS bplist_decode parses an untrusted binary plist (iOS / macOS forensics loot, dumped preference files). A string / array / set / dict object encodes its element count in a size marker that, when the low nibble is 0xF, is an extended integer up to int64-max. That count flowed into the per-object span computations unbounded: - refs(): need = n * objectRefSize - object() UTF-16 string: bytesAt(p, n*2) With a large n (e.g. 2^63-1) and objectRefSize=2, n*objectRefSize overflows uint64 and wraps to a small value, so the `p + need > len(d.b)` range check passes — and then make([]uint64, n) (or make([]uint16, n) / make(map, n)) allocates the ORIGINAL huge n. Decode has no recover, so a crafted plist as small as 51 bytes triggers an uncaught `makeslice: len out of range` panic (or, for other size/refSize combinations, an OOM), crashing the agent turn. The existing numObjects <= len(data) trailer check does not help — it bounds the object *count*, not a single object's declared element count. The decoder's fuzz harness never crafted a deep-enough valid skeleton (header + offset table + trailer + an extended-count object) to reach it. Fixed at the source: sizedCount now rejects a count greater than the file size. No object can hold more elements than there are bytes on disk (a data byte costs 1, a UTF-16 char 2, an object ref objectRefSize>=1), so the bound never rejects a legitimate plist; it does keep n <= len(d.b), so the n*2 / n*objectRefSize spans can no longer overflow and every downstream make is bounded by the input size. Regression test crafts the minimal repro (header + one array object with a 2^63-1 extended count + a one-entry offset table + a trailer with objectRefSize=2) and asserts Decode returns an error instead of panicking; verified red->green (the pre-fix code panics makeslice, the fixed code errors). Sibling sweep of the count*size-overflow-before-make class: internal/wsframe bounds the payload against len(input) before allocating, internal/quic's CRYPTO length is a <=2^62 varint that cannot overflow the uint64 check, and internal/subghz.BytesToBits has no non-test callers — bplist was the only live instance. gofmt / go vet / golangci-lint (0 issues) / go test -race + 15s fuzz all green; legitimate plists still decode.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
bplist_decodeparses an untrusted binary plist (iOS/macOS forensics loot, dumped preference files). A string/array/set/dict object encodes its element count in a size marker that, when the low nibble is0xF, is an extended integer up to int64-max. That count flowed into the per-object span computations unbounded:refs():need = n * objectRefSizebytesAt(p, n*2)With a large
n(e.g.2^63-1) andobjectRefSize=2,n*objectRefSizeoverflows uint64 and wraps small, so thep + need > len(d.b)range check passes — thenmake([]uint64, n)(ormake([]uint16, n)/make(map, n)) allocates the original huge n.Decodehas no recover, so a crafted plist as small as 51 bytes triggers an uncaughtmakeslice: len out of rangepanic (or an OOM), crashing the agent turn.The existing
numObjects <= len(data)trailer check doesn't help — it bounds the object count, not a single object's declared element count. The fuzz harness never crafted a deep-enough valid skeleton to reach it.Fix
sizedCountnow rejects a count greater than the file size. No object can hold more elements than there are bytes on disk (a data byte costs 1, a UTF-16 char 2, an object refobjectRefSize>=1), so the bound never rejects a legitimate plist — but it keepsn <= len(d.b), so then*2/n*objectRefSizespans can no longer overflow and every downstreammakeis bounded by the input size.Verification
Regression test crafts the minimal repro (header + one array object with a
2^63-1extended count + one-entry offset table + trailer withobjectRefSize=2) and assertsDecodereturns an error instead of panicking — red→green proven (pre-fix panicsmakeslice, fixed code errors).Sibling sweep of the
count*size-overflow-before-makeclass:internal/wsframebounds the payload againstlen(input)first,internal/quic's CRYPTO length is a<=2^62varint that can't overflow the uint64 check, andinternal/subghz.BytesToBitshas no non-test callers — bplist was the only live instance.gofmt/go vetclean ·golangci-lint run ./...→ 0 issuesgo test -race ./internal/bplist/→ok; 15s fuzz clean; legitimate plists still decode.No registry/risk change (internal decoder hardening).