extract: verify always runs, failures remove the extract, walk fails closed
Security review of52b1e65: 1. (HIGH) tarfile's stream could enumerate differently from GNU tar. Now extract.sh captures tar's exit code (`|| TAR_RC=$?`), ALWAYS runs the post-extract verify on whatever tar wrote, and any failure (tar, verify, either pre-extract scan) removes $DEST before exiting non-zero. The classifier also refuses (exit 3) on any stream exception and when its member count differs from GNU tar's own escape-quoted listing. Measured: a lone zero block mid-archive stops BOTH readers at the same member (no -i is used), so this is a guard, not a known differential. 2. (MEDIUM) os.walk swallowed unreadable directories. verify now passes onerror (recorded => fail) and refuses any directory that is not readable+searchable.52b1e65exited 0 on the unreadable fixture. 3. (MEDIUM) a refused tree stayed in the bind-mounted output. fail_extract opens up 0000 dirs (find -P -type d, never follows links), then rm -rf -- "$DEST". Fixtures: cpmove-bad-{unreadable,tarfail,truncated}; a fake tar that lists one extra member proves the count check. 14 checks fail on52b1e65. Docker: unreadable / tarfail runs end with status failed, failed_stage extract and no extract-work left in /host/sanitized/<id>. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
52b1e656c3
commit
d4a63fba0a
4 files changed
+161
-33
No files matched your search
@@ -31,13 +31,19 @@ mail tree is stripped (pre-extract) or fails the extract (post-extract).
|
||||
Post-extract authority. lstat-walks every cpmove-*/homedir/mail under
|
||||
<dest> (never following links) and exits 4 when it finds a symlink, or
|
||||
a regular file whose st_nlink is larger than the number of its names
|
||||
inside that same mail tree (some other name exists outside it).
|
||||
inside that same mail tree (some other name exists outside it), or any
|
||||
directory it cannot read+search (an uninspectable subtree fails).
|
||||
|
||||
scan also cross-checks its member count against `tar -tf` and refuses on
|
||||
any difference or stream error, so nothing GNU tar extracts goes
|
||||
unclassified (verify remains the authority either way).
|
||||
"""
|
||||
import json
|
||||
import os
|
||||
import posixpath
|
||||
import re
|
||||
import stat
|
||||
import subprocess
|
||||
import sys
|
||||
import tarfile
|
||||
|
||||
@@ -71,24 +77,47 @@ def scan(tar_path, excl_path, actions_path):
|
||||
doc = {'actions': [], 'count': 0}
|
||||
already = {a.get('path') for a in doc.get('actions', [])}
|
||||
found = []
|
||||
with tarfile.open(tar_path, 'r|*') as tf:
|
||||
for m in tf:
|
||||
for label, value in (('name', m.name), ('link target', m.linkname)):
|
||||
if CTRL.search(value or ''):
|
||||
refuse(3, f'member {label} contains a control character: {value!r}')
|
||||
name = canonical(m.name)
|
||||
if name is None:
|
||||
refuse(3, f'non-canonical member name: {m.name!r}')
|
||||
if m.issym():
|
||||
if owner(name):
|
||||
found.append((m.name, 'symlink', m.linkname))
|
||||
elif m.islnk():
|
||||
target = canonical(m.linkname)
|
||||
if target is None:
|
||||
refuse(3, f'non-canonical hard-link target: {m.name!r} -> {m.linkname!r}')
|
||||
a, b = owner(name), owner(target)
|
||||
if (a or b) and a != b:
|
||||
found.append((m.name, 'hardlink', m.linkname))
|
||||
seen = 0
|
||||
try:
|
||||
tf = tarfile.open(tar_path, 'r|*')
|
||||
except Exception as e:
|
||||
refuse(3, f'archive unreadable by the classifier: {e}')
|
||||
try:
|
||||
with tf:
|
||||
for m in tf:
|
||||
seen += 1
|
||||
for label, value in (('name', m.name), ('link target', m.linkname)):
|
||||
if CTRL.search(value or ''):
|
||||
refuse(3, f'member {label} contains a control character: {value!r}')
|
||||
name = canonical(m.name)
|
||||
if name is None:
|
||||
refuse(3, f'non-canonical member name: {m.name!r}')
|
||||
if m.issym():
|
||||
if owner(name):
|
||||
found.append((m.name, 'symlink', m.linkname))
|
||||
elif m.islnk():
|
||||
target = canonical(m.linkname)
|
||||
if target is None:
|
||||
refuse(3, f'non-canonical hard-link target: {m.name!r} -> {m.linkname!r}')
|
||||
a, b = owner(name), owner(target)
|
||||
if (a or b) and a != b:
|
||||
found.append((m.name, 'hardlink', m.linkname))
|
||||
except Exception as e: # TarError, EOFError, zlib.error ... (SystemExit passes through)
|
||||
refuse(3, f'archive stream error after {seen} member(s): {e}')
|
||||
# The classifier must have seen every member GNU tar will extract: a
|
||||
# stream that stops early (zero block, truncation, a header tarfile
|
||||
# skips) would leave members unclassified. Count GNU tar's own listing
|
||||
# (escape quoting = one line per member, even with a newline in a name).
|
||||
try:
|
||||
lst = subprocess.run(['tar', '--quoting-style=escape', '-tf', tar_path],
|
||||
stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=False)
|
||||
except OSError as e:
|
||||
refuse(3, f'cannot list the archive with tar: {e}')
|
||||
if lst.returncode != 0:
|
||||
refuse(3, f'tar -tf failed (rc={lst.returncode}): {lst.stderr.decode(errors="replace").strip()[:300]}')
|
||||
tar_count = lst.stdout.count(b'\n')
|
||||
if tar_count != seen:
|
||||
refuse(3, f'member count differs: classifier saw {seen}, tar lists {tar_count}')
|
||||
found = [f for f in found if f[0] not in already]
|
||||
with open(excl_path, 'a') as eh:
|
||||
for name, _, _ in found:
|
||||
@@ -108,7 +137,15 @@ def scan(tar_path, excl_path, actions_path):
|
||||
|
||||
def verify(dest):
|
||||
bad = []
|
||||
for top in sorted(os.listdir(dest)):
|
||||
|
||||
def walk_error(err):
|
||||
bad.append(f'cannot inspect: {os.path.relpath(getattr(err, "filename", "") or dest, dest)} ({err.strerror})')
|
||||
|
||||
try:
|
||||
tops = sorted(os.listdir(dest))
|
||||
except OSError as e:
|
||||
refuse(4, f'cannot list the extract: {e}')
|
||||
for top in tops:
|
||||
mail = os.path.join(dest, top, 'homedir', 'mail')
|
||||
if not top.startswith('cpmove-'):
|
||||
continue
|
||||
@@ -119,13 +156,23 @@ def verify(dest):
|
||||
if stat.S_ISLNK(st.st_mode):
|
||||
bad.append(f'symlink: {top}/homedir/mail')
|
||||
continue
|
||||
if stat.S_ISDIR(st.st_mode) and not os.access(mail, os.R_OK | os.X_OK):
|
||||
bad.append(f'directory not readable+searchable: {top}/homedir/mail')
|
||||
continue
|
||||
names = {} # (dev, ino) -> [count of names in this tree, st_nlink, a path]
|
||||
for root, dirs, files in os.walk(mail, followlinks=False):
|
||||
# Fail closed: os.walk silently skips what it can't read unless told.
|
||||
for root, dirs, files in os.walk(mail, followlinks=False, onerror=walk_error):
|
||||
for e in dirs + files:
|
||||
p = os.path.join(root, e)
|
||||
s = os.lstat(p)
|
||||
try:
|
||||
s = os.lstat(p)
|
||||
except OSError as err:
|
||||
walk_error(err)
|
||||
continue
|
||||
if stat.S_ISLNK(s.st_mode):
|
||||
bad.append(f'symlink: {os.path.relpath(p, dest)}')
|
||||
elif stat.S_ISDIR(s.st_mode) and not os.access(p, os.R_OK | os.X_OK):
|
||||
bad.append(f'directory not readable+searchable: {os.path.relpath(p, dest)}')
|
||||
elif stat.S_ISREG(s.st_mode) and s.st_nlink > 1:
|
||||
rec = names.setdefault((s.st_dev, s.st_ino), [0, s.st_nlink, p])
|
||||
rec[0] += 1
|
||||
@@ -136,7 +183,7 @@ def verify(dest):
|
||||
for b in bad:
|
||||
sys.stderr.write(f'mail-tree-links: {b}\n')
|
||||
if bad:
|
||||
refuse(4, f'{len(bad)} link(s) in the extracted mail tree')
|
||||
refuse(4, f'{len(bad)} problem(s) in the extracted mail tree (links or uninspectable directories)')
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
|
||||
Reference in new issue
Block a user