fix(im): correct 64-bit MP4 box size handling to prevent panic on crafted media#1165
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughValidates 64-bit MP4 "largesize" headers before computing box boundaries in findMP4Box, readMp4DurationBytes, and readMp4Duration, and adds tests that cover malformed largesize overflow and non-zero-offset 64-bit boxes. ChangesMP4 Box Overflow Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Actionable comments posted: 0 |
9b1beed to
3003007
Compare
…fted media The size==1 (64-bit "largesize") branch of all three MP4 box walkers (findMP4Box, readMp4DurationBytes, readMp4Duration) set boxEnd to the raw largesize instead of offset+largesize — even though the 32-bit branch right below correctly uses offset+size. Two consequences: - Correctness: for any MP4 that carries a 64-bit box size at a non-zero offset, the box walk is computed from the wrong end, so the moov/mvhd lookup is truncated and the media duration is silently lost. - Robustness/security (CWE-190): the unguarded uint64->int(64) conversion of a largesize with the high bit set yields a negative boxEnd. The in-memory walkers then assign it to offset and feed it back as a slice index (data[offset:]), panicking with "slice bounds out of range" and crashing the CLI on a crafted or corrupt MP4. This is reachable via URL-sourced IM media, whose bytes the caller does not control. Fix: compute boxEnd as offset+largesize (matching the 32-bit branch) and reject largesize values smaller than the 16-byte header or larger than the remaining input. Malformed media now honours the parsers' best-effort contract by returning 0/-1 instead of panicking, and the bounds guarantee the conversion can no longer overflow. Add regression tests covering both the overflow (must not panic) and a 64-bit box at a non-zero offset (must walk correctly). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3003007 to
b8976ad
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1165 +/- ##
==========================================
+ Coverage 68.77% 68.79% +0.01%
==========================================
Files 628 628
Lines 58670 58679 +9
==========================================
+ Hits 40353 40367 +14
+ Misses 15021 15014 -7
- Partials 3296 3298 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@b8976adc7a4f2ee3358119f35776f006a8d11804🧩 Skill updatenpx skills add lhfer/cli#fix/mp4-64bit-box-size-overflow -y -g |
|
Thanks for the fix. I reviewed the change and it looks good to me. The implementation correctly treats 64-bit MP4 The added regression tests cover the overflow/no-panic case and 64-bit boxes at non-zero offsets. Verified with:
LGTM, thanks for the careful fix. |
|
Thank you for the review! |
…fted media (larksuite#1165) The size==1 (64-bit "largesize") branch of all three MP4 box walkers (findMP4Box, readMp4DurationBytes, readMp4Duration) set boxEnd to the raw largesize instead of offset+largesize — even though the 32-bit branch right below correctly uses offset+size. Two consequences: - Correctness: for any MP4 that carries a 64-bit box size at a non-zero offset, the box walk is computed from the wrong end, so the moov/mvhd lookup is truncated and the media duration is silently lost. - Robustness/security (CWE-190): the unguarded uint64->int(64) conversion of a largesize with the high bit set yields a negative boxEnd. The in-memory walkers then assign it to offset and feed it back as a slice index (data[offset:]), panicking with "slice bounds out of range" and crashing the CLI on a crafted or corrupt MP4. This is reachable via URL-sourced IM media, whose bytes the caller does not control. Fix: compute boxEnd as offset+largesize (matching the 32-bit branch) and reject largesize values smaller than the 16-byte header or larger than the remaining input. Malformed media now honours the parsers' best-effort contract by returning 0/-1 instead of panicking, and the bounds guarantee the conversion can no longer overflow. Add regression tests covering both the overflow (must not panic) and a 64-bit box at a non-zero offset (must walk correctly).
Summary
The three MP4 box walkers in
shortcuts/im/helpers.gomishandle the 64-bit ("largesize") box-size form. They setboxEndto the rawlargesizeinstead ofoffset + largesize— even though the 32-bit branch directly below correctly usesoffset + size— and convert theuint64to a signed int without any bounds check. This silently loses the duration of MP4s that carry a 64-bit box size at a non-zero offset, and panics the CLI (slice bounds out of range) on a crafted or corrupt MP4 whoselargesizehas the high bit set. The in-memory walkers run on URL-sourced IM media, whose bytes the caller does not control, so a malformed download crashes the process instead of degrading gracefully.Changes
findMP4Box,readMp4DurationBytes,readMp4Duration: computeboxEnd = offset + largesize, mirroring the existing 32-bitoffset + size.largesizevalues smaller than the 16-byte 64-bit header or larger than the remaining input, so malformed media returns0/-1(the parsers' documented best-effort contract) instead of overflowinguint64 → int(64)into a negativeboxEndand panicking (CWE-190). The lower bound also guarantees the walk always makes forward progress.shortcuts/im/mp4_box_test.gocovering the overflow (must not panic) and a 64-bit box at a non-zero offset (must walk correctly).Test Plan
go test -race ./shortcuts/im/...passes. The added regression tests panic / fail on the pre-fix code and pass after the fix.go build ./...,go vet ./..., andgofmt -lare clean; no new dependencies (go.mod/go.sumunchanged).lark-cli <domain> <command>verification — this is an internal binary-parser fix with no change to any shortcut's flags, params, or request structure, so perAGENTS.mdno dry-run E2E is required; behavior is covered by the added unit tests.Related Issues
Summary by CodeRabbit
Bug Fixes
Tests