From 36d4949d348cf2d8f0932662a8b713a8a75383fb Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Tue, 20 Oct 2026 10:51:17 -0700 Subject: [PATCH] extract: own-account links judged re-rooted, normalized, for the trusted username only (N2-1 review 2) - strip-escaping-links.py: the /home[N]// exemption ran on the RAW target (`/home/u/../../etc/shadow` was exempt) and on usernames taken from the archive's own top-dir names (an archive could name itself cpmove-root). Now only the username WHP passes (extract.sh = IMPORT_USERNAME, validated) is trusted; the target must be normalized; and it is RE-ROOTED at the link's /homedir in the extract and kept only if that resolves (realpath strict) inside it. No trusted name: nothing exempt. - scan-symlinks.php: same trusted-name rule; a non-normalized absolute target is DANGEROUS outright (never prefix-compared). - tests: /home/u/../../etc/shadow, /home/u/./x, re-rooted dangling, re-rooted through an escaping link, another user's /home, an archive-named user (cpmove-root) via extract.sh, no/invalid trusted name. 8 checks fail on 72b8fa9. Co-Authored-By: Claude Opus 5.5 --- scripts/extract.sh | 13 +++--- scripts/lib/scan-symlinks.php | 29 ++++++++----- scripts/lib/strip-escaping-links.py | 63 ++++++++++++++++++++++------- tests/run-tests.sh | 39 ++++++++++++++++-- 4 files changed, 109 insertions(+), 35 deletions(-) diff --git a/scripts/extract.sh b/scripts/extract.sh index 9783946..07e6927 100755 --- a/scripts/extract.sh +++ b/scripts/extract.sh @@ -179,15 +179,12 @@ tar --no-same-owner --no-same-permissions --no-overwrite-dir \ # targets lexically, so a chain through another link (a -> b/.., b -> x/y/z) # can pass it. strip-escaping-links.py asks realpath() where every extracted # symlink really lands and removes (records) any that leave $DEST, point at -# $DEST itself, dangle or loop — own-account absolute /home// targets -# excepted. A walk error fails the extract closed. -LINK_USERS=("$USERNAME") -for top in "$DEST"/*/; do - [[ -d "$top" && ! -L "${top%/}" ]] || continue - t="$(basename "$top")"; t="${t#cpmove-}"; LINK_USERS+=("$t" "${t##*_}") -done +# $DEST itself, dangle or loop. A walk error fails the extract closed. +# Only the TRUSTED username (, the WHP account WHP passes) is +# exempted, and only re-rooted inside the extract's homedir; names read from +# the archive (top-dir names) are never trusted. STRIP_RC=0 -ESCAPING=$(python3 -I "$SCRIPT_DIR/lib/strip-escaping-links.py" "$DEST" "$ACTIONS_OUT" "${LINK_USERS[@]}") || STRIP_RC=$? +ESCAPING=$(python3 -I "$SCRIPT_DIR/lib/strip-escaping-links.py" "$DEST" "$ACTIONS_OUT" "$USERNAME") || STRIP_RC=$? if [[ $STRIP_RC -eq 0 && "${ESCAPING:-0}" -gt 0 ]]; then log "stripped $ESCAPING symlink(s) that resolve outside the extract, dangle or loop (post-extract check)" fi diff --git a/scripts/lib/scan-symlinks.php b/scripts/lib/scan-symlinks.php index 21adeaf..aae5c2b 100644 --- a/scripts/lib/scan-symlinks.php +++ b/scripts/lib/scan-symlinks.php @@ -124,7 +124,6 @@ function relative_symlink_escape(string $archivePath, string $target, array $lin } $findings = []; -$cpanelUsername = null; $cmd = 'tar -tvf ' . escapeshellarg($tarPath) . ' 2>/dev/null'; $fh = @popen($cmd, 'r'); @@ -170,19 +169,18 @@ foreach ($links as [$archivePath, $target]) { } if ($target === '') continue; - if ($cpanelUsername === null) { - if (preg_match('#^cpmove-([^/]+)/#', $archivePath, $m)) { - $cpanelUsername = $m[1]; - } - } // (1) user-internal — accept symlinks pointing into the customer's // own /home// tree. The panel rewrites these on extract. $userInternal = false; $usernames = []; - if ($cpanelUsername !== null && $cpanelUsername !== '') $usernames[] = $cpanelUsername; - if ($username !== '') $usernames[] = $username; - foreach ($usernames as $u) { + // N2-1 review: only the TRUSTED --username (the WHP account WHP passes); + // a name read from the archive's top dir (cpmove-) is never trusted. + if (preg_match('/\A[a-z_][a-z0-9_-]{0,31}\z/', $username)) $usernames[] = $username; + // N2-1 review: only a NORMALIZED target can be user-internal + // (`/home//../../etc/shadow` is not). + $normalized = !preg_match('#(^|/)\.{1,2}(/|$)|//#', $target); + foreach ($normalized ? $usernames : [] as $u) { $prefix = '/home/' . $u . '/'; if (strpos($target, $prefix) === 0 || $target === rtrim($prefix, '/')) { $userInternal = true; @@ -195,6 +193,19 @@ foreach ($links as [$archivePath, $target]) { } if ($userInternal) continue; + // N2-1 review: a non-normalized absolute target ('.', '..', '//') is + // never compared by prefix (`/home//../../etc` would dodge /etc): + // DANGEROUS outright. + if (!$normalized) { + $findings[] = [ + 'type' => 'DANGEROUS', + 'archive_path' => $archivePath, + 'target' => $target, + 'reason' => "absolute target is not normalized ('.', '..' or empty component)", + ]; + continue; + } + // (2) exact root. $type = null; $reason = ''; diff --git a/scripts/lib/strip-escaping-links.py b/scripts/lib/strip-escaping-links.py index ffda4c0..29cc768 100644 --- a/scripts/lib/strip-escaping-links.py +++ b/scripts/lib/strip-escaping-links.py @@ -11,13 +11,17 @@ # removed and recorded as a `stripped_escaping_symlink` action when: # - it resolves outside , or to itself; # - it is dangling (strict resolution fails), or loops. -# Exception: an ABSOLUTE target under /home[N]// for one of the -# given usernames (cPanel's own-account links, e.g. wp-content -> -# /home//shared). Those cannot resolve inside this sandbox by design, and -# WHP resolves them only inside the customer's own site container; the -# pre-extract scan calls them BENIGN_USER_INTERNAL. Every other absolute -# target either dangles here or resolves into the container's own /etc, -# /usr, ... — both outside — so it is stripped. +# Own-account absolute links (cPanel's wp-content -> /home//shared and +# the like) are judged RE-ROOTED, never against the container's own /home: +# a target that is already normalized (no '.', '..' or empty component) +# and lies under /home[N]// for the TRUSTED username only (the WHP +# account name WHP passes as extract.sh's ; names read from the +# archive are never trusted) is mapped to //homedir/, where +# is the link's own top-level dir, and kept only if THAT resolves +# (realpath strict) inside /homedir. Every other absolute target either +# dangles here or resolves into the container's own /etc, /usr, ... — both +# outside — so it is stripped. A link to the extract itself is +# stripped too. # # Repeats until a pass strips nothing (removing one link can leave another # that resolved through it dangling). Any walk error fails closed (exit 1). @@ -25,6 +29,7 @@ import json import os +import posixpath import re import stat import sys @@ -32,12 +37,37 @@ import sys def main(argv): if len(argv) < 3: - sys.stderr.write("usage: strip-escaping-links.py [username ...]\n") + sys.stderr.write("usage: strip-escaping-links.py [trusted-username]\n") return 2 dest = os.path.realpath(argv[1]) actions_path = argv[2] - users = [u for u in argv[3:] if u] - user_res = [re.compile(r'\A/home\d*/' + re.escape(u) + r'(/|\Z)') for u in users] + trusted = argv[3] if len(argv) > 3 else '' + if not re.fullmatch(r'[a-z_][a-z0-9_-]{0,31}', trusted): + trusted = '' # no (valid) trusted name: exempt nothing + own_re = re.compile(r'\A/home\d*/' + re.escape(trusted) + r'(?:/(.*))?\Z') if trusted else None + + def own_account_ok(link_path, target): + """True when an absolute own-account target, re-rooted at the link's + /homedir, resolves inside that homedir.""" + if own_re is None or posixpath.normpath(target) != target.rstrip('/') or '/../' in target + '/' or '/./' in target + '/': + return False + m = own_re.match(target.rstrip('/')) + if not m: + return False + rel = os.path.relpath(link_path, dest).split(os.sep) + if len(rel) < 2: + return False + home = os.path.join(dest, rel[0], 'homedir') + try: + hst = os.lstat(home) + if not stat.S_ISDIR(hst.st_mode): + return False + home_real = os.path.realpath(home, strict=True) + cand = os.path.join(home_real, m.group(1)) if m.group(1) else home_real + real = os.path.realpath(cand, strict=True) + except OSError: + return False + return real == home_real or real.startswith(home_real + '/') if not os.path.isdir(dest): sys.stderr.write(f"strip-escaping-links: not a directory: {dest}\n") return 1 @@ -55,13 +85,16 @@ def main(argv): if not stat.S_ISLNK(st.st_mode): continue target = os.readlink(p) - if target.startswith('/') and any(r.match(target) for r in user_res): - continue reason = None + if target.startswith('/') and own_re is not None and own_re.match(target.rstrip('/') or '/'): + if own_account_ok(p, target): + continue + reason = 'own-account target does not resolve inside the extracted homedir' try: - real = os.path.realpath(p, strict=True) - if not real.startswith(dest + '/'): - reason = 'resolves outside the extract' + if reason is None: + real = os.path.realpath(p, strict=True) + if not real.startswith(dest + '/'): + reason = 'resolves outside the extract' except OSError: reason = 'dangling or looping' if reason is None: diff --git a/tests/run-tests.sh b/tests/run-tests.sh index b32e369..ab8577e 100755 --- a/tests/run-tests.sh +++ b/tests/run-tests.sh @@ -102,16 +102,49 @@ ln -s "../.." "$P/$U/top-link" ln -s "inner/f" "$P/$U/homedir/public_html/ok-file" ln -s "public_html" "$P/$U/homedir/www" ln -s "/home/testuser/public_html/inner" "$P/$U/homedir/own-abs" +# Review round 2: own-account absolute links are judged re-rooted in the +# extract, normalized, and only for the TRUSTED username argument. +ln -s "/home/testuser/../../etc/shadow" "$P/$U/homedir/own-dotdot" +ln -s "/home/testuser/./public_html/inner" "$P/$U/homedir/own-dot" +ln -s "/home/testuser/public_html/missing" "$P/$U/homedir/own-dangling" +ln -s "../../.." "$P/$U/homedir/esc"; ln -s "/home/testuser/esc" "$P/$U/homedir/own-via-esc" +ln -s "/home/root/x" "$P/$U/homedir/other-user" +mkdir -p "$P/cpmove-root/homedir/x"; ln -s "/home/root/x" "$P/cpmove-root/homedir/archive-named-user" echo '{"actions":[{"action":"pre"}]}' > "$WORK/postx.json" n=$(python3 -I "$REPO/scripts/lib/strip-escaping-links.py" "$P" "$WORK/postx.json" testuser 2>/dev/null) -for gone in d1/d2/a d1/d2/l1 homedir/abs-etc homedir/public_html/x.php.bak.cpanel-import homedir/public_html/y.bak homedir/loop1 homedir/loop2 top-link; do +for gone in d1/d2/a d1/d2/l1 homedir/abs-etc homedir/public_html/x.php.bak.cpanel-import homedir/public_html/y.bak homedir/loop1 homedir/loop2 top-link \ + homedir/own-dotdot homedir/own-dot homedir/own-dangling homedir/esc homedir/own-via-esc homedir/other-user; do check "post-extract: $gone stripped" "[[ ! -L '$P/$U/$gone' ]]" done for kept in d1/d2/b d1/d2/l2 homedir/public_html/ok-file homedir/www homedir/own-abs; do check "post-extract: $kept kept" "[[ -L '$P/$U/$kept' ]]" done -check "post-extract: 8 links stripped (got $n)" "[[ '$n' == 8 ]]" -check "post-extract: strips appended as stripped_escaping_symlink actions" "grep -q '\"pre\"' '$WORK/postx.json' && [[ \$(grep -c stripped_escaping_symlink '$WORK/postx.json') == 8 ]]" +check "post-extract: cpmove-root/…/archive-named-user (/home/root/x; 'root' only named by the archive) stripped" "[[ ! -L '$P/cpmove-root/homedir/archive-named-user' ]]" +check "post-extract: 15 links stripped (got $n)" "[[ '$n' == 15 ]]" +check "post-extract: strips appended as stripped_escaping_symlink actions" "grep -q '\"pre\"' '$WORK/postx.json' && [[ \$(grep -c stripped_escaping_symlink '$WORK/postx.json') == 15 ]]" +rm -rf "$P"; mkdir -p "$P/$U/homedir/public_html/inner" +ln -s "/home/testuser/public_html/inner" "$P/$U/homedir/own-abs" +python3 -I "$REPO/scripts/lib/strip-escaping-links.py" "$P" "$WORK/postx2.json" >/dev/null 2>&1 +check "post-extract: with no trusted username, an own-account link is NOT exempt" "[[ ! -L '$P/$U/homedir/own-abs' ]]" +rm -rf "$P"; mkdir -p "$P/$U/homedir/public_html/inner" +ln -s "/home/testuser/public_html/inner" "$P/$U/homedir/own-abs" +python3 -I "$REPO/scripts/lib/strip-escaping-links.py" "$P" "$WORK/postx3.json" 'testuser/../x' >/dev/null 2>&1 +check "post-extract: an invalid 'trusted' username exempts nothing" "[[ ! -L '$P/$U/homedir/own-abs' ]]" +echo "extract.sh: an archive cannot name its own exempt user (top dir cpmove-root)" +AR="$WORK/archuser-src"; rm -rf "$AR"; mkdir -p "$AR/cpmove-root/homedir/x" +ln -s "/home/root/x" "$AR/cpmove-root/homedir/self-exempt" +tar -C "$AR" -czf "$WORK/archuser.tar.gz" cpmove-root +env -u IMPORT_INCLUDE_MAIL bash "$REPO/scripts/extract.sh" "$WORK/archuser.tar.gz" "$WORK/ex-archuser" testuser "$WORK/archuser.json" >/dev/null 2>&1 +check "archive-named user: /home/root/x link stripped (only the WHP username is trusted)" "[[ -d '$WORK/ex-archuser/cpmove-root/homedir/x' && ! -L '$WORK/ex-archuser/cpmove-root/homedir/self-exempt' ]]" +env -u IMPORT_INCLUDE_MAIL bash "$REPO/scripts/extract.sh" "$WORK/archuser.tar.gz" "$WORK/ex-archuser2" root "$WORK/archuser2.json" >/dev/null 2>&1 +check "control: the same link is kept when WHP itself passes that username" "[[ -L '$WORK/ex-archuser2/cpmove-root/homedir/self-exempt' ]]" + +echo "scan-symlinks: /home//../../etc is not user-internal" +DT="$WORK/dotdot-src"; rm -rf "$DT"; mkdir -p "$DT/$U/homedir" +ln -s "/home/testuser/../../etc/shadow" "$DT/$U/homedir/sneaky" +tar -C "$DT" -czf "$WORK/dotdot.tar.gz" "$U" +php "$REPO/scripts/lib/scan-symlinks.php" --tarball "$WORK/dotdot.tar.gz" --report "$WORK/relesc-scan.json" --username testuser 2>/dev/null +check "scan: /home/testuser/../../etc/shadow is DANGEROUS (not BENIGN_USER_INTERNAL)" "[[ \$(relesc_type '$U/homedir/sneaky') == DANGEROUS ]]" echo "hard link crossing OUT of the mail tree (cpmove-mail-xlink)" cp "$FIX/cpmove-mail-xlink.tar.gz" "$WORK/"