From 07378506a7fc1339ca7d4d030a302c1996079c9a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 13:46:52 -0700 Subject: [PATCH] harden(cac-lsphp): close the two remaining unvetted ini emissions; stop MINFO lying MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three non-blocking findings from the re-review of this branch. No design change: the extension's fail-open-absolute invariant is untouched (still zero error emitters, every RINIT return is SUCCESS) and both RINIT guards stay pure narrowing. 1. entrypoint-lsphp.sh: 99-user-opcache.ini was still emitted unquoted ------------------------------------------------------------------- Twenty-five lines below the mapping fix, the opcache override block interpolated the raw env into an unquoted `echo` — the same injection class the mapping fix closed. Measured on this branch's image, before this commit: OPCACHE_MEMORY_MB=$'128\nprecision = 7\n; ' -> 99-user-opcache.ini gained a `precision = 7` line -> lsphp -i reported precision => 7 => 7 WHP casts (int) and clamps 32-512 / 2000-32000 (site-pool-env.php), so this is not exploitable today — but "the panel validates it" is precisely the argument this branch already rejected for `domain`, and the panel is a different repo on a different release cadence. Both siblings in the block are now validated at the point of use (digits only, length-capped, range-checked) and emitted double-quoted. A rejected value is dropped with a WARNING and the image default applies; nothing here is ever fatal. The accepted ranges are PHP's own limits for these directives (>= 8 MB; [200, 1000000] files), deliberately a strict SUPERSET of the panel's clamps, so widening a panel clamp later cannot start silently rejecting real sites. The block now also removes a stale fragment when it has nothing valid to write: the container filesystem outlives `docker restart`, so without that an override that is later cleared — or rejected — would keep applying from the previous boot's file. 2. 99-user-error-log.ini was written from an unvetted $user -------------------------------------------------------- It was emitted before the INI_TOKENS_OK branch. Contained in practice (a newline is inert inside the quotes, and a `${`-bearing user cannot exist because useradd would have failed under `set -euo pipefail`), but "this particular unvetted value happens to be contained" is the reasoning this branch rejected one screenful up. Now gated identically. Costs a rejected user nothing it needs: `log_errors = On` is already baked in by 99-prod-overrides.ini, so PHP still logs — to stderr, i.e. `docker logs`, which is more visible than a per-site file, not less. Verified fleet-wide that no legitimate user reaches the branch (30 shared_ols sites, 4 hosts). 3. MINFO reported "active" for mappings RINIT ignores --------------------------------------------------- The absolute-path guard was added to RINIT and MINFO kept testing only "both values non-empty", so: from=mnt/users/bob/site.com (relative -> INERT since the guard landed) lsphp -i -> Rewriting => active That row is what the post-deploy fleet canary greps to confirm parity is live, so the diagnostic would have masked exactly the failure the canary exists to find — and the C comment added by this branch documents it as the only runtime signal. RINIT and MINFO now share one predicate pair (cacpp_mapping_configured / cacpp_mapping_active) rather than two longhand copies, which is what drifted. MINFO now distinguishes "inactive (mapping not absolute)" from "inactive (unconfigured)" — different operational problems. Pure reporting change: the predicates are side-effect-free and cannot fail, so MINFO gains no error path. Tests: two new .phpt cover both directions of the MINFO fix (009 relative mapping must report inactive, 010 well-formed mapping must still report active), so tightening it cannot overshoot into the opposite lie. The build gate's EXPECTED count is derived from `ls tests/*.phpt`, so it picked them up: 10/10. Non-vacuity, all five demonstrated by mutation: - opcache quoting reverted -> injection lands, `precision => 7` observed - error-log gate removed -> fragment written from the unvetted user - MINFO reverted to non-empty -> 009 FAILS, build gate exits 1 - MINFO forced always-inactive -> 010 FAILS, build gate exits 1 - all restored -> 10/10, build exit 0 --- ext/cac-path-parity/cac_path_parity.c | 59 +++++++++- .../009-minfo-reports-inert-mapping.phpt | 30 +++++ .../010-minfo-reports-active-mapping.phpt | 24 ++++ scripts/entrypoint-lsphp.sh | 104 ++++++++++++++++-- 4 files changed, 203 insertions(+), 14 deletions(-) create mode 100644 ext/cac-path-parity/tests/009-minfo-reports-inert-mapping.phpt create mode 100644 ext/cac-path-parity/tests/010-minfo-reports-active-mapping.phpt diff --git a/ext/cac-path-parity/cac_path_parity.c b/ext/cac-path-parity/cac_path_parity.c index 00aea48..5a77121 100644 --- a/ext/cac-path-parity/cac_path_parity.c +++ b/ext/cac-path-parity/cac_path_parity.c @@ -88,6 +88,13 @@ * louder way. The only runtime signal is `lsphp -i`, which prints * "Rewriting => active" alongside the live from/to values. * + * That row is what the post-deploy fleet canary greps, so MINFO's "active" test + * must stay a mirror of the conditions RINIT actually rewrites under — see + * PHP_MINFO_FUNCTION below, which shares cacpp_mapping_active() with RINIT + * precisely so the two cannot drift. A MINFO that reported "active" for a + * mapping RINIT treats as inert would mask exactly the failure the canary + * exists to catch. + * * SCOPE / KNOWN LIMITS * -------------------- * Only $_SERVER is rewritten. LSAPI also answers getenv('DOCUMENT_ROOT') from @@ -154,6 +161,30 @@ static PHP_GINIT_FUNCTION(cac_path_parity) cac_path_parity_globals->to = NULL; } +/* + * THE MAPPING PREDICATE — one definition, two callers. + * + * RINIT uses it to decide whether to rewrite; MINFO uses it to REPORT whether + * rewriting is live. Those two tests were written out longhand in two places + * and promptly drifted: the absolute-path guard was added to RINIT only, so + * `lsphp -i` went on printing "Rewriting => active" for a mapping RINIT had + * already decided to ignore. That row is the fleet canary's signal, so the lie + * masked precisely the failure the canary looks for. Keep them sharing this. + * + * Pure predicates over two NUL-terminated strings: no allocation, no side + * effect, no way to fail — MINFO gains no error path by calling them, and the + * fail-open invariant is untouched. + */ +static int cacpp_mapping_configured(const char *from, const char *to) +{ + return from != NULL && *from != '\0' && to != NULL && *to != '\0'; +} + +static int cacpp_mapping_active(const char *from, const char *to) +{ + return cacpp_mapping_configured(from, to) && *from == '/' && *to == '/'; +} + /* Trailing slashes would defeat the component-boundary test below. */ static size_t cacpp_trim(const char *s, size_t len) { @@ -235,7 +266,7 @@ PHP_RINIT_FUNCTION(cac_path_parity) const char *to = CACPP_G(to); /* Unconfigured (any tier that isn't shared-ols) => completely inert. */ - if (from == NULL || *from == '\0' || to == NULL || *to == '\0') { + if (!cacpp_mapping_configured(from, to)) { return SUCCESS; } @@ -245,7 +276,7 @@ PHP_RINIT_FUNCTION(cac_path_parity) * against a SAPI-supplied path could only ever produce nonsense. Treat it * like an absent mapping — inert, no diagnostic, request proceeds. */ - if (*from != '/' || *to != '/') { + if (!cacpp_mapping_active(from, to)) { return SUCCESS; } @@ -292,13 +323,31 @@ PHP_MINFO_FUNCTION(cac_path_parity) { const char *from = CACPP_G(from); const char *to = CACPP_G(to); - int active = (from && *from && to && *to); + + /* + * Report what RINIT would ACTUALLY do, by asking the same predicates RINIT + * asks — never a longhand copy of them (see cacpp_mapping_active above for + * what that cost last time). Three distinct answers, because "configured but + * ignored" is a different operational problem from "not configured" and the + * canary must be able to tell them apart. + */ + const char *state; + if (cacpp_mapping_active(from, to)) { + state = "active"; + } else if (cacpp_mapping_configured(from, to)) { + state = "inactive (mapping not absolute)"; + } else { + state = "inactive (unconfigured)"; + } php_info_print_table_start(); php_info_print_table_header(2, "cac_path_parity support", "enabled"); php_info_print_table_row(2, "Version", PHP_CAC_PATH_PARITY_VERSION); - /* The canary greps for this row: "active" proves the mapping is live. */ - php_info_print_table_row(2, "Rewriting", active ? "active" : "inactive (unconfigured)"); + /* + * The canary greps for this row: "active" proves the mapping is live — and, + * since the predicate is shared with RINIT, proves the request path agrees. + */ + php_info_print_table_row(2, "Rewriting", state); php_info_print_table_end(); DISPLAY_INI_ENTRIES(); diff --git a/ext/cac-path-parity/tests/009-minfo-reports-inert-mapping.phpt b/ext/cac-path-parity/tests/009-minfo-reports-inert-mapping.phpt new file mode 100644 index 0000000..683c3f3 --- /dev/null +++ b/ext/cac-path-parity/tests/009-minfo-reports-inert-mapping.phpt @@ -0,0 +1,30 @@ +--TEST-- +cac_path_parity: MINFO reports a non-absolute mapping as INACTIVE, not active +--EXTENSIONS-- +cac_path_parity +--INI-- +cac_path_parity.from=mnt/users/bob/site.com +cac_path_parity.to=/home/bob +--FILE-- + active". A canary that reports healthy for a dead mapping hides +// precisely the failure it was deployed to find. +// +// MINFO and RINIT now share cacpp_mapping_active(); revert MINFO to the +// non-empty test and this prints "active". +ob_start(); +phpinfo(INFO_MODULES); +$info = ob_get_clean(); + +// Also assert the row is unique, so the match below cannot be some other +// module's identically-named row. +var_dump(preg_match_all('/^Rewriting => (.+)$/m', $info, $m)); +var_dump(rtrim($m[1][0])); +?> +--EXPECT-- +int(1) +string(31) "inactive (mapping not absolute)" diff --git a/ext/cac-path-parity/tests/010-minfo-reports-active-mapping.phpt b/ext/cac-path-parity/tests/010-minfo-reports-active-mapping.phpt new file mode 100644 index 0000000..82bc40c --- /dev/null +++ b/ext/cac-path-parity/tests/010-minfo-reports-active-mapping.phpt @@ -0,0 +1,24 @@ +--TEST-- +cac_path_parity: MINFO reports a well-formed mapping as ACTIVE +--EXTENSIONS-- +cac_path_parity +--INI-- +cac_path_parity.from=/mnt/users/bob/site.com +cac_path_parity.to=/home/bob +--FILE-- +/ -> +// /home/), and 001 proves RINIT really does rewrite under it. +ob_start(); +phpinfo(INFO_MODULES); +$info = ob_get_clean(); + +var_dump(preg_match_all('/^Rewriting => (.+)$/m', $info, $m)); +var_dump(rtrim($m[1][0])); +?> +--EXPECT-- +int(1) +string(6) "active" diff --git a/scripts/entrypoint-lsphp.sh b/scripts/entrypoint-lsphp.sh index a794714..348d358 100644 --- a/scripts/entrypoint-lsphp.sh +++ b/scripts/entrypoint-lsphp.sh @@ -85,8 +85,9 @@ INI_TOKENS_OK=yes case "$user" in ''|*[!A-Za-z0-9._-]*) INI_TOKENS_OK=no ;; esac case "$SAFE_DOMAIN" in ''|*[!A-Za-z0-9._-]*) INI_TOKENS_OK=no ;; esac if [ "$INI_TOKENS_OK" != yes ]; then - echo "WARNING: entrypoint-lsphp: user/domain contain characters outside [A-Za-z0-9._-] — refusing to write the \$_SERVER path-parity mapping (the extension stays inert; requests are unaffected). user=$(printf '%q' "$user") domain=$(printf '%q' "$domain")" >&2 + echo "WARNING: entrypoint-lsphp: user/domain contain characters outside [A-Za-z0-9._-] — refusing to generate php.ini fragments from them, so the \$_SERVER path-parity mapping and the per-site error_log are BOTH skipped (the extension stays inert, log_errors stays On from the image defaults and PHP logs to stderr i.e. \`docker logs\`; requests are unaffected). user=$(printf '%q' "$user") domain=$(printf '%q' "$domain")" >&2 fi + ## The exact path prefix the shared-ols container serves this site from — the ## string OLS puts in SCRIPT_FILENAME/DOCUMENT_ROOT. Used twice: for the symlink ## that makes it RESOLVE, and for the cac_path_parity mapping that makes it READ @@ -131,6 +132,34 @@ export LSPHP_ENABLE_USER_INI="${LSPHP_ENABLE_USER_INI:-on}" echo "Container memory: ${CONTAINER_MEMORY_MB}MB | PHP_LSAPI_CHILDREN=${PHP_LSAPI_CHILDREN} | LSAPI_MAX_IDLE=${LSAPI_MAX_IDLE} | PHPVER=${PHPVER} | bind=${LSPHP_BIND} | user_ini=${LSPHP_ENABLE_USER_INI}" +## Validate a numeric value destined for a generated php.ini fragment. +## Sets INI_NUM to the value when it is acceptable, and to "" (plus a WARNING) +## when it is not. Never fatal: a rejected override just leaves the image +## default in place, and the site serves either way. +## +## Digits-only is what closes the injection: no newline, quote, `$` or `{` can +## survive it, so neither an ini-directive injection nor php.ini's `${VAR}` +## interpolation is reachable regardless of what the caller sent. The range +## bound is a separate concern — it stops a typo'd value from making opcache +## fail its shared-memory allocation at startup. +validate_ini_num() { + local name="$1" val="$2" min="$3" max="$4" + INI_NUM="" + case "$val" in + ''|*[!0-9]*) ;; + *) + ## Length-cap first: `[ -lt ]` on a 25-digit string is an arithmetic + ## error, not a comparison. 7 digits covers every max below. + if [ "${#val}" -le 7 ] && [ "$val" -ge "$min" ] && [ "$val" -le "$max" ]; then + INI_NUM="$val" + return 0 + fi + ;; + esac + echo "WARNING: entrypoint-lsphp: ${name}=$(printf '%q' "$val") is not a plain integer in ${min}-${max} — ignoring it; the image default from 99-prod-overrides.ini applies." >&2 + return 0 +} + ## ---- per-site ini drop-ins (identical mechanism to entrypoint-litespeed.sh) ---- ## error_log → the same customer-visible path cac:phpNN / cac-litespeed use, so ## "where's my PHP error log?" is answered identically across all site types. @@ -145,11 +174,31 @@ if [ -n "$SCAN_DIR" ]; then mkdir -p "$SCAN_DIR" ## Values emitted double-quoted via printf rather than interpolated into an ## unquoted heredoc — see the INI_TOKENS_OK note above for what that prevents. - { - echo '; rendered at container start by entrypoint-lsphp.sh' - printf 'error_log = "%s"\n' "/home/$user/logs/php-fpm/error.log" - echo 'log_errors = On' - } > "$SCAN_DIR/99-user-error-log.ini" + ## + ## Gated on INI_TOKENS_OK for the same reason the mapping below is: this + ## fragment interpolates $user into generated ini too. Leaving it ungated was + ## an INCONSISTENCY, not a live hole — a newline is inert inside the quotes, + ## and a `${`-bearing $user cannot exist because the useradd above would have + ## failed under `set -euo pipefail`. But "this particular unvetted value + ## happens to be contained" is the reasoning this branch already rejected one + ## screenful up, so it is not the reasoning that guards this line either. + ## + ## Rejecting costs such a user nothing it needs: `log_errors = On` is already + ## baked in by 99-prod-overrides.ini, so PHP still logs — to stderr, i.e. + ## `docker logs`, which is MORE visible than a per-site file, not less. No + ## legitimate user reaches this branch (verified fleet-wide: 30 shared_ols + ## sites across 4 hosts, none rejected by the charset check). + if [ "$INI_TOKENS_OK" = yes ]; then + { + echo '; rendered at container start by entrypoint-lsphp.sh' + printf 'error_log = "%s"\n' "/home/$user/logs/php-fpm/error.log" + echo 'log_errors = On' + } > "$SCAN_DIR/99-user-error-log.ini" + else + ## The container filesystem survives `docker restart`, so a fragment an + ## earlier boot wrote from a different env must not outlive the rejection. + rm -f "$SCAN_DIR/99-user-error-log.ini" + fi ## ---- $_SERVER path parity with cac-fpm ---- ## Point the cac_path_parity extension at THIS site's mapping. Same two ## values the compatibility symlink above is built from, so the rewrite and @@ -195,13 +244,50 @@ EOF fi ## Per-site opcache override (panel: Advanced Tuning → OpCache size); falls ## back to the baked lsphp-overrides.ini defaults when unset. - if [ -n "${OPCACHE_MEMORY_MB:-}" ] || [ -n "${OPCACHE_MAX_FILES:-}" ]; then + ## + ## SAME INJECTION CLASS AS THE MAPPING ABOVE, and it was left open when that + ## one was closed. These two lines interpolated the raw env into an UNQUOTED + ## `echo`, so (measured against the pre-fix script on this branch's image) + ## OPCACHE_MEMORY_MB=$'128\nprecision = 7\n; ' put `precision = 7` into + ## 99-user-opcache.ini and lsphp duly reported `precision => 7`. + ## + ## WHP does cast (int) and clamp these before setting the env + ## (web-files/libs/site-pool-env.php: 32-512 MB, 2000-32000 files) — but + ## "the panel validates it" is exactly the argument this branch rejected for + ## `domain`, and the panel is a different repo on a different release cadence. + ## Validate at the point of use, where the ini is actually generated. + ## + ## The accepted ranges below are PHP's OWN limits for these directives + ## (opcache refuses memory_consumption under 8 MB and clamps + ## max_accelerated_files into [200, 1000000]), deliberately a strict SUPERSET + ## of the panel's clamps: a value outside them could not have taken effect + ## anyway, and widening a panel clamp later can never start silently + ## rejecting real sites here. + OPCACHE_LINES=() + if [ -n "${OPCACHE_MEMORY_MB:-}" ]; then + validate_ini_num OPCACHE_MEMORY_MB "$OPCACHE_MEMORY_MB" 8 4096 + if [ -n "$INI_NUM" ]; then + OPCACHE_LINES+=("$(printf 'opcache.memory_consumption = "%s"' "$INI_NUM")") + fi + fi + if [ -n "${OPCACHE_MAX_FILES:-}" ]; then + validate_ini_num OPCACHE_MAX_FILES "$OPCACHE_MAX_FILES" 200 1000000 + if [ -n "$INI_NUM" ]; then + OPCACHE_LINES+=("$(printf 'opcache.max_accelerated_files = "%s"' "$INI_NUM")") + fi + fi + if [ "${#OPCACHE_LINES[@]}" -gt 0 ]; then { echo "; rendered at container start by entrypoint-lsphp.sh" echo "; per-site override from WHP whp.sites.opcache_*_override" - [ -n "${OPCACHE_MEMORY_MB:-}" ] && echo "opcache.memory_consumption = ${OPCACHE_MEMORY_MB}" - [ -n "${OPCACHE_MAX_FILES:-}" ] && echo "opcache.max_accelerated_files = ${OPCACHE_MAX_FILES}" + printf '%s\n' "${OPCACHE_LINES[@]}" } > "$SCAN_DIR/99-user-opcache.ini" + else + ## Nothing valid to say. Remove rather than leave whatever a previous boot + ## wrote — the env is the source of truth and the container filesystem + ## outlives a `docker restart`. Without this, an override that is later + ## cleared (or rejected) would keep applying from the stale fragment. + rm -f "$SCAN_DIR/99-user-opcache.ini" fi else ## No scan dir means none of the per-site ini drop-ins land — including the