Skip to content

fix(filedata): cap aggregate offline archive extraction (SEC-9491) - #849

Open
kparkinson-ld wants to merge 2 commits into
v8from
devin/1788292311-sec-9491-archive-limits
Open

kparkinson-ld wants to merge 2 commits into
v8from
devin/1788292311-sec-9491-archive-limits

Conversation

@kparkinson-ld

@kparkinson-ld kparkinson-ld commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Caps offline archive extraction before unbounded aggregate data or entry counts can be written.

Closes SEC-9491.

  • Proposes a 1 GiB aggregate decompressed-size ceiling.
  • Proposes a 10,000-entry ceiling.
  • Retains the existing 200 MiB per-file ceiling.

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

SEC-9491

Implementation details

Describe the solution you've provided

readTar tracks the cumulative declared size and number of tar entries, rejecting the archive before extracting an entry that would exceed either ceiling. Boundary and rejection behavior are covered by unit tests.

Describe alternatives you've considered

A new customer-facing configuration surface was avoided because these are safety ceilings rather than operational tuning controls. The provisional constants can be adjusted during review without changing the implementation shape.

Additional context

Validated with make lint, make test, and the race-enabled internal/filedata tests.

Link to Devin session: https://app.devin.ai/sessions/52a2fa364bbe4af4ae3571fba69b2172
Open in Devin Desktop: https://app.devin.ai/desktop/session/52a2fa364bbe4af4ae3571fba69b2172?variant=devin
Requested by: @kparkinson-ld


Note

Overview
Adds aggregate safety limits on offline .tar/.tar.gz extraction so malicious or malformed archives cannot declare unbounded total uncompressed size or entry count before files are written.

readTar now delegates to readTarWithLimits, which counts every tar header and sums declared regular-file sizes against new ceilings (1 GiB total, 10,000 entries) while keeping the existing 200 MiB per-file io.CopyN cap. Over-limit archives fail early with new “malformed or malicious archive” errors.

Unit tests exercise rejection for aggregate size and entry count, plus acceptance at the exact limits via injectable limits in readTarWithLimits.

Reviewed by Cursor Bugbot for commit d852251. Bugbot is set up for automated code reviews on this repo. Configure here.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration

Copy link
Copy Markdown
Contributor

@keelerm84 Are the proposed ceilings—1 GiB aggregate decompressed data and 10,000 tar entries—safe for the largest legitimate customer offline archive? If not, what two values should this PR use?

@devin-ai-integration
devin-ai-integration Bot requested a review from a team September 1, 2026 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant