fix(controller): block tar path-traversal on extraction (D11)#15
Merged
Merged
Conversation
luisguzman-adfa
force-pushed
the
fix/phase1-security-d11-archive-traversal
branch
from
13e01a7 to
cbfb358
Compare
TarExtractor extracted archives with `tar -xf -C destDir` without validating member names. The "Import backup" flow accepts an arbitrary file and "restore" extracts it into filesDir/rootfs, so a crafted .tar.gz using `../` or absolute member names could escape the destination and overwrite app files (tech-debt D11 path-traversal). - domain: new pure ArchiveEntry.escapesRoot(name) — rejects absolute paths and `..` segments that climb above the root; allows benign `./` and internal `a/../b`. Pure JVM, unit-tested (ArchiveEntryTest). - TarExtractor: pre-list the archive members (tar -t; gzip decompressed in Java and piped) and fail closed before extracting if any member escapes. Runs for every extraction (the D6-verified rootfs install included) as defense in depth. - DeployFragment: single-quote the interpolated paths in the backup-creation `sh -c` pipe (the "unquoted backup pipe" half of D11). Note: legitimate distro rootfs / backup members are relative by convention, so the guard should not false-positive; worth confirming on a real install/restore.
luisguzman-adfa
force-pushed
the
fix/phase1-security-d11-archive-traversal
branch
from
cbfb358 to
cbfa6c9
Compare
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
1 participant
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.
Summary
Phase 1 (security hardening).
TarExtractorextracted archives withtar -xf -C destDirwithout validating member names. The Import backup flow accepts an arbitrary file (*/*) and restore extracts it intofilesDir/rootfs, so a crafted.tar.gzwhose members use../or absolute paths could escape the destination and overwrite app files — tech-debt item D11 (path-traversal / "tar slip").Changes
deploy/domain/ArchiveEntry(new, pure JVM):escapesRoot(name)rejects absolute paths and..segments that climb above the destination root; allows benign./and an internala/../bthat stays inside. Unit-tested (ArchiveEntryTest).TarExtractor: pre-lists the archive members (tar -t; gzip decompressed in Java and piped) and fails closed — if any member escapes, it aborts before extracting anything. Runs for every extraction (the D6-verified rootfs install included) as defense in depth.DeployFragment: single-quoted the interpolated paths in the backup-creationsh -cpipe (the "unquoted backup pipe" half of D11).Design notes (per discussion)
We chose safe-by-default for all extractions (not only the untrusted restore). The guard focuses on the real attack —
..escape and absolute members — mirroring tar's own "strip leading slash, forbid climbing out" model, so it does not false-positive on legitimate relative distro rootfs / backup members.How to identify it in the build / behaviour
../../evilor/etc/x) now fails the restore (Unsafe archive entry (path traversal): …) instead of writing outside the rootfs.Testing
ArchiveEntryTest(pure JVM, runs in the CItestDebugUnitTestgate): allows normal relative entries; blocks../, absolute, and backslash-normalized traversal. Logic also verified out of band (15 cases).