From 6e1b018adba381fc7f54228ca7803c68a79086cf Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 5 Sep 2026 16:59:11 +1000 Subject: [PATCH] Validate zip symlinks and backslash paths before extraction - treat `\` as a separator in safe_relative so `\abs` and `..\..\x` are refused - read zip symlink targets with `unzip -p` off the `unzip -Z -l` listing and reject escapes before unpacking --- README.md | 9 ++- doc/vimpack.txt | 10 ++- lua/vimpack/fs.lua | 167 ++++++++++++++++++++++++++++++++++----------- tests/fs_spec.lua | 85 +++++++++++++++++++++++ 4 files changed, 227 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index c83a184..d20c101 100644 --- a/README.md +++ b/README.md @@ -136,9 +136,12 @@ the UI stays responsive. 1. Resolve `(name, version)` to a URL and headers. 2. `curl` the archive into `stdpath('cache')/vimpack/archives/`. 3. Verify `sha256` when the spec carries one; a mismatch aborts with expected and got. -4. List every archive member and reject the archive outright if any entry is absolute, - escapes the destination via `..`, is a device or fifo, or is a link pointing outside - the destination. Nothing is written until the whole listing is clean. +4. List every archive member (`tar -tvf`, `unzip -Z -l`) and reject the archive outright + if any entry is absolute, escapes the destination via `..` or `..\`, is a device or + fifo, or is a link pointing outside the destination. A zip keeps its symlink targets + in the member data rather than the listing, so each one is read with `unzip -p`, which + writes to stdout and never to disk. Nothing is written until the whole listing is + clean; whatever lands is walked again afterwards as a backstop. 5. Extract into `/.vimpack-stage/`, hoisting a single wrapper directory. Staging lives under the pack root so the swap below is a same-filesystem rename; `stdpath('cache')` and `stdpath('data')` are frequently separate mounts. diff --git a/doc/vimpack.txt b/doc/vimpack.txt index 0da5f3e..b408bae 100644 --- a/doc/vimpack.txt +++ b/doc/vimpack.txt @@ -147,14 +147,20 @@ A literal `token` key is rejected at |vimpack-setup| time. 7. LAYOUT *vimpack-layout* stdpath('cache')/vimpack/archives/ downloaded archives - stdpath('cache')/vimpack/stage/ extraction staging area + stdpath('data')/site/pack/vimpack/.vimpack-stage/ extraction staging area stdpath('data')/vimpack/manifest.json installed name/version/sha256 stdpath('data')/site/pack/vimpack/start/ stdpath('data')/site/pack/vimpack/opt/ An install extracts to the staging area, hoists a single wrapper directory if the archive has one, then renames into place after parking any previous tree -aside. If the rename fails the previous tree is restored. +aside. If the rename fails the previous tree is restored. Staging sits under +the pack root so that rename stays on one filesystem. + +Every member is listed and validated before extraction: absolute paths, `..` +and `..\` escapes, devices, fifos and links pointing out of the destination +are all refused. Zip symlink targets are read with `unzip -p` (stdout only, +nothing written) because the zip listing does not carry them. Lazy loading is out of scope: `opt = true` only chooses the opt/ bucket. diff --git a/lua/vimpack/fs.lua b/lua/vimpack/fs.lua index 4a3cb7c..c30efa5 100644 --- a/lua/vimpack/fs.lua +++ b/lua/vimpack/fs.lua @@ -174,7 +174,10 @@ function M.safe_relative(path) if type(path) ~= 'string' or path == '' then return nil end - if path:sub(1, 1) == '/' or path:match('^%a:[/\\]') then + -- Archives written on Windows separate with `\`, so treat it as a separator + -- here too: otherwise `\abs.txt` and `..\..\x` would look like plain names. + path = path:gsub('\\', '/') + if path:sub(1, 1) == '/' or path:match('^%a:/') then return nil end local parts = {} @@ -194,10 +197,14 @@ end --- True when `target`, followed from the directory holding `entry`, lands --- outside the extraction root. local function link_escapes(entry, target) - if type(target) ~= 'string' or target == '' or target:sub(1, 1) == '/' then + if type(target) ~= 'string' or target == '' then return true end - local dir = entry:match('^(.*)/[^/]*$') + target = target:gsub('\\', '/') + if target:sub(1, 1) == '/' then + return true + end + local dir = entry:gsub('\\', '/'):match('^(.*)/[^/]*$') return M.safe_relative(dir and (dir .. '/' .. target) or target) == nil end @@ -224,15 +231,37 @@ local function parse_tar_listing(out) return entries end ---- Parse `unzip -Z1` output; zip symlinks are caught by the post-extract walk. +-- perms, version, os, size, text/binary, compressed size, method, date, time, name +local ZIP_ENTRY = '^(.)%S*%s+%d+%.%d+%s+%S+%s+%d+%s+%S+%s+%d+%s+%S+%s+%S+%s+%S+%s+(.+)$' +local ZIP_NOISE = { '^Archive:%s', '^Zip file size:%s', '^%d+ files?,', '^Empty zipfile%.?$' } + +--- Parse `unzip -Z -l` output into { name, kind } records. Symlink targets live +--- in the member data, not the listing; `resolve_zip_links` fills them in. local function parse_zip_listing(out) local entries = {} for line in (out or ''):gmatch('[^\n]+') do - entries[#entries + 1] = { kind = '-', name = line } + local kind, name = line:match(ZIP_ENTRY) + if kind then + entries[#entries + 1] = { kind = kind, name = name } + else + local noise = false + for _, pattern in ipairs(ZIP_NOISE) do + noise = noise or line:match(pattern) ~= nil + end + if not noise then + return nil, string.format('cannot parse archive listing line %q', line) + end + end end return entries end +--- Info-ZIP treats `*`, `?`, `[` and `\` in a member name as pattern syntax, so +--- escape them before handing a name back to unzip. +local function zip_filespec(name) + return (name:gsub('[%*%?%[\\]', '\\%0')) +end + --- Reject anything that would write outside the extraction root. Returns an --- error message naming the offending entry, or nil when every entry is safe. function M.check_entries(entries, archive) @@ -244,6 +273,8 @@ function M.check_entries(entries, archive) why = 'is absolute or escapes the destination directory' elseif entry.link and link_escapes(entry.name, entry.link) then why = string.format('is a symlink to %q outside the destination directory', entry.link) + -- Belt and braces: GNU tar already strips leading `/` and `..` from the + -- hard link targets it reports, so this only bites other tar flavours. elseif entry.hardlink and not M.safe_relative(entry.hardlink) then why = string.format('is a hard link to %q outside the destination directory', entry.hardlink) end @@ -277,6 +308,78 @@ local function spawn(cmd, cb) end end +--- Read the stored target of every zip symlink with `unzip -p`, which writes the +--- member to stdout and never to disk. Calls cb(ok, err) once each is filled in. +local function resolve_zip_links(archive, entries, cb) + local links = vim.tbl_filter(function(entry) + return entry.kind == 'l' + end, entries) + + local index = 0 + local function step() + index = index + 1 + local entry = links[index] + if not entry then + cb(true) + return + end + spawn({ 'unzip', '-p', archive, zip_filespec(entry.name) }, function(res, serr) + if not res then + cb(false, serr) + return + end + if res.code ~= 0 then + cb( + false, + string.format( + 'refusing to extract %s: cannot read the symlink target of entry %q (unzip exited %d)', + archive, + entry.name, + res.code + ) + ) + return + end + entry.link = res.stdout or '' + step() + end) + end + step() +end + +--- Run the unpack command, tearing `dest` down again on failure or when the +--- backstop walk still finds a symlink pointing out of it. +local function unpack_into(archive, dest, unpack, cb) + spawn(unpack, function(xres, xerr) + if not xres then + M.rm_rf(dest) + cb(false, xerr) + return + end + if xres.code ~= 0 then + M.rm_rf(dest) + local detail = string.format('(%s exited %d): %s', unpack[1], xres.code, vim.trim(xres.stderr or '')) + cb(false, string.format('extraction failed for %s %s', archive, detail)) + return + end + local name, target = escaping_symlink(dest) + if name then + M.rm_rf(dest) + cb( + false, + string.format( + 'refusing to extract %s: entry %q is a symlink to %q outside the destination directory', + archive, + name, + target + ) + ) + return + end + cb(true) + end) +end + --- Extract an archive into `dest` asynchronously. Calls cb(ok, err). Every --- member is enumerated and validated first, so a hostile archive is rejected --- before anything is written. @@ -287,7 +390,7 @@ function M.extract(archive, dest, cb) list = { 'tar', '-tvf', archive } unpack = { 'tar', '-xf', archive, '-C', dest } elseif kind == 'zip' then - list = { 'unzip', '-Z1', archive } + list = { 'unzip', '-Z', '-l', archive } unpack = { 'unzip', '-q', '-o', archive, '-d', dest } else cb(false, string.format('unsupported archive type for %s (expected .tar.*, .tgz or .zip)', archive)) @@ -312,41 +415,27 @@ function M.extract(archive, dest, cb) cb(false, string.format('extraction failed for %s: %s', archive, perr)) return end - local unsafe = M.check_entries(entries, archive) - if unsafe then - cb(false, unsafe) - return + local function unpack_validated() + local unsafe = M.check_entries(entries, archive) + if unsafe then + cb(false, unsafe) + return + end + M.mkdir_p(dest) + unpack_into(archive, dest, unpack, cb) end - M.mkdir_p(dest) - spawn(unpack, function(xres, xerr) - if not xres then - M.rm_rf(dest) - cb(false, xerr) - return - end - if xres.code ~= 0 then - M.rm_rf(dest) - local detail = string.format('(%s exited %d): %s', unpack[1], xres.code, vim.trim(xres.stderr or '')) - cb(false, string.format('extraction failed for %s %s', archive, detail)) - return - end - local name, target = escaping_symlink(dest) - if name then - M.rm_rf(dest) - cb( - false, - string.format( - 'refusing to extract %s: entry %q is a symlink to %q outside the destination directory', - archive, - name, - target - ) - ) - return - end - cb(true) - end) + if kind == 'zip' then + resolve_zip_links(archive, entries, function(ok, lerr) + if ok then + unpack_validated() + else + cb(false, lerr) + end + end) + else + unpack_validated() + end end) end diff --git a/tests/fs_spec.lua b/tests/fs_spec.lua index 1ddccaa..11b8007 100644 --- a/tests/fs_spec.lua +++ b/tests/fs_spec.lua @@ -142,6 +142,14 @@ describe('safe_relative', function() assert_nil(fsmod.safe_relative('a/../../evil')) assert_nil(fsmod.safe_relative('')) end) + + it('treats a backslash as a separator', function() + assert_nil(fsmod.safe_relative('\\absolute_backslash.txt')) + assert_nil(fsmod.safe_relative('..\\..\\..\\deep_escape.txt')) + assert_nil(fsmod.safe_relative('a\\..\\..\\evil')) + assert_nil(fsmod.safe_relative('C:\\windows\\evil')) + assert_eq('lua/plug.lua', fsmod.safe_relative('lua\\plug.lua')) + end) end) describe('extract', function() @@ -257,6 +265,83 @@ describe('extract', function() assert_match('plug/lua/escape', result.err) end) + local function sh(root, script) + vim.fn.system({ 'sh', '-c', 'cd ' .. vim.fn.shellescape(root) .. ' && ' .. script }) + return vim.v.shell_error == 0 + end + + -- A symlink out of the destination followed by a member written *through* it. + -- unzip would create the link and only then notice, so the whole archive has + -- to be refused off the listing, before a single byte lands. + it('refuses a zip that writes through an escaping symlink', function() + local root = tmpdir() + local made = sh( + root, + 'mkdir -p src && cd src && ln -s ../../../outside link && zip -qy ../evil.zip link' + .. ' && rm link && mkdir link && echo pwned > link/pwned.txt && zip -qgy ../evil.zip link/pwned.txt' + ) + if not made then + return + end + + local result = wait_extract(root .. '/evil.zip', root .. '/dest') + assert_true(not result.ok) + assert_match('refusing to extract', result.err) + assert_match('is a symlink to', result.err) + assert_match('../../../outside', result.err) + assert_true(not fsmod.exists(root .. '/dest')) + assert_true(not fsmod.exists(root .. '/outside')) + assert_true(not fsmod.exists(vim.fs.dirname(root) .. '/outside')) + end) + + it('refuses a zip whose symlink points at an absolute path', function() + local root = tmpdir() + if not sh(root, 'mkdir -p src/plug && ln -s /etc/passwd src/plug/escape && cd src && zip -qry ../abs.zip plug') then + return + end + + local result = wait_extract(root .. '/abs.zip', root .. '/dest') + assert_true(not result.ok) + assert_match('refusing to extract', result.err) + assert_match('plug/escape', result.err) + assert_true(not fsmod.exists(root .. '/dest')) + end) + + it('allows a zip symlink that stays inside the destination', function() + local root = tmpdir() + if + not sh( + root, + 'mkdir -p src/plug/lua && echo "return {}" > src/plug/lua/plug.lua' + .. ' && ln -s lua/plug.lua src/plug/alias && cd src && zip -qry ../ok.zip plug' + ) + then + return + end + + local result = wait_extract(root .. '/ok.zip', root .. '/dest') + assert_true(result.ok, tostring(result.err)) + assert_true(fsmod.exists(root .. '/dest/plug/alias')) + end) + + it('reads the symlink target of a member whose name looks like a glob', function() + local root = tmpdir() + if + not sh( + root, + 'mkdir -p src/plug && ln -s ../../../outside "src/plug/od[d]*link" && cd src && zip -qry ../glob.zip plug' + ) + then + return + end + + local result = wait_extract(root .. '/glob.zip', root .. '/dest') + assert_true(not result.ok) + assert_match('refusing to extract', result.err) + assert_match('is a symlink to', result.err) + assert_true(not fsmod.exists(root .. '/dest')) + end) + it('allows a symlink that stays inside the destination', function() local root = tmpdir() vim.fn.mkdir(root .. '/src/plug/lua', 'p')