Repository navigation
Drop the sh and runuser -c sudo grants: the file's own TODO has never been filed #417
Description
Activity
@QinXi-ai four pull requests in one night, and the reviews are on each of them. Three are approved; #413 has one blocking item and a baseline bump.
Below are three issues I am holding for you. This is a batch, not a queue: take them in any order, take one, or take none. Declining any of them costs nothing and I will not ask twice. I have assigned them so they show on your dashboard rather than only on these threads, and one word hands any of them back.
Every finding was re-measured today at
f5dffd8before I offered it, because an issue that no longer reproduces wastes an evening in the worst way.#417 — drop the
shandrunuser -csudo grants. Hard, and the largest security item on the tracker.packaging/sysknife-sudoers:111grantsNOPASSWD: /usr/bin/sh, and the file's own SECURITY NOTE says what that means: sudoers is not the boundary, the daemon is. There is aTODO(post-launch hardening)namingConfigureFirewalland the ssh key ops as the call sites holding it up, and that TODO has never been filed, so nothing tracks it. You are the obvious person: in #413 you wrote the pattern it needs, a helper with a fixed argv and two argument-restricted grants instead of a wildcard on the binary. TheAddAuthorizedKeypath has a property that must survive the refactor, and #145 is why.#416 — six
Ufw*actions still say "Ubuntu only". Easy. #384 moved ufw out of the Ubuntu fence and #412 makes Debian eligible, so all eight are in the Debian catalogue. You dropped the phrase from the two you touched in #415; the other six kept it, and that text reaches the model. The part worth doing is the third scope bullet, a test that derives the claim from the production lists so the next re-partitioning cannot leave prose behind.#346 —
ci-local.shmisses seven of the scripts CI runs. Medium. I re-ran the comparison today and the number in the title is still exact:CI runs 29 script(s); ci-local mentions 25 missing: cassette-replay-parity, codex-plugin-manifest, grub-kargs-edit, log-edit, mount-edit, no-secrets, rmswapSomebody running
ci-local.shbefore pushing gets a green board and a red PR. It was offered to @xianjianlf2 on 2 September and released this morning with nothing owed.On your four: the only pairwise conflict among them is
CHANGELOG.md, one hunk each, and I will resolve those at merge rather than making you rebase four times. Only #413 moves the test count, so merging it last means it is the only baseline regeneration anyone has to do.Correction to the comment above, before anyone acts on it.
I said I had assigned these so they would show on your dashboard. The assignment did not take. GitHub returns success for
POST /assigneesand then silently drops anyone who is not a repository collaborator and has not already posted on that specific issue, which is the case here. My tooling read the assignee list back afterwards, found it empty, and refused to report success, which is the only reason I know.So the reservation is the
claimedlabel plus this thread, and that is visible to everybody reading the tracker but not on your own dashboard. The moment you reply on one of these threads, the assignment becomes possible and I will apply it.Nothing else in that comment changes. The issues are held for you, and declining any of them still costs nothing.
I will take this. Before implementation, I enumerated daemon command construction at main 61b3a87 rather than relying on the TODO. The list is longer:
- sudo sh -c: ConfigureFirewall, AddUserToGroup, RemoveUserFromGroup, and SnapInstall when auto_update=false.
- sudo runuser -u ... -- sh -c: AddAuthorizedKey and RemoveAuthorizedKey.
- sudo runuser -l ... -c: six Podman actions and three Toolbox actions.
- The unrestricted runuser grant also serves the shell-free Flatpak paths; those must move to a bounded helper too before removing the grant.
- Separate unprivileged bash -c paths: AptListUpgradable, AptHistoryList, and CheckPendingReboot. Executor shell invocations below cfg(test) are process-control fixtures, not production action constructors.
I will replace the production shell paths with fixed argv/operation-specific helpers, retain the group-existence guards and snap install-then-hold failure sequencing, and drop supplementary groups/GID/UID to the resolved target account before any SSH key file access. User-scoped container/Flatpak execution will use an explicit target-user environment and fixed tool/subcommand allowlists, with no arbitrary command parameter. Both whole-binary grants will be removed and guarded against reintroduction. Codex is assisting; Linux/runtime evidence will be reported separately from Windows-local checks.
packaging/sysknife-sudoers:111carries this, and the file says plainly what itmeans:
The
runusergrant twenty lines below says the-cform "adds no privilegebeyond the existing
shgrant", which is true and is the other half of the sameknot.
This issue is that TODO. It has never been filed, so nothing tracks it and it
does not appear in any release checklist.
Nothing here is a disclosure: the grant is deliberate, documented in the file it
lives in, and the compensating controls are named. What is missing is a work item
with the remaining call sites enumerated.
What the grant is currently holding up
Two families, per the note:
ConfigureFirewallAddAuthorizedKeyandRemoveAuthorizedKey, which gothrough
runuser -u <user> -- sh -c '<script>' sh <key> <path>so the edithappens as the target user and a symlink planted at
~/.ssh/authorized_keyscannot redirect a root write. That property was won in AddAuthorizedKey follows an attacker symlink, appending as root anywhere #145 and must survive
any refactor: whatever replaces
sh -cstill has to drop privilege to theowning user before touching the file.
Start by enumerating every site rather than trusting this list. The grant is
whole-binary, so
grepfor the shell invocation incrates/sysknife-daemonrather than for the two action names.
Scope
sh -corrunuser -c, and sayin the issue thread what the real list is before writing code. If it is longer
than the note claims, that is the finding.
reason for the shell, a small helper in
packaging/with a fixed argv and anargument-restricted grant is the pattern;
packaging/sysknife-firewall-stateand its two
nft list rulesetgrants show the shape.reintroduces a root write to a user-owned path re-opens AddAuthorizedKey follows an attacker symlink, appending as root anywhere #145.
The removal is the deliverable; the refactor is the work.
Difficulty
Hard, and the largest security item on this tracker. It is also the one that
changes what the sudoers file means: today it documents that it is not the
boundary, and finishing this is what would let it become one.