From 6aef3cba31444323795fe9a0a0c780b0ec4d7486 Mon Sep 17 00:00:00 2001 From: Yolean k8s-qa Date: Wed, 30 Sep 2026 05:24:12 +0000 Subject: [PATCH 1/4] y-etc-hosts-write: bounded input, and outside names keep their address Two ways a NOPASSWD grantee could still use the root-run writer against the host, found reviewing #85 and reproduced with Y_ETC_HOSTS on scratch files (material in bots-state/code-review/prs/ystack-85/ on metalblack): - The block could give a name that root maintains outside it another address. The block's position is free, and hosts(5) lookups take the first match, so a block placed first with "127.0.0.9 db.internal" shadowed root's "10.1.1.1 db.internal" for every user of the host. A block entry for a name with an entry outside the block must now use that entry's address. A duplicate with the same address is still accepted, so a hand-made line for a cluster name keeps working. - stdin went to a temporary file next to /etc/hosts with no bound, as root, so `yes | sudo y-etc-hosts-write` could fill the root filesystem including its root reserve, before any check ran. A valid 46 MB block of comments was also accepted as /etc/hosts. Input is now limited to 1 MiB, far above what k8s-ingress-hosts writes. Verified with the PR head's probes rerun against this version: the shadowing input and the 46 MB input are refused (exit 2); route comments, same-address duplicates and a 40000-entry block are accepted; the PR's refusals (public address, edits outside the block) are unchanged. shellcheck and y-script-lint are clean. Co-Authored-By: Claude Opus 5.5 (1M context) --- bin/y-etc-hosts-write | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/bin/y-etc-hosts-write b/bin/y-etc-hosts-write index 4ae49068..db0e5e6c 100755 --- a/bin/y-etc-hosts-write +++ b/bin/y-etc-hosts-write @@ -19,8 +19,9 @@ the block k8s-ingress-hosts owns: everything outside the lines # generated using k8s-ingress-hosts end # must stay as it is in the current file, there is at most one such block, and each entry in it maps plain hostnames to a loopback or private IPv4 address (127/8, -10/8, 172.16/12, 192.168/16), optionally followed by a # comment. Other edits of -/etc/hosts are for root to make. +10/8, 172.16/12, 192.168/16), optionally followed by a # comment. A name that has +an entry outside the block keeps that address. Input is limited to 1 MiB. Other +edits of /etc/hosts are for root to make. Environment: Y_ETC_HOSTS Target path (default: /etc/hosts) @@ -56,7 +57,13 @@ fi TMP=$(mktemp "$TARGET_DIR/.hosts.XXXXXX") trap 'rm -f "$TMP"' EXIT -cat > "$TMP" +# Root writes this into the target's filesystem, so the input is bounded +MAX_BYTES=1048576 +head -c $((MAX_BYTES + 1)) > "$TMP" +if [ "$(wc -c < "$TMP")" -gt "$MAX_BYTES" ]; then + echo "ERROR: refusing input over $MAX_BYTES bytes" >&2 + exit 2 +fi if [ ! -s "$TMP" ]; then echo "ERROR: refusing to write an empty $TARGET" >&2 @@ -86,12 +93,22 @@ if [ -f "$TARGET" ] && ! cmp -s <(outside_block "$TARGET") <(outside_block "$TMP exit 2 fi +# Two passes over the input: the first records the addresses of names outside the block, +# so a block entry cannot give such a name another address (the block may come first) BAD_ENTRIES=$(awk -v s="$BLOCK_START" -v e="$BLOCK_END" ' function octet(x) { return x ~ /^[0-9]+$/ && x + 0 <= 255 } function allowed(ip, o) { if (split(ip, o, ".") != 4 || !octet(o[1]) || !octet(o[2]) || !octet(o[3]) || !octet(o[4])) return 0 return o[1] == 127 || o[1] == 10 || (o[1] == 172 && o[2] >= 16 && o[2] <= 31) || (o[1] == 192 && o[2] == 168) } + NR == FNR { + if ($0 == s) { inb = 1 } else if ($0 == e) { inb = 0 } + else if (!inb && $0 !~ /^[[:space:]]*(#|$)/) { + for (i = 2; i <= NF && $i !~ /^#/; i++) if (!($i in outside)) outside[$i] = $1 + } + next + } + FNR == 1 {inb=0} $0 == s {inb=1; next} $0 == e {inb=0; next} inb && $0 !~ /^[[:space:]]*(#|$)/ { @@ -101,11 +118,12 @@ BAD_ENTRIES=$(awk -v s="$BLOCK_START" -v e="$BLOCK_END" ' for (i = 2; i <= NF && $i !~ /^#/; i++) { names++ if ($i !~ /^[A-Za-z0-9]([-A-Za-z0-9.]*[A-Za-z0-9])?$/) ok = 0 + if (($i in outside) && outside[$i] != $1) ok = 0 } if (!ok || names == 0) print - }' "$TMP") + }' "$TMP" "$TMP") if [ -n "$BAD_ENTRIES" ]; then - echo "ERROR: refusing k8s-ingress-hosts entries that are not a loopback or private IPv4 address followed by hostnames:" >&2 + echo "ERROR: refusing k8s-ingress-hosts entries that are not a loopback or private IPv4 address followed by hostnames, or that give a name from outside the block another address:" >&2 echo "$BAD_ENTRIES" >&2 exit 2 fi From ee9bc6a063214eb49bae85b8bfd745b9d614371c Mon Sep 17 00:00:00 2001 From: Yolean k8s-qa Date: Wed, 30 Sep 2026 05:37:05 +0000 Subject: [PATCH 2/4] y-cluster-sudoers: refuse sources a grantee can write, not only own --write checked the owner of the two helpers and of bin/ only. On a noble VM (classic sudo 1.9.15p5) a grantee got root without a password in three ways the owner check does not see, all with a root-owned checkout: a helper group-writable by the grantee's group, bin/ group-writable (the grantee exchanged a helper with its own file between the check and the install, renameat2 RENAME_EXCHANGE, 1 in 13 runs), and a root-created symlink to a file the grantee owns. A grantee-owned directory above the checkout let it swap the whole tree during --write. Report and logs: bots-state/code-review/prs/ystack-85/vm/ on metalblack. The check now walks the helpers, this script and every directory above them (on the physical path, pwd -P), and refuses when a grantee owns one, can write one (as root: sudo -u test -w, so group, other and ACL grants count), or one is a symlink. With nothing on the path writable by a grantee, nothing can change between the check and the copy. The script cannot vouch for itself, which the usage now says: a grantee who could write it before the admin runs it runs as root, so the admin checks with the show mode first, as root, which reports the same. Co-Authored-By: Claude Opus 5.5 (1M context) --- bin/y-cluster-sudoers | 53 +++++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/bin/y-cluster-sudoers b/bin/y-cluster-sudoers index 72d16965..7b9a6a88 100755 --- a/bin/y-cluster-sudoers +++ b/bin/y-cluster-sudoers @@ -2,7 +2,7 @@ [ -z "$DEBUG" ] || set -x set -eo pipefail -YSTACK_HOME="$(cd "$(dirname "$0")/.." && pwd)" +YSTACK_HOME="$(cd "$(dirname "$0")/.." && pwd -P)" YBIN="$YSTACK_HOME/bin" SUDOERS_FILE="/etc/sudoers.d/ystack-cluster" # Root-owned snapshots of the helpers: a NOPASSWD rule for a file its grantee can edit @@ -25,8 +25,11 @@ Usernames: one or more users to grant NOPASSWD rules to. Defaults to the current user (via SUDO_USER or USER). Run --write from a checkout you trust at a commit you have reviewed, as it copies -the helpers that will run as root. It refuses a checkout owned by one of the -grantees, whose content they control, unless --allow-grantee-source is given. +the helpers that will run as root. It refuses when a grantee owns or can write the +helpers, this script or any directory above them, or when one of those is a symlink, +unless --allow-grantee-source is given. It cannot vouch for itself: a grantee who +could write this script before you run it runs as root, so check with the plain +(show) mode first, as root, which reports the same. Rerun it after updating ystack to refresh the copies; y-k8s-ingress-hosts warns when they differ. EOF @@ -77,15 +80,35 @@ file_owner() { esac } -# A grantee who owns the checkout could change a helper right before the copy is taken -grantee_owned_sources() { - local h owner u - for h in "${HELPERS[@]}" ""; do - owner=$(file_owner "$YBIN/$h") +# The helpers, this script, and every directory above them +source_paths() { + local p d + for p in "${HELPERS[@]/#/$YBIN/}" "$YBIN/y-cluster-sudoers"; do + d=$p + while :; do + echo "$d" + [ "$d" = / ] && break + d=$(dirname "$d") + done + done | sort -u +} + +# A grantee who owns or can write any source path can change a helper between this check +# and the copy, or swap the checkout itself; a symlink can point anywhere. Write access +# (group, other, ACL) is checked only as root, which --write is. +grantee_controlled_sources() { + local p owner u + while IFS= read -r p; do + if [ -L "$p" ]; then echo "$p is a symlink"; fi + owner=$(file_owner "$p") for u in "${USERS[@]}"; do - if [ "$owner" = "$u" ]; then echo "$YBIN/$h is owned by grantee $u"; fi + if [ "$owner" = "$u" ]; then + echo "$p is owned by grantee $u" + elif [ "$(id -u)" -eq 0 ] && sudo -n -u "$u" test -w "$p"; then + echo "$p is writable by grantee $u" + fi done - done + done < <(source_paths) } sudo_rs_check() { @@ -104,8 +127,8 @@ sudo_rs_check() { if [ "$MODE" = "show" ]; then rules echo "# --write would install: ${HELPERS[*]/#/$YBIN/} -> $PRIVILEGED_DIR/ (root:0 0755)" - GRANTEE_OWNED=$(grantee_owned_sources) - [ -z "$GRANTEE_OWNED" ] || { echo "# --write would refuse, a grantee controls the source:"; echo "$GRANTEE_OWNED" | sed 's/^/# /'; } + GRANTEE_CONTROLLED=$(grantee_controlled_sources) + [ -z "$GRANTEE_CONTROLLED" ] || { echo "# --write would refuse, a grantee controls the source:"; echo "$GRANTEE_CONTROLLED" | sed 's/^/# /'; } sudo_rs_check exit 0 fi @@ -116,10 +139,10 @@ if [ "$(id -u)" -ne 0 ]; then exit 1 fi -GRANTEE_OWNED=$(grantee_owned_sources) -if [ -n "$GRANTEE_OWNED" ] && [ "$ALLOW_GRANTEE_SOURCE" != "true" ]; then +GRANTEE_CONTROLLED=$(grantee_controlled_sources) +if [ -n "$GRANTEE_CONTROLLED" ] && [ "$ALLOW_GRANTEE_SOURCE" != "true" ]; then echo "ERROR: refusing to install root-run helpers from a checkout a grantee controls:" >&2 - echo "$GRANTEE_OWNED" >&2 + echo "$GRANTEE_CONTROLLED" >&2 echo "Run from your own ystack checkout, or pass --allow-grantee-source after reviewing it." >&2 exit 1 fi From 2dcac6cdc0d694ea97cf411bb14bcdcbfe91c54a Mon Sep 17 00:00:00 2001 From: Yolean k8s-qa Date: Wed, 30 Sep 2026 05:42:47 +0000 Subject: [PATCH 3/4] y-etc-hosts-write: compare names outside the block case-insensitively 6aef3cb refused a block entry that gives a name from outside the block another address, comparing names byte for byte. Host lookups ignore case, so "127.0.0.9 DB.INTERNAL" in the block was accepted and won over root's "10.1.1.1 db.internal": getent hosts and getaddrinfo both returned 127.0.0.9 first, and ping went there, whether the block came first or last (VM run, bots-state/code-review/prs/ystack-85/vm-fix/ 09b-case-bypass.log on metalblack). Both sides are now lowercased. Verified on scratch targets: DB.INTERNAL at the top and Db.Internal at the end refused (exit 2); "10.1.1.1 DB.internal" and "127.0.0.1 LOCALHOST" (same addresses as outside) and a route-comment block accepted. Co-Authored-By: Claude Opus 5.5 (1M context) --- bin/y-etc-hosts-write | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/bin/y-etc-hosts-write b/bin/y-etc-hosts-write index db0e5e6c..1f0c8916 100755 --- a/bin/y-etc-hosts-write +++ b/bin/y-etc-hosts-write @@ -104,7 +104,7 @@ BAD_ENTRIES=$(awk -v s="$BLOCK_START" -v e="$BLOCK_END" ' NR == FNR { if ($0 == s) { inb = 1 } else if ($0 == e) { inb = 0 } else if (!inb && $0 !~ /^[[:space:]]*(#|$)/) { - for (i = 2; i <= NF && $i !~ /^#/; i++) if (!($i in outside)) outside[$i] = $1 + for (i = 2; i <= NF && $i !~ /^#/; i++) if (!(tolower($i) in outside)) outside[tolower($i)] = $1 } next } @@ -118,7 +118,8 @@ BAD_ENTRIES=$(awk -v s="$BLOCK_START" -v e="$BLOCK_END" ' for (i = 2; i <= NF && $i !~ /^#/; i++) { names++ if ($i !~ /^[A-Za-z0-9]([-A-Za-z0-9.]*[A-Za-z0-9])?$/) ok = 0 - if (($i in outside) && outside[$i] != $1) ok = 0 + # Host lookups ignore case, so DB.INTERNAL is db.internal + if ((tolower($i) in outside) && outside[tolower($i)] != $1) ok = 0 } if (!ok || names == 0) print }' "$TMP" "$TMP") From 943bfc4013e4214b2371c1bbc8d62b075c28b0c0 Mon Sep 17 00:00:00 2001 From: Yolean k8s-qa Date: Wed, 30 Sep 2026 05:42:47 +0000 Subject: [PATCH 4/4] y-cluster-sudoers: do not advise running the check as root ee9bc6a's usage told the admin to check with the show mode as root first. On the VM a grantee who could write bin/ replaced this script, and that advice ran the grantee's line as root before any report (prs/ystack-85/vm-fix/07-show-as-root.log on metalblack). The script cannot check itself, so the usage now says: show mode as yourself for what a grantee owns, and write access checked without executing the checkout (ls -ld on the path) before sudo. Co-Authored-By: Claude Opus 5.5 (1M context) --- bin/y-cluster-sudoers | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/bin/y-cluster-sudoers b/bin/y-cluster-sudoers index 7b9a6a88..cd0a6819 100755 --- a/bin/y-cluster-sudoers +++ b/bin/y-cluster-sudoers @@ -27,9 +27,10 @@ Usernames: one or more users to grant NOPASSWD rules to. Run --write from a checkout you trust at a commit you have reviewed, as it copies the helpers that will run as root. It refuses when a grantee owns or can write the helpers, this script or any directory above them, or when one of those is a symlink, -unless --allow-grantee-source is given. It cannot vouch for itself: a grantee who -could write this script before you run it runs as root, so check with the plain -(show) mode first, as root, which reports the same. +unless --allow-grantee-source is given. It cannot vouch for itself: if a grantee +could write this script, running it as root in any mode runs their code. The show +mode run as yourself lists what a grantee owns; group and ACL write access you check +without executing the checkout, e.g. ls -ld on the path, before running it with sudo. Rerun it after updating ystack to refresh the copies; y-k8s-ingress-hosts warns when they differ. EOF