Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,9 @@
"/usr/sbin/ip6tables --line-numbers -w -nvxL -t *": [ "exec" ],
"/usr/sbin/ip6tables": [ "list" ],
"/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

"/usr/sbin/ip6tables-legacy-save": [ "exec" ]
},
"ubus": {
"file": [ "stat" ]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,8 @@
"/proc/mtd": [ "read" ],
"/proc/partitions": [ "read" ],
"/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

},
"ubus": {
"file": [ "exec", "read", "stat" ]
Expand Down