extract: verify always runs, failures remove the extract, walk fails closed

Security review of 52b1e65:
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. 52b1e65 exited 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 on 52b1e65.
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>

scripts/extract.sh

@@ -36,6 +36,24 @@ log() { printf '[%s] extract: %s\n' "$(ts)" "$*"; }
 [[ -f "$TARBALL" ]] || { log "tarball not found: $TARBALL"; exit 2; }
 mkdir -p "$DEST"
 
+# Any failure after this point removes the (partial) extract before exiting:
+# it is the container's own scratch dir inside the bind-mounted output, and a
+# half-extracted tree, or one the link check refused, must not be left
+# behind for anything to read. Directories the archive made unreadable are
+# opened up first (find -P never follows links; -type d skips them) so rm
+# can descend.
+fail_extract() {
+    local code="$1"; shift
+    log "$*"
+    cd /
+    if [[ -n "$DEST" && "$DEST" != "/" && -d "$DEST" ]]; then
+        find -P "$DEST" -type d ! -perm -u=rwx -exec chmod u+rwx {} \; 2>/dev/null || true
+        rm -rf -- "$DEST" || log "WARN: could not remove the failed extract $DEST"
+        log "removed the failed extract $DEST"
+    fi
+    exit "$code"
+}
+
 # --- pre-extract symlink scan ---------------------------------------------
 
 log "scanning tarball for dangerous symlinks (cpmove vector check)"
@@ -44,9 +62,8 @@ if ! php "$SCRIPT_DIR/lib/scan-symlinks.php" \
         --tarball "$TARBALL" \
         --username "$USERNAME" \
         --report "$SYMLINK_REPORT"; then
-    log "scan-symlinks.php exited with usage/IO error; aborting (this is not a sanitize-able state)"
     cat "$SYMLINK_REPORT" >&2 || true
-    exit 3
+    fail_extract 3 "scan-symlinks.php exited with usage/IO error; aborting (this is not a sanitize-able state)"
 fi
 
 # --- compute exclude list from dangerous findings -------------------------
@@ -113,7 +130,7 @@ if [[ "$IMPORT_INCLUDE_MAIL" == "true" ]]; then
     # classifier and by tar (control characters, non-canonical paths).
     # scan-symlinks.php alone only strips DANGEROUS absolute targets.
     MAIL_LINKS=$(python3 -I "$SCRIPT_DIR/lib/mail-tree-links.py" scan "$TARBALL" "$EXCLUDES_FILE" "$ACTIONS_OUT") \
-        || { log "mail-tree link scan refused the archive (see above); aborting"; exit 3; }
+        || fail_extract 3 "mail-tree link scan refused the archive (see above); aborting"
     log "stripped $MAIL_LINKS symlink / boundary-crossing hard-link entr(y/ies) in the mail tree"
 else
     {
@@ -148,15 +165,26 @@ log "extracting with hardened tar flags into $DEST"
 #     match is effectively a literal-path skip.
 #   --absolute-names is NOT used — leading / in a member name is stripped.
 cd "$DEST"
+# tar's exit code is captured, not fatal on the spot: the mail-tree verify
+# below must run on whatever tar did write, and a failed tar must still get
+# its partial tree removed.
+TAR_RC=0
 tar --no-same-owner --no-same-permissions --no-overwrite-dir \
     --exclude-from="$EXCLUDES_FILE" \
-    $TAR_FLAGS "$TARBALL"
+    $TAR_FLAGS "$TARBALL" || TAR_RC=$?
 
+VERIFY_RC=0
 if [[ "$IMPORT_INCLUDE_MAIL" == "true" ]]; then
-    # The authority: whatever the classifier above decided, no symlink and
-    # no hard link leaving the mail tree may exist in the extract.
-    python3 -I "$SCRIPT_DIR/lib/mail-tree-links.py" verify "$DEST" \
-        || { log "extracted mail tree contains links (see above); aborting"; exit 4; }
+    # The authority: whatever the classifier above decided, no symlink, no
+    # hard link leaving the mail tree, and no uninspectable directory may
+    # exist in the extract. Always runs, even after a failed tar.
+    python3 -I "$SCRIPT_DIR/lib/mail-tree-links.py" verify "$DEST" || VERIFY_RC=$?
+fi
+if [[ $TAR_RC -ne 0 ]]; then
+    fail_extract "$TAR_RC" "tar failed (rc=$TAR_RC)$( [[ $VERIFY_RC -ne 0 ]] && echo "; mail-tree verify also failed (rc=$VERIFY_RC)" ); aborting"
+fi
+if [[ $VERIFY_RC -ne 0 ]]; then
+    fail_extract 4 "extracted mail tree failed the link/readability check (see above); aborting"
 fi
 
 log "extracted OK ($(find "$DEST" -type f | wc -l) files; $DANGEROUS_COUNT symlinks stripped)"

scripts/lib/mail-tree-links.py

@@ -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__':

tests/build-fixtures.sh

@@ -188,6 +188,10 @@ def build(fname, members):
             ti = tarfile.TarInfo(name); ti.mode = 0o644
             if kind == 'sym':
                 ti.type = tarfile.SYMTYPE; ti.linkname = arg; tf.addfile(ti)
+            elif kind == 'hard':
+                ti.type = tarfile.LNKTYPE; ti.linkname = arg; tf.addfile(ti)
+            elif kind in ('dir', 'dir0'):
+                ti.type = tarfile.DIRTYPE; ti.mode = 0o755 if kind == 'dir' else 0; tf.addfile(ti)
             else:
                 ti.size = len(arg); tf.addfile(ti, io.BytesIO(arg))
 mb = 'homedir/mail/example.com/alice'
@@ -196,8 +200,23 @@ build('cpmove-bad-dslash.tar.gz',  [(f'cpmove-testuser/homedir//mail/example.com
 build('cpmove-bad-dotdot.tar.gz',  [(f'cpmove-testuser/homedir/../{mb}/.x', 'sym', '../../../../etc')])
 build('cpmove-bad-abs.tar.gz',     [(f'/cpmove-testuser/{mb}/.x', 'sym', '../../../../etc')])
 build('cpmove-bad-newline.tar.gz', [(f'cpmove-testuser/{mb}/cur/a\ncpmove-testuser/homedir/public_html', 'file', b'x')])
+# A mail-tree directory the archive makes unreadable (mode 0000): verify
+# cannot inspect it, so the extract must fail (and be removed).
+build('cpmove-bad-unreadable.tar.gz', [(f'cpmove-testuser/{mb}/cur/m1', 'file', b'hello'),
+                                       (f'cpmove-testuser/{mb}/.Locked', 'dir0', None),
+                                       (f'cpmove-testuser/{mb}/.Locked/cur', 'dir0', None)])
+# Passes the classifier but GNU tar fails mid-way (hard link to a member that
+# does not exist), after it already wrote the mail files: the partial tree
+# must be removed.
+build('cpmove-bad-tarfail.tar.gz', [(f'cpmove-testuser/{mb}/cur/m1', 'file', b'hello'),
+                                    ('cpmove-testuser/homedir/public_html', 'dir', None),
+                                    ('cpmove-testuser/homedir/public_html/x', 'hard', 'cpmove-testuser/homedir/nonexistent'),
+                                    (f'cpmove-testuser/{mb}/cur/m2', 'file', b'after the failure')])
 PY
-echo "wrote $FIXTURES_DIR/cpmove-bad-{dot,dslash,dotdot,abs,newline}.tar.gz"
+# Truncated archive: the first half of a valid one.
+head -c "$(( $(stat -c%s "$FIXTURES_DIR/cpmove-mail.tar.gz") / 2 ))" "$FIXTURES_DIR/cpmove-mail.tar.gz" \
+    > "$FIXTURES_DIR/cpmove-bad-truncated.tar.gz"
+echo "wrote $FIXTURES_DIR/cpmove-bad-{dot,dslash,dotdot,abs,newline,unreadable,tarfail,truncated}.tar.gz"
 
 echo ""
 echo "fixtures built:"

tests/run-tests.sh

@@ -79,9 +79,43 @@ for bad in dot dslash dotdot abs newline; do
     IMPORT_INCLUDE_MAIL=true bash "$REPO/scripts/extract.sh" "$WORK/cpmove-bad-$bad.tar.gz" "$OUT" testuser "$WORK/bad-$bad.json" >"$WORK/bad-$bad.log" 2>&1 || rc=$?
     check "bad-$bad: extract exits non-zero (rc=$rc)" "[[ $rc -ne 0 ]]"
     check "bad-$bad: refusal logged" "grep -q 'REFUSING archive' '$WORK/bad-$bad.log'"
-    check "bad-$bad: nothing extracted" "[[ -z \$(find '$OUT' -mindepth 1 2>/dev/null) ]]"
+    check "bad-$bad: extract dir removed" "[[ ! -e '$OUT' ]]"
 done
 
+echo "failures after extraction starts remove the extract (IMPORT_INCLUDE_MAIL=true)"
+for bad in unreadable tarfail truncated; do
+    cp "$FIX/cpmove-bad-$bad.tar.gz" "$WORK/"
+    OUT="$WORK/ex-bad-$bad"
+    rc=0
+    IMPORT_INCLUDE_MAIL=true bash "$REPO/scripts/extract.sh" "$WORK/cpmove-bad-$bad.tar.gz" "$OUT" testuser "$WORK/bad-$bad.json" >"$WORK/bad-$bad.log" 2>&1 || rc=$?
+    check "bad-$bad: extract exits non-zero (rc=$rc)" "[[ $rc -ne 0 ]]"
+    check "bad-$bad: extract dir removed" "[[ ! -e '$OUT' ]]"
+done
+check "bad-unreadable: failed in verify (cannot inspect the 0000 dir)" "grep -q 'not readable+searchable\|cannot inspect' '$WORK/bad-unreadable.log'"
+check "bad-tarfail: tar itself failed (rc logged), verify still ran" "grep -q 'tar failed (rc=' '$WORK/bad-tarfail.log'"
+check "bad-truncated: refused by the classifier" "grep -q 'REFUSING archive' '$WORK/bad-truncated.log'"
+
+echo "classifier refuses when GNU tar lists a different member count"
+FAKEBIN="$WORK/faketar"
+mkdir -p "$FAKEBIN"
+REALTAR="$(command -v tar)"
+cat > "$FAKEBIN/tar" <<FAKE
+#!/usr/bin/env bash
+# Real tar, plus one extra member in the escape-quoted listing only.
+"$REALTAR" "\$@"; rc=\$?
+[[ " \$* " == *" --quoting-style=escape "* ]] && echo "cpmove-testuser/homedir/mail/hidden-from-the-classifier"
+exit \$rc
+FAKE
+chmod +x "$FAKEBIN/tar"
+: > "$WORK/cm.excl"; echo '{"actions":[]}' > "$WORK/cm.json"
+rc=0
+PATH="$FAKEBIN:$PATH" python3 -I "$REPO/scripts/lib/mail-tree-links.py" scan "$MAILTAR" "$WORK/cm.excl" "$WORK/cm.json" >/dev/null 2>"$WORK/cm.log" || rc=$?
+check "count mismatch: scan exits 3 (rc=$rc)" "[[ $rc -eq 3 ]]"
+check "count mismatch: reason logged" "grep -q 'member count differs' '$WORK/cm.log'"
+rc=0
+python3 -I "$REPO/scripts/lib/mail-tree-links.py" scan "$MAILTAR" "$WORK/cm.excl" "$WORK/cm.json" >/dev/null 2>&1 || rc=$?
+check "control: real tar, same archive => scan exits 0" "[[ $rc -eq 0 ]]"
+
 echo "post-extract authority: mail-tree-links.py verify"
 V="$WORK/verify"
 mkvt() { rm -rf "$V"; mkdir -p "$V/$U/homedir/mail/$D/a/cur" "$V/$U/homedir/mail/$D/a/.F/cur" "$V/$U/homedir/public_html" "$V/cpmove-other/homedir/mail/$D/b/cur"; echo m > "$V/$U/homedir/mail/$D/a/cur/1"; }