Skip to content

luci-mod-system: permit sysupgrade --create-backup over rpcd - #8947

Open
JuliusBairaktaris wants to merge 2 commits into
openwrt:masterfrom
JuliusBairaktaris:backup-create-acl
Open

luci-mod-system: permit sysupgrade --create-backup over rpcd#8947
JuliusBairaktaris wants to merge 2 commits into
openwrt:masterfrom
JuliusBairaktaris:backup-create-acl

Conversation

@JuliusBairaktaris

Copy link
Copy Markdown
Contributor

cgi-io now delegates archive creation to rpcd's file exec method so that it keeps working when uhttpd runs as a non-root user (see openwrt/openwrt#24558). rpcd gates the exec on the caller's session ACL, so the luci-mod-system-flash ACL must permit the exact command line /sbin/sysupgrade --create-backup -.

Pairs with the cgi-io change.

cgi-io now delegates archive creation to rpcd's file exec method so that
it keeps working when uhttpd runs as a non-root user. rpcd gates the
exec on this session ACL entry.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
Copilot AI lite review requested due to automatic review settings August 15, 2026 07:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@JuliusBairaktaris

Copy link
Copy Markdown
Contributor Author

@jow- — review ping.

One-line ACL grant permitting exactly /sbin/sysupgrade --create-backup -, in the same luci-mod-system-flash read section as the existing --list-backup grant. rpcd matches the bare executable first and then executable + " " + each param, so this is an exact match with no glob metacharacters.

Needed by openwrt/cgi-io#6, which is in turn needed by openwrt/openwrt#24558.

The firewall status page prefers iptables-legacy-save and
ip6tables-legacy-save whenever those binaries are present, falling back
to the non-legacy pair otherwise, but only the fallback pair is in the
session ACL.

That is invisible today because fs.exec_direct reaches cgi-io, which
authorises the cgi-io object as a whole rather than the command it is
asked to run. Once cgi-exec dispatches through rpcd, the command itself
is matched against the session ACL and an ungranted path is denied, so a
device with the legacy binaries installed would lose its ruleset view.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 new commits. Both commit messages match their diffs, both files stay valid JSON, and the ACL strings are in the correct read sections. Two inline notes — the main one is a question about whether rpcd's file.exec can actually carry a backup tarball, since it is fully buffered and returns stdout as a NUL-terminated string.


Generated by Claude Code

"/proc/sys/kernel/hostname": [ "read" ],
"/sbin/sysupgrade --list-backup": [ "exec" ]
"/sbin/sysupgrade --list-backup": [ "exec" ],
"/sbin/sysupgrade --create-backup -": [ "exec" ]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ACL string itself looks right: rpcd checks the bare executable first and then executable + " " + each param joined with spaces (rpc_file_exec_run() file.c:1055-1083), so this is an exact match for today's execl("/sbin/sysupgrade", "/sbin/sysupgrade", "--create-backup", "-", NULL) in cgi-io, and the read section next to --list-backup is the right home for it.

What I can't square is the transport. --create-backup - emits a gzip stream on stdout, and rpcd's file.exec cannot carry that payload:

  • stdout is copied into a NUL-terminated blobmsg string (rpc_ustream_to_blobmsg() file.c:889-909), so the archive is cut at the first NUL byte;
  • the ustream read buffer is capped at RPC_FILE_MAX_SIZE = 4096 * 64 = 256 KB (file.c:46, file.c:57-59), and filling it aborts the call with UBUS_STATUS_NOT_SUPPORTED (file.c:982-989);
  • rpcd master has no fd-passing/streaming exec variant, so there is no way around the buffer.

That limit is precisely why cgi-io exists in the first place — fs.exec_direct() docs, fs.js:404-407 describe /cgi-bin/cgi-exec as the path for "large command outputs which might exceed the ubus message size limits or which contain binary data", and flash.js:83 posts the backup form straight at cgi-backup.

So: does openwrt/cgi-io#6 still spawn and stream the child's stdout to the HTTP client itself, using rpcd only for the privileged spawn / ACL check, or does it go through ubus call file exec? If the latter, any backup over 256 KB (i.e. most of them) fails outright and smaller ones come back truncated at the first NUL. I couldn't read that PR from here, so this may already be handled — worth stating in the commit message either way, since the ACL grant is the only visible half.

The same question applies to the luci-mod-status hunk in this PR: iptables-save / ip6tables-save output on a large ruleset also runs past 256 KB.


Generated by Claude Code

"/usr/sbin/iptables-save": [ "exec" ],
"/usr/sbin/ip6tables-save": [ "exec" ]
"/usr/sbin/ip6tables-save": [ "exec" ],
"/usr/sbin/iptables-legacy-save": [ "exec" ],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the PR title and description only cover the luci-mod-system backup grant; this second commit adds luci-mod-status entries that aren't mentioned anywhere in the body. Worth widening the title/description (or splitting), so the merge doesn't look like it touched an unrelated module.

For the record the grant itself checks out: nftables.js:163-173`` probes /usr/sbin/iptables-legacy-save with `fs.stat()` (covered by the `/*: [ "list" ]` grant in `luci-base`) and then `fs.exec_direct()`s the bare path with no params, which matches the bare-executable check in rpcd. Both alternatives are real install paths (iptables/Makefile:65 and :485), and although they are symlinks to `xtables-legacy-multi`, rpcd's exec path only canonicalizes textually and never resolves symlinks, so the grant on the link name is the correct one.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants