From 4a43d26de7235d1463c50953ec7eac6dd935c5f8 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Tue, 20 Oct 2026 09:59:41 -0700 Subject: [PATCH] extract: mail-tree policy covers ANY top-level dir, not just cpmove-* (I-1) Review I-1: WHP's CpanelArchivePaths::selectTopDir() takes the first non-hidden top dir of any name (cpmove-, cPanel's user download backup-_/, acct/ ...), but the link classifier, the post-extract verify and scan-files' --skip-mail only matched cpmove-*. Measured by the reviewer: an acct/ archive kept a ../../etc symlink in its Maildir, and ClamAV would quarantine messages OUT of a backup-* Maildir (=> Verified with mail missing). - mail-tree-links.py: MAIL = ^([^/]+)/homedir/mail; verify walks every top-level dir, and fails on a top-level or homedir symlink instead of walking through it. Covering every top dir is a superset of whichever one WHP picks, so multi-top-dir archives need no special case. - scan-files.php: --skip-mail patterns use [^/]+ for the top dir. - M-7: entrypoint refuses IMPORT_INCLUDE_MAIL=true as uid 0 (the verify's os.access check is meaningless for root; WHP runs --user 999:999). - M-2: the mail-tree refusals tell the operator the same backup may import with "Import without email". - M-4: run-tests.sh checks the env/marker contract against a WHP checkout when WHP_REPO is set. - Fixtures cpmove-acct / cpmove-backupdir (the mail fixture under acct/ and backup-10.20.2026_10-00-00_testuser/). 13 checks fail on d4a63fb. Docker: backup-* Maildir EICAR not quarantined, public_html one is; --user 0 with the opt-in => status failed. The default (no-opt-in) mail exclude keeps its historical cpmove-* scope. Co-Authored-By: Claude Opus 5.5 --- scripts/entrypoint.sh | 5 ++++ scripts/extract.sh | 8 ++++--- scripts/lib/mail-tree-links.py | 27 ++++++++++++++++++---- scripts/scan-files.php | 10 ++++---- tests/build-fixtures.sh | 16 +++++++++++++ tests/run-tests.sh | 42 +++++++++++++++++++++++++++++++--- 6 files changed, 94 insertions(+), 14 deletions(-) diff --git a/scripts/entrypoint.sh b/scripts/entrypoint.sh index 0f19e89..ed2ca83 100755 --- a/scripts/entrypoint.sh +++ b/scripts/entrypoint.sh @@ -83,6 +83,11 @@ else IMPORT_INCLUDE_MAIL=false fi export IMPORT_INCLUDE_MAIL +# The mail-tree verify's readability check (os.access) is meaningless for +# root; the image runs as uid 999 and the opt-in path insists on it. +if [[ "$IMPORT_INCLUDE_MAIL" == "true" && "$(id -u)" == "0" ]]; then + die "IMPORT_INCLUDE_MAIL=true must not run as root (container --user 999:999 expected)" +fi log "import_id=$IMPORT_ID username=$IMPORT_USERNAME backup=$IMPORT_BACKUP_FILE include_mail=$IMPORT_INCLUDE_MAIL" diff --git a/scripts/extract.sh b/scripts/extract.sh index 25ea716..0340dba 100755 --- a/scripts/extract.sh +++ b/scripts/extract.sh @@ -118,7 +118,9 @@ fi # pattern uses tar's fnmatch globs and covers the whole tree at any depth. # # IMPORT_INCLUDE_MAIL=true (WHP passes it only when the import copies -# mailboxes) extracts it. The DANGEROUS-symlink strips computed above apply +# mailboxes) extracts it. The link policy and verify below apply to +# /homedir/mail, not only cpmove-*: WHP accepts any top-level +# dir name. (The default-mode exclude keeps its historical cpmove-* scope.) The DANGEROUS-symlink strips computed above apply # to the mail tree exactly as to the rest of the archive: scan-symlinks.php # lists every entry in the tarball, mail included. if [[ "$IMPORT_INCLUDE_MAIL" == "true" ]]; then @@ -130,7 +132,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") \ - || fail_extract 3 "mail-tree link scan refused the archive (see above); aborting" + || fail_extract 3 "mail-tree link scan refused the archive (see above); aborting — the same backup may import with 'Import without email'" log "stripped $MAIL_LINKS symlink / boundary-crossing hard-link entr(y/ies) in the mail tree" else { @@ -184,7 +186,7 @@ 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" + fail_extract 4 "extracted mail tree failed the link/readability check (see above); aborting — the same backup may import with 'Import without email'" fi log "extracted OK ($(find "$DEST" -type f | wc -l) files; $DANGEROUS_COUNT symlinks stripped)" diff --git a/scripts/lib/mail-tree-links.py b/scripts/lib/mail-tree-links.py index 2bfad8c..17b2a98 100644 --- a/scripts/lib/mail-tree-links.py +++ b/scripts/lib/mail-tree-links.py @@ -47,7 +47,10 @@ import subprocess import sys import tarfile -MAIL = re.compile(r'^(cpmove-[^/]+)/homedir/mail(/|$)') +# ANY top-level directory: WHP's CpanelArchivePaths::selectTopDir() takes the +# first non-hidden dir of any name (cpmove-, cPanel's backup-_, +# acct, ...). Covering every top dir is a superset of whichever it picks. +MAIL = re.compile(r'^([^/]+)/homedir/mail(/|$)') CTRL = re.compile(r'[\x00-\x1f\x7f]') @@ -146,13 +149,29 @@ def verify(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 try: + tst = os.lstat(os.path.join(dest, top)) + except OSError as err: + walk_error(err) + continue + if stat.S_ISLNK(tst.st_mode): + # Never walk through it (WHP refuses a top-level symlink anyway). + bad.append(f'top-level symlink: {top}') + continue + if not stat.S_ISDIR(tst.st_mode): + continue + mail = os.path.join(dest, top, 'homedir', 'mail') + try: + hst = os.lstat(os.path.join(dest, top, 'homedir')) + if stat.S_ISLNK(hst.st_mode): + bad.append(f'symlink: {top}/homedir') + continue st = os.lstat(mail) except FileNotFoundError: continue + except OSError as err: + walk_error(err) + continue if stat.S_ISLNK(st.st_mode): bad.append(f'symlink: {top}/homedir/mail') continue diff --git a/scripts/scan-files.php b/scripts/scan-files.php index ebc126d..1e5f6c9 100755 --- a/scripts/scan-files.php +++ b/scripts/scan-files.php @@ -10,7 +10,7 @@ * scan-files.php --extract --quarantine --report --import-id [--skip-mail] * * --skip-mail: do not scan (or count, or quarantine anything in) the - * cpmove-/homedir/mail tree. entrypoint.sh passes it when the mail tree + * /homedir/mail tree (any top-level dir). entrypoint.sh passes it when the mail tree * was extracted for WHP's mailbox import (IMPORT_INCLUDE_MAIL=true). * * Exit codes: @@ -45,13 +45,15 @@ $reportPath = $opts['report']; $importId = $opts['import-id']; $skipMail = array_key_exists('skip-mail', $opts); +// /homedir/mail: WHP accepts any top-level dir name +// (cpmove-, backup-_, ...), so the skip covers all of them. // The mail tree is copied verbatim into the customer's own mailbox, and // WHP verifies the copy message-by-message against what it packaged. So it // is NOT scanned: quarantining a message would silently drop mail, and // ClamAV over a multi-GB maildir would dominate the run. PCRE form for the // PHP walk, POSIX ERE form for clamscan --exclude-dir. -$mailDirPcre = '#\A' . preg_quote($extractDir, '#') . '/cpmove-[^/]+/homedir/mail(?:/|\z)#'; -$mailDirEre = '^' . preg_replace('/[.\[\]()*+?{}|^$\\\\]/', '\\\\$0', $extractDir) . '/cpmove-[^/]+/homedir/mail(/.*)?$'; +$mailDirPcre = '#\A' . preg_quote($extractDir, '#') . '/[^/]+/homedir/mail(?:/|\z)#'; +$mailDirEre = '^' . preg_replace('/[.\[\]()*+?{}|^$\\\\]/', '\\\\$0', $extractDir) . '/[^/]+/homedir/mail(/.*)?$'; if (!is_dir($extractDir)) { fwrite(STDERR, "scan-files: extract dir does not exist: $extractDir\n"); @@ -62,7 +64,7 @@ if (!is_dir($extractDir)) { fwrite(STDERR, "scan-files: starting (extract=$extractDir, quarantine=$quarantineDir)\n"); if ($skipMail) { - fwrite(STDERR, "scan-files: NOT scanning the mail tree (cpmove-*/homedir/mail): it is copied verbatim into the customer's own mailbox and verified against the source; scanning would quarantine messages out of it and multi-GB maildirs would dominate the run\n"); + fwrite(STDERR, "scan-files: NOT scanning the mail tree (/homedir/mail): it is copied verbatim into the customer's own mailbox and verified against the source; scanning would quarantine messages out of it and multi-GB maildirs would dominate the run\n"); } // -- v1.0 cleaner registry (intentionally empty) ---------------------------- diff --git a/tests/build-fixtures.sh b/tests/build-fixtures.sh index 115a805..a6998af 100755 --- a/tests/build-fixtures.sh +++ b/tests/build-fixtures.sh @@ -213,6 +213,22 @@ build('cpmove-bad-tarfail.tar.gz', [(f'cpmove-testuser/{mb}/cur/m1', 'file', b'h ('cpmove-testuser/homedir/public_html/x', 'hard', 'cpmove-testuser/homedir/nonexistent'), (f'cpmove-testuser/{mb}/cur/m2', 'file', b'after the failure')]) PY +# The same mail fixture under other top-level dir names WHP accepts +# (CpanelArchivePaths::selectTopDir takes the first non-hidden dir of ANY +# name): `acct/` and cPanel's user-download layout `backup-_/`. +python3 - "$FIXTURES_DIR" <<'PY' +import io, sys, tarfile +out = sys.argv[1] +src = f'{out}/cpmove-mail.tar.gz' +for top, fname in (('acct', 'cpmove-acct.tar.gz'), ('backup-10.20.2026_10-00-00_testuser', 'cpmove-backupdir.tar.gz')): + with tarfile.open(src) as tin, tarfile.open(f'{out}/{fname}', 'w:gz', format=tarfile.PAX_FORMAT) as tout: + for m in tin.getmembers(): + data = tin.extractfile(m).read() if m.isfile() else None + m.name = m.name.replace('cpmove-testuser', top, 1) + if m.islnk(): + m.linkname = m.linkname.replace('cpmove-testuser', top, 1) + tout.addfile(m, io.BytesIO(data) if data is not None else None) +PY # 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" diff --git a/tests/run-tests.sh b/tests/run-tests.sh index 40836cb..af902a6 100755 --- a/tests/run-tests.sh +++ b/tests/run-tests.sh @@ -147,8 +147,9 @@ cat > "$STUBBIN/clamscan" <<'STUB' #!/usr/bin/env bash printf '%s\n' "$@" > "$CLAMSCAN_ARGS_OUT" dir="${!#}" -echo "$dir/cpmove-testuser/homedir/mail/example.com/alice/cur/1700000000.M1P1.host,S=40:2,S: Test.Mail.Sig FOUND" -echo "$dir/cpmove-testuser/homedir/public_html/index.php: Test.Php.Sig FOUND" +top="${CLAM_TOP:-cpmove-testuser}" +echo "$dir/$top/homedir/mail/example.com/alice/cur/1700000000.M1P1.host,S=40:2,S: Test.Mail.Sig FOUND" +echo "$dir/$top/homedir/public_html/index.php: Test.Php.Sig FOUND" STUB chmod +x "$STUBBIN/clamscan" @@ -167,7 +168,7 @@ run_scan "$WORK/scan-skip" skip --skip-mail # Expected argv entry: the extract dir with ERE metacharacters escaped # (the only one mktemp's path can hold is "."), then the mail-tree pattern. SKIPDIR="$WORK/scan-skip" -EXCL="--exclude-dir=^${SKIPDIR//./\\.}/cpmove-[^/]+/homedir/mail(/.*)?\$" +EXCL="--exclude-dir=^${SKIPDIR//./\\.}/[^/]+/homedir/mail(/.*)?\$" if grep -qxF -- "$EXCL" "$WORK/clam-skip.args"; then pass "--skip-mail: clamscan gets --exclude-dir for the mail tree" else @@ -184,6 +185,41 @@ check "--skip-mail: files_scanned counts only non-mail files ($NONMAIL, after th "[[ \$(jq_php '$WORK/scan-skip.json' files_scanned) == $((NONMAIL + 1)) ]]" check "--skip-mail: the skip is logged with its reason" "grep -q 'NOT scanning the mail tree' '$WORK/scan-skip.log'" +echo "I-1: top-level dirs other than cpmove-* (acct/, backup-_/)" +for fx in acct:acct backupdir:backup-10.20.2026_10-00-00_testuser; do + name="${fx%%:*}"; top="${fx#*:}" + cp "$FIX/cpmove-$name.tar.gz" "$WORK/" + OUT="$WORK/ex-$name" + rc=0 + IMPORT_INCLUDE_MAIL=true bash "$REPO/scripts/extract.sh" "$WORK/cpmove-$name.tar.gz" "$OUT" testuser "$WORK/ex-$name.actions.json" >"$WORK/ex-$name.log" 2>&1 || rc=$? + M="$OUT/$top/homedir/mail/$D/alice" + check "$name: extract exits 0 (rc=$rc)" "[[ $rc -eq 0 ]]" + check "$name: relative symlinks in the Maildir stripped (.evil-rel, .Other)" "[[ ! -L '$M/.evil-rel' && ! -L '$M/.Other' ]]" + check "$name: no symlink at all under $top/homedir/mail" "[[ -z \$(find '$OUT/$top/homedir/mail' -type l) ]]" + check "$name: hard link INTO the mail tree stripped" "[[ ! -e '$M/cur/leak' ]]" + check "$name: in-tree Dovecot hard link kept" "[[ -f '$M/cur/1700000002.M3P1.host,S=41:2,S' ]]" + check "$name: strips recorded as stripped_mail_link" "grep -q '$top/homedir/mail/$D/alice/.evil-rel' '$WORK/ex-$name.actions.json'" + rm -rf "$WORK/q-$name" + CLAM_TOP="$top" CLAMSCAN_ARGS_OUT="$WORK/clam-$name.args" PATH="$STUBBIN:$PATH" \ + php "$REPO/scripts/scan-files.php" --extract "$OUT" --quarantine "$WORK/q-$name" \ + --report "$WORK/scan-$name.json" --import-id "t-$name" --skip-mail 2>/dev/null + check "$name: --skip-mail keeps a hit inside $top/homedir/mail in the Maildir" "[[ -f '$M/cur/1700000000.M1P1.host,S=40:2,S' ]]" + check "$name: --skip-mail still quarantines public_html" "[[ ! -e '$OUT/$top/homedir/public_html/index.php' ]]" +done +mkvt; mkdir -p "$V/acct/homedir/mail/$D/a/cur"; ln -s ../../../../../etc "$V/acct/homedir/mail/$D/a/x" +check "verify: symlink under acct/homedir/mail => 4" "[[ \$(vrc) == 4 ]]" +mkvt; ln -s "$U" "$V/zz-link" +check "verify: top-level symlink => 4 (never walked through)" "[[ \$(vrc) == 4 ]]" + +if [[ -n "${WHP_REPO:-}" ]]; then + echo "M-4: contract with the WHP panel at \$WHP_REPO" + SAN="$WHP_REPO/web-files/libs/CpanelImportSanitizationContainer.php" + check "WHP sends the env name this image reads (INCLUDE_MAIL_ENV = 'IMPORT_INCLUDE_MAIL')" "grep -q \"INCLUDE_MAIL_ENV = 'IMPORT_INCLUDE_MAIL'\" '$SAN'" + check "WHP sends exactly '=true'" "grep -q \"INCLUDE_MAIL_ENV . '=true'\" '$SAN'" + check "WHP reads the report key this image writes (mail_included === true)" "grep -q \"\\['mail_included'\\] ?? null) === true\" '$SAN'" + check "this image writes mail_included" "grep -q '\"mail_included\"' '$REPO/scripts/entrypoint.sh'" +fi + echo "scan-files.php without --skip-mail (unchanged behaviour)" cp -a "$WORK/ex-mail" "$WORK/scan-noskip" run_scan "$WORK/scan-noskip" noskip