cpio: contain extraction within root using SecureJoin#3632
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3632 +/- ##
==========================================
+ Coverage 60.86% 60.88% +0.01%
==========================================
Files 659 659
Lines 46497 46533 +36
==========================================
+ Hits 28301 28332 +31
- Misses 18196 18201 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
rminnich
left a comment
rminnich
left a comment
There was a problem hiding this comment.
This is a lot of complexity, and I would prefer that you first check the safepath (recently added) support in upstream packages.
Can you check to see if that could be used instead of this custom code?
Extraction rebases each record name into rootDir with a lexical join, but a symlink record unpacked earlier can still redirect a later write outside the root (e.g. record `x` -> `/etc`, then record `x/passwd` lands in `/etc`), defeating the existing zipslip check. Resolve each record name with `securejoin.SecureJoin`, which clamps `..` and follows the symlinks already present under rootDir so the resolved path stays contained. This drops the custom path-walking code in favour of the upstream safepath helper. Signed-off-by: alhudz <al.hudz.k@gmail.com>
alhudz
force-pushed
the
cpio-symlink-escape
branch
from
eaf8604 to
42825e0
Compare
alhudz
changed the title
|
Good call. |
|
|
||
| securejoin "github.com/cyphar/filepath-securejoin" | ||
| "github.com/u-root/u-root/pkg/ls" | ||
| "github.com/u-root/u-root/pkg/upath" |
There was a problem hiding this comment.
Our AI policy require human in the loop. The issue is here, but fix don't make a lot of sense. If the problem is in upath.SafeFilepathJoin what a point to replace once occurrence of it?
3 participants
Repro: extract a cpio that stores a symlink record (x-> an absolute path, or../.., outside the destination) followed by a regular-file record namedx/foo; the file lands outsiderootDir.Cause:CreateFileInRootonly ran the record name through the lexicalSafeFilepathJoin, so a write through a symlink unpacked from an earlier record still followed it out ofrootDir.Fix: resolve each record name withsecurejoin.SecureJoin(already vendored), which clamps..and follows the symlinks already present underrootDir, so the resolved path stays contained. This drops the custom path-walking. Legitimate symlinks that stay within the root keep working.TestCreateFileInRootSymlinkEscapefails before this change and passes after;TestCreateFileInRootSymlinkWithinRootcovers the in-root case.