Merge pull request 'Reject path-traversal archive entries and make the install swap cross-filesystem safe' (#2) from benvin/extract-and-swap-fixes into main

Reviewed-on: #2

.woodpecker/lint.yaml

@@ -9,7 +9,7 @@ steps:
       - luacheck lua plugin tests
     backend_options:
       kubernetes:
-        serviceAccountName: default
+        serviceAccountName: vimpack-ci
         resources:
           requests:
             memory: 512Mi
@@ -25,7 +25,7 @@ steps:
       - stylua --check lua plugin tests
     backend_options:
       kubernetes:
-        serviceAccountName: default
+        serviceAccountName: vimpack-ci
         resources:
           requests:
             memory: 512Mi

.woodpecker/pre-commit.yaml

@@ -10,7 +10,7 @@ steps:
       - pre-commit run --all-files --show-diff-on-failure
     backend_options:
       kubernetes:
-        serviceAccountName: default
+        serviceAccountName: vimpack-ci
         resources:
           requests:
             memory: 512Mi

.woodpecker/test.yaml

@@ -5,11 +5,11 @@ steps:
   - name: test
     image: alpine:3.21
     commands:
-      - apk add --no-cache neovim tar curl unzip
+      - apk add --no-cache neovim tar curl unzip zip
       - nvim --headless --clean -l tests/minimal.lua
     backend_options:
       kubernetes:
-        serviceAccountName: default
+        serviceAccountName: vimpack-ci
         resources:
           requests:
             memory: 512Mi

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 `<pack root>/.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.

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/<name>
   stdpath('data')/site/pack/vimpack/opt/<name>
 
 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.
 

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
 

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')