[24.10]: Chery pick changes to openwrt-24.10#36
Merged
Conversation
Async exec replies currently tear down their context directly from ustream callbacks. When the callback chain keeps running after the reply path frees the context, rpcd can hit a use-after-free and crash with SIGSEGV. Defer reply completion through a 0 ms uloop timeout and guard against duplicate scheduling so cleanup runs after the callbacks unwind in both exec paths. Signed-off-by: Meng <x.meng@genexis.eu> (cherry picked from commit e655a0d)
rpc_touch_session() computes `ses->timeout * 1000` as int*int, which overflows INT_MAX once ses->timeout exceeds 2147483 seconds (~24.85 days). The wrapped-around negative value passed to uloop_timeout_set() causes libubox to fire the session timeout callback on the very next uloop iteration, destroying the just-created session before the caller can use it. In practice this makes `ubus call session login` with e.g. `timeout:2592000` (30 days) return a valid-looking session id whose reported `expires` is a large negative number, and any subsequent session.set / session.get call on that SID returns "Not found". LuCI's ucode dispatcher passes the value of `luci.sauth.sessiontime` straight through as the login timeout, so users who follow common advice to bump sessiontime to 30 days get silently locked out of LuCI (uhttpd logs `accepted login`, response is 403 with no Set-Cookie) while SSH keeps working with the same credentials. At least one affected user resorted to factory-resetting multiple APs before recovering, see https://forum.openwrt.org/t/241892 . The same overflow lurks in rpc_session_from_blob() when thawing a persisted session, where `blobmsg_get_u64(EXPIRES) * 1000` is passed to uloop_timeout_set()'s int `msecs` parameter. Introduce a small helper that converts a seconds value to milliseconds with clamping to INT_MAX (and negative-input guard), and use it at both call sites. The cap still allows uloop timeouts of ~24.85 days, which is longer than any realistic administrative session. Signed-off-by: Breeze <chanlikessummer@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> (cherry picked from commit d005c88)
rpc_exec_process_cb() closes the stdout/stderr read descriptors after the child has exited, and rpc_exec_reply() closes them once more during teardown. ustream_free() neither closes nor resets the stored fd, so both code paths operate on the same descriptor number. In the window between the two closes the daemon may open another descriptor that reuses the freed number, which rpc_exec_reply() would then close by mistake. Reset the descriptors to -1 after closing them in the process callback so the later close() in rpc_exec_reply() becomes a harmless no-op. Assisted-by: Claude:claude-opus-4-8 Link: openwrt#34 Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> (cherry picked from commit 26dba52)
rpc_uci_apply() reads the "timeout" argument as a uint32 into an int and scales it with "timeout * 1000" before passing it to uloop_timeout_set(). A large value overflows the signed multiplication, and a value with the high bit set wraps to a negative int; both cases produce a negative millisecond value that schedules the rollback timer in the past, firing it immediately instead of after the requested window. Clamp the millisecond value to a sane range, consistent with the session timeout and -t argument overflow fixes. Assisted-by: Claude:claude-opus-4-8 Link: openwrt#34 Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> (cherry picked from commit af5d6f4)
Several handlers fork and, in the child, fall back to "return <ubus status>" when open()/malloc()/execv() etc. fail. Returning a ubus status code from the child does not terminate it: control unwinds back into the ubus dispatch loop and the child keeps running as a duplicate rpcd, racing the parent on the ubus socket and pipes. Replace these child-side returns with _exit(127) so a child that cannot exec the target simply terminates, which is also what the parent's WEXITSTATUS based reporting expects. Affected paths: exec.c (rpc_exec), file.c (rpc_file_exec_run), sys.c (rpc_cgi_password_set) and plugin.c (rpc_plugin_register_exec). Assisted-by: Claude:claude-opus-4-8 Link: openwrt#34 Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> (cherry picked from commit 3037a0e)
rpc_file_write() computes data_len as blobmsg_data_len() - 1, which is 0
for an empty "data" string. With base64 enabled it then called
b64_decode(data, data, 0); b64_decode() asserts dest size > 0, so a
request like file.write {"data":"","base64":true} aborts the daemon when
libubox is built with assertions enabled (and is a pointless call
otherwise).
Skip the decode when there is no data; the file is still created/
truncated and zero bytes are written, which is the correct result.
Assisted-by: Claude:claude-opus-4-8
Link: openwrt#34
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
(cherry picked from commit 79c8087)
rpc_ucode_blob_array_to_ucv()/rpc_ucode_blob_to_ucv() recurse once per nesting level of the incoming request message, which is fully attacker controlled. ubus permits messages up to UBUS_MAX_MSGLEN (1 MiB) and libubox enforces no nesting limit, so a deeply nested array/table argument to a ucode plugin method can drive tens of thousands of recursion levels and overflow the stack, crashing rpcd. Thread a depth counter through the conversion and stop descending past RPC_UCODE_MAX_NESTING (32) levels, which is far beyond any legitimate ubus message. Over-deep subtrees become null values instead of crashing. Assisted-by: Claude:claude-opus-4-8 Link: openwrt#34 Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> (cherry picked from commit cd1d958)
rc_list() allocates a struct rc_list_context with calloc() and defers the request; rc_list_readdir() walks /etc/init.d asynchronously and, once the directory is exhausted (or the requested entry was found), sends the reply and completes the deferred request -- but never frees the context. free(c) only existed in the early opendir() failure path, so every successful "rc list" ubus call leaked the context (which embeds a PATH_MAX sized path buffer), slowly exhausting memory. Free the context in the terminal path after completing the request. Assisted-by: Claude:claude-opus-4-8 Link: openwrt#34 Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> (cherry picked from commit d06d2a8)
rc_list() stored the "name" argument as a bare pointer into the parsed request message and then deferred the request. The init.d directory is walked asynchronously across multiple uloop iterations (each script's "running" check is a forked child), during which the single per-context ubus receive buffer that backs the message is overwritten by any other incoming ubus request. c->req_name then points at unrelated/freed data and is compared against directory entries, leading to wrong matches or a crash. Duplicate the name into the context and release it when the request completes. Assisted-by: Claude:claude-opus-4-8 Link: openwrt#34 Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> (cherry picked from commit 0de6668)
rc_list() built the reply in a function-local "static struct blob_buf", shared by every invocation, and kept using it across the deferred, asynchronous directory walk. Two overlapping "rc list" requests (the walk waits on forked children, so a second request is easily interleaved) would both blob_buf_init() and append to the same buffer, corrupting each other's reply or crashing. Move the blob_buf into the per-request context so each request owns its own buffer, and free it when the request completes. Assisted-by: Claude:claude-opus-4-8 Link: openwrt#34 Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> (cherry picked from commit 75470f4)
The file plugin matched ACL grants against the textual, canonicalized path but then let open()/stat()/opendir() follow symlinks unchecked. A symlink placed inside an ACL-covered directory therefore let a grant on the link authorize read/write/list/stat access to whatever file it pointed to, including files entirely outside the granted scope. Re-resolve the path with realpath() and re-run the ACL check against the resolved target whenever it differs from the canonicalized string, for every operation that dereferences the final component (read, write, md5, list, stat). lstat and remove are left untouched since they either need to see the link itself or already handle it safely via unlink()'s no-follow semantics. Newly created files (file.write) and dangling symlinks pointing outside the granted scope are handled via the containing directory. Also mask file.write's mode parameter to 0777 and drop the local umask(0) override, and authorize recursive file.remove per-entry instead of only at the top-level path. Fixes GHSA-q5gr-86pq-vvwr. Signed-off-by: Jo-Philipp Wich <jo@mein.io> (cherry picked from commit e37ed9d)
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
4 participants
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.
No description provided.