Repository navigation
LLM-review sudoers setup - #86
Merged
Merged
Conversation
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) <noreply@anthropic.com>
--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 <grantee> 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Collaborator
Author
|
worked |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For #85
Three ways the grantee still got root or hurt the host, each reproduced: (1) the --write source check only looked at owners, so a group-writable helper or bin/, a root-made symlink to a bot file, or a bot-owned parent directory each got bot's code installed as root. (2) y-etc-hosts-write copied stdin unbounded as root; a killed sudo left a root cat that filled / to 100 %, and an 80 MB /etc/hosts made every lookup take 11 s. (3) A block entry could re-address a name root keeps outside the block; getaddrinfo preferred bot's 127.0.0.9 db.internal over root's 10.1.1.1 wherever the block sat, also as DB.INTERNAL.