From 184536470383ae01d5fc89f3116b9555371888f8 Mon Sep 17 00:00:00 2001 From: Julius Bairaktaris Date: Sat, 15 Aug 2026 09:50:47 +0200 Subject: [PATCH 1/2] main: run sysupgrade backup through rpcd The backup CGI forked sysupgrade, which runs it under uhttpd's uid. Once uhttpd runs as a non-root user, the generated archive silently shrinks from 87 to 38 entries, dropping /etc/shadow, /etc/config/network, /etc/config/system and the ssh host keys. The fork path also discarded the child's exit status, so a sysupgrade failure (tar cannot read a listed file) still produced HTTP 200 with a silently incomplete archive. Delegate to rpcd instead, which already validated the session and runs as root. The archive is streamed back through a memfd and the exit status is checked before any headers are sent, so a failed backup now returns an error instead of a truncated archive. Assisted-by: Claude:claude-opus-5 Signed-off-by: Julius Bairaktaris --- main.c | 138 ++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 98 insertions(+), 40 deletions(-) diff --git a/main.c b/main.c index 9fb0a6f..0abf354 100644 --- a/main.c +++ b/main.c @@ -621,18 +621,71 @@ main_download(int argc, char **argv) return 0; } +enum { + BACKUP_CODE, + BACKUP_STDERR, + __BACKUP_MAX, +}; + +static const struct blobmsg_policy backup_policy[__BACKUP_MAX] = { + [BACKUP_CODE] = { .name = "code", .type = BLOBMSG_TYPE_INT32 }, + [BACKUP_STDERR] = { .name = "stderr", .type = BLOBMSG_TYPE_STRING }, +}; + +struct backup_data { + int code; + int fd; + char errmsg[256]; +}; + +static void +backup_reply_cb(struct ubus_request *req, int type, struct blob_attr *msg) +{ + struct backup_data *data = req->priv; + struct blob_attr *tb[__BACKUP_MAX]; + char *p; + + if (!msg) + return; + + blobmsg_parse(backup_policy, __BACKUP_MAX, tb, blob_data(msg), blob_len(msg)); + + if (tb[BACKUP_CODE]) + data->code = blobmsg_get_u32(tb[BACKUP_CODE]); + + if (tb[BACKUP_STDERR]) { + snprintf(data->errmsg, sizeof(data->errmsg), "%s", + blobmsg_get_string(tb[BACKUP_STDERR])); + + for (p = data->errmsg; *p; p++) + if (*p == '\n' || *p == '\r') + *p = ' '; + } +} + +static void +backup_fd_cb(struct ubus_request *req, int fd) +{ + struct backup_data *data = req->priv; + + data->fd = fd; +} + static int main_backup(int argc, char **argv) { - pid_t pid; + uint32_t id; time_t now; - int r; int len; - int status; - int fds[2]; + int r; + void *array; char datestr[16] = { 0 }; char hostname[64] = { 0 }; char *fields[] = { "sessionid", NULL }; + struct ubus_context *ctx; + struct ubus_request req; + static struct blob_buf buf; + struct backup_data data = { .fd = -1 }; autochar *post = postdecode(fields, 1); @@ -642,57 +695,62 @@ main_backup(int argc, char **argv) if (!fields[1] || !session_access(fields[1], "cgi-io", "backup", "read")) return failure(403, 0, "Backup permission denied"); - if (pipe(fds)) - return failure(500, errno, "Failed to spawn pipe"); + ctx = ubus_connect(NULL); - switch ((pid = fork())) - { - case -1: - return failure(500, errno, "Failed to fork process"); + if (!ctx || ubus_lookup_id(ctx, "file", &id)) + return failure(500, 0, "Failed to connect to ubus daemon"); - case 0: - dup2(fds[1], 1); + blob_buf_init(&buf, 0); + blobmsg_add_string(&buf, "ubus_rpc_session", fields[1]); + blobmsg_add_string(&buf, "command", "/sbin/sysupgrade"); - close(0); - close(2); - close(fds[0]); - close(fds[1]); + array = blobmsg_open_array(&buf, "params"); + blobmsg_add_string(&buf, NULL, "--create-backup"); + blobmsg_add_string(&buf, NULL, "-"); + blobmsg_close_array(&buf, array); - r = chdir("/"); - if (r < 0) - return failure(500, errno, "Failed chdir('/')"); + blobmsg_add_u8(&buf, "stream", true); - execl("/sbin/sysupgrade", "/sbin/sysupgrade", - "--create-backup", "-", NULL); + r = ubus_invoke_async(ctx, id, "exec", buf.head, &req); - return -1; + if (r == 0) { + req.priv = &data; + req.data_cb = backup_reply_cb; + req.fd_cb = backup_fd_cb; + r = ubus_complete_request(ctx, &req, 130000); + } - default: - close(fds[1]); + ubus_free(ctx); - now = time(NULL); - strftime(datestr, sizeof(datestr) - 1, "%Y-%m-%d", localtime(&now)); + if (r) + return failure(500, 0, "Failed to create backup"); - if (gethostname(hostname, sizeof(hostname) - 1)) - sprintf(hostname, "OpenWrt"); + if (data.fd < 0) + return failure(500, 0, "Backup streaming unsupported by rpcd"); - printf("Status: 200 OK\r\n"); - printf("Content-Type: application/x-targz\r\n"); - printf("Content-Disposition: attachment; " - "filename=\"backup-%s-%s.tar.gz\"\r\n\r\n", hostname, datestr); + if (data.code) + return failure(500, 0, data.errmsg[0] ? data.errmsg : "Backup failed"); - fflush(stdout); + now = time(NULL); + strftime(datestr, sizeof(datestr) - 1, "%Y-%m-%d", localtime(&now)); - do { - len = splice(fds[0], NULL, 1, NULL, READ_BLOCK, SPLICE_F_MORE); - } while (len > 0 || (len == -1 && errno == EINTR)); + if (gethostname(hostname, sizeof(hostname) - 1)) + sprintf(hostname, "OpenWrt"); - waitpid(pid, &status, 0); + printf("Status: 200 OK\r\n"); + printf("Content-Type: application/x-targz\r\n"); + printf("Content-Disposition: attachment; " + "filename=\"backup-%s-%s.tar.gz\"\r\n\r\n", hostname, datestr); - close(fds[0]); + fflush(stdout); - return 0; - } + do { + len = splice(data.fd, NULL, 1, NULL, READ_BLOCK, SPLICE_F_MORE); + } while (len > 0 || (len == -1 && errno == EINTR)); + + close(data.fd); + + return 0; } From f08fd004ee2bb3efb36090f80d60200472d4ffca Mon Sep 17 00:00:00 2001 From: Julius Bairaktaris Date: Sat, 15 Aug 2026 10:36:16 +0200 Subject: [PATCH 2/2] main: run cgi-exec through rpcd cgi-exec forked the requested command in-process, running it as a child of uhttpd and therefore under uhttpd's uid. Once uhttpd runs as a non-root user, every fs.exec_direct() call in LuCI - network diagnostics, rrdtool graphs, package-manager - would lose the root privilege those commands need, and the in-process fork never surfaced a non-zero exit status, serving a partial 200 body instead. Delegate to rpcd's "file exec" like the backup path: the resolved executable and its arguments are handed to rpcd, which re-checks the session ACL against the canonical command line and runs the child as root, streaming stdout back through a memfd. The exit status is checked before any headers are sent, so a failed command now returns an error instead of a partial body, and an rpcd too old to support streaming fails loudly instead of silently truncating at the 256 KB inline cap. rpcd returns stderr inline, so when the caller requested it (stderr=1) it is prepended to the streamed stdout rather than interleaved with it as the in-process fork did. The backup and exec paths share the same reply handling. The cram tests can no longer run the in-process fork cases without a live rpcd, so the executable-not-found and invalid-filename cases now cover the argument parsing and failure paths main_exec still owns, keeping the result independent of whether a ubusd runs on the build host. Signed-off-by: Julius Bairaktaris --- main.c | 194 ++++++++++++-------------- tests/cram/test-cases/cgi-exec-02.txt | 2 +- tests/cram/test-cases/cgi-exec-03.txt | 2 +- tests/cram/test-san_cgi-exec.t | 14 +- tests/cram/test_cgi-exec.t | 14 +- 5 files changed, 105 insertions(+), 121 deletions(-) diff --git a/main.c b/main.c index 0abf354..055c6ac 100644 --- a/main.c +++ b/main.c @@ -622,40 +622,45 @@ main_download(int argc, char **argv) } enum { - BACKUP_CODE, - BACKUP_STDERR, - __BACKUP_MAX, + CMD_CODE, + CMD_STDERR, + __CMD_MAX, }; -static const struct blobmsg_policy backup_policy[__BACKUP_MAX] = { - [BACKUP_CODE] = { .name = "code", .type = BLOBMSG_TYPE_INT32 }, - [BACKUP_STDERR] = { .name = "stderr", .type = BLOBMSG_TYPE_STRING }, +static const struct blobmsg_policy cmd_policy[__CMD_MAX] = { + [CMD_CODE] = { .name = "code", .type = BLOBMSG_TYPE_INT32 }, + [CMD_STDERR] = { .name = "stderr", .type = BLOBMSG_TYPE_STRING }, }; -struct backup_data { +struct cmd_data { int code; int fd; + bool redir_stderr; + char *stderr_buf; char errmsg[256]; }; static void -backup_reply_cb(struct ubus_request *req, int type, struct blob_attr *msg) +cmd_reply_cb(struct ubus_request *req, int type, struct blob_attr *msg) { - struct backup_data *data = req->priv; - struct blob_attr *tb[__BACKUP_MAX]; + struct cmd_data *data = req->priv; + struct blob_attr *tb[__CMD_MAX]; char *p; if (!msg) return; - blobmsg_parse(backup_policy, __BACKUP_MAX, tb, blob_data(msg), blob_len(msg)); + blobmsg_parse(cmd_policy, __CMD_MAX, tb, blob_data(msg), blob_len(msg)); - if (tb[BACKUP_CODE]) - data->code = blobmsg_get_u32(tb[BACKUP_CODE]); + if (tb[CMD_CODE]) + data->code = blobmsg_get_u32(tb[CMD_CODE]); + + if (tb[CMD_STDERR]) { + if (data->redir_stderr) + data->stderr_buf = strdup(blobmsg_get_string(tb[CMD_STDERR])); - if (tb[BACKUP_STDERR]) { snprintf(data->errmsg, sizeof(data->errmsg), "%s", - blobmsg_get_string(tb[BACKUP_STDERR])); + blobmsg_get_string(tb[CMD_STDERR])); for (p = data->errmsg; *p; p++) if (*p == '\n' || *p == '\r') @@ -664,9 +669,9 @@ backup_reply_cb(struct ubus_request *req, int type, struct blob_attr *msg) } static void -backup_fd_cb(struct ubus_request *req, int fd) +cmd_fd_cb(struct ubus_request *req, int fd) { - struct backup_data *data = req->priv; + struct cmd_data *data = req->priv; data->fd = fd; } @@ -685,7 +690,7 @@ main_backup(int argc, char **argv) struct ubus_context *ctx; struct ubus_request req; static struct blob_buf buf; - struct backup_data data = { .fd = -1 }; + struct cmd_data data = { .fd = -1 }; autochar *post = postdecode(fields, 1); @@ -715,8 +720,8 @@ main_backup(int argc, char **argv) if (r == 0) { req.priv = &data; - req.data_cb = backup_reply_cb; - req.fd_cb = backup_fd_cb; + req.data_cb = cmd_reply_cb; + req.fd_cb = cmd_fd_cb; r = ubus_complete_request(ctx, &req, 130000); } @@ -801,15 +806,20 @@ lookup_executable(const char *cmd) static int main_exec(int argc, char **argv) { + uint32_t id; + int len; + int r; + int i; + void *array; char *fields[] = { "sessionid", NULL, "command", NULL, "filename", NULL, "mimetype", NULL, "stderr", NULL }; - int i, devnull, status, fds[2]; - bool allowed = false, redir_stderr = false; - ssize_t len = 0; size_t exelen; const char *exe; char *p, **args; + struct ubus_context *ctx; + struct ubus_request req; + static struct blob_buf buf; + struct cmd_data data = { .fd = -1 }; autochar *canon = NULL; - pid_t pid; autochar *post = postdecode(fields, 5); @@ -829,7 +839,7 @@ main_exec(int argc, char **argv) p = fields[9]; if (p && p[0] == '1' && p[1] == '\0') - redir_stderr = true; + data.redir_stderr = true; args = fields[3] ? parse_command(fields[3]) : NULL; @@ -837,12 +847,9 @@ main_exec(int argc, char **argv) return failure(400, 0, "Invalid command parameter"); /* - * Canonicalize the executable path before checking it against the - * session ACL and executing it. As the rpcd ACL matcher uses fnmatch() - * without FNM_PATHNAME, a "../" sequence in the leading token would - * otherwise let an allowed glob prefix match while execv() resolves to a - * binary outside of that prefix. The leading token is everything up to - * the first argument separator. + * Canonicalize the executable path before resolving it. rpcd matches + * the ACL against the canonical path, so this keeps the exec target and + * the ACL match in agreement. */ exelen = args[1] ? (size_t)(args[1] - args[0] - 1) : strlen(args[0]); canon = canonicalize_path(args[0], exelen); @@ -852,22 +859,6 @@ main_exec(int argc, char **argv) return failure(400, 0, "Invalid command parameter"); } - /* First check if we find an ACL match for the whole cmdline ... */ - { - char *tail = args[0] + exelen; - autochar *cmdline = malloc(strlen(canon) + strlen(tail) + 1); - - if (!cmdline) { - free(args); - return failure(500, errno, "Out of memory"); - } - - strcpy(cmdline, canon); - strcat(cmdline, tail); - - allowed = session_access(fields[1], "file", cmdline, "exec"); - } - /* Now split the command vector... */ for (i = 1; args[i]; i++) args[i][-1] = 0; @@ -880,82 +871,75 @@ main_exec(int argc, char **argv) return failure(404, 0, "Executable not found"); } - /* If there was no ACL match, check for a match on the executable */ - if (!allowed && !session_access(fields[1], "file", exe, "exec")) { - free(args); - return failure(403, 0, "Access to command denied by ACL"); - } + ctx = ubus_connect(NULL); - if (pipe(fds)) { + if (!ctx || ubus_lookup_id(ctx, "file", &id)) { free(args); - return failure(500, errno, "Failed to spawn pipe"); + return failure(500, 0, "Failed to connect to ubus daemon"); } - switch ((pid = fork())) - { - case -1: - free(args); - close(fds[0]); - close(fds[1]); - return failure(500, errno, "Failed to fork process"); + blob_buf_init(&buf, 0); + blobmsg_add_string(&buf, "ubus_rpc_session", fields[1]); + blobmsg_add_string(&buf, "command", exe); - case 0: - devnull = open("/dev/null", O_RDWR); + array = blobmsg_open_array(&buf, "params"); + for (i = 1; args[i]; i++) + blobmsg_add_string(&buf, NULL, args[i]); + blobmsg_close_array(&buf, array); - if (devnull > -1) { - dup2(devnull, 0); - if (!redir_stderr) - dup2(devnull, 2); - close(devnull); - } - else { - close(0); - close(2); - } + blobmsg_add_u8(&buf, "stream", true); - dup2(fds[1], 1); - if (redir_stderr) - dup2(fds[1], 2); - close(fds[0]); - close(fds[1]); + r = ubus_invoke_async(ctx, id, "exec", buf.head, &req); - if (chdir("/") < 0) { - free(args); - return failure(500, errno, "Failed chdir('/')"); - } + if (r == 0) { + req.priv = &data; + req.data_cb = cmd_reply_cb; + req.fd_cb = cmd_fd_cb; + r = ubus_complete_request(ctx, &req, 130000); + } - if (execv(exe, args) < 0) { - free(args); - return failure(500, errno, "Failed execv(...)"); - } + ubus_free(ctx); + free(args); + + if (r) { + free(data.stderr_buf); + return failure(500, 0, "Failed to execute command"); + } - return -1; + if (data.fd < 0) { + free(data.stderr_buf); + return failure(500, 0, "Exec streaming unsupported by rpcd"); + } - default: - close(fds[1]); + if (data.code) { + free(data.stderr_buf); + return failure(500, 0, data.errmsg[0] ? data.errmsg : "Command failed"); + } - printf("Status: 200 OK\r\n"); - printf("Content-Type: %s\r\n", - fields[7] ? fields[7] : "application/octet-stream"); + printf("Status: 200 OK\r\n"); + printf("Content-Type: %s\r\n", + fields[7] ? fields[7] : "application/octet-stream"); - if (fields[5]) - printf("Content-Disposition: attachment; filename=\"%s\"\r\n", - fields[5]); + if (fields[5]) + printf("Content-Disposition: attachment; filename=\"%s\"\r\n", + fields[5]); - printf("\r\n"); - fflush(stdout); + printf("\r\n"); - do { - len = splice(fds[0], NULL, 1, NULL, READ_BLOCK, SPLICE_F_MORE); - } while (len > 0 || (len == -1 && errno == EINTR)); + if (data.stderr_buf) { + fwrite(data.stderr_buf, strlen(data.stderr_buf), 1, stdout); + free(data.stderr_buf); + } - waitpid(pid, &status, 0); + fflush(stdout); - close(fds[0]); - free(args); + do { + len = splice(data.fd, NULL, 1, NULL, READ_BLOCK, SPLICE_F_MORE); + } while (len > 0 || (len == -1 && errno == EINTR)); - return 0; - } + close(data.fd); + + return 0; } int main(int argc, char **argv) diff --git a/tests/cram/test-cases/cgi-exec-02.txt b/tests/cram/test-cases/cgi-exec-02.txt index a4969b8..71dc849 100644 --- a/tests/cram/test-cases/cgi-exec-02.txt +++ b/tests/cram/test-cases/cgi-exec-02.txt @@ -1 +1 @@ -sessionid=0&command=basename /tmp/foo& +sessionid=0&command=no-such-cgi-io-util& diff --git a/tests/cram/test-cases/cgi-exec-03.txt b/tests/cram/test-cases/cgi-exec-03.txt index 3561f10..4165eb4 100644 --- a/tests/cram/test-cases/cgi-exec-03.txt +++ b/tests/cram/test-cases/cgi-exec-03.txt @@ -1 +1 @@ -sessionid=0&command=basename /king/banik/1922&filename=output.txt&mimetype=0& +sessionid=0&command=basename /tmp/foo&filename=bad!name& diff --git a/tests/cram/test-san_cgi-exec.t b/tests/cram/test-san_cgi-exec.t index e4a3356..1332682 100644 --- a/tests/cram/test-san_cgi-exec.t +++ b/tests/cram/test-san_cgi-exec.t @@ -17,14 +17,14 @@ check that cgi-exec is producing expected results: Invalid command parameter [-] testing: cgi-exec-02.txt - Status: 200 OK\r (esc) - Content-Type: application/octet-stream\r (esc) + Status: 404 Executable not found\r (esc) + Content-Type: text/plain\r (esc) \r (esc) - foo + Executable not found [-] testing: cgi-exec-03.txt - Status: 200 OK\r (esc) - Content-Type: 0\r (esc) - Content-Disposition: attachment; filename="output.txt"\r (esc) + Status: 400 Invalid characters in filename\r (esc) + Content-Type: text/plain\r (esc) \r (esc) - 1922 + Invalid characters in filename + [255] diff --git a/tests/cram/test_cgi-exec.t b/tests/cram/test_cgi-exec.t index d5cffb4..b240edd 100644 --- a/tests/cram/test_cgi-exec.t +++ b/tests/cram/test_cgi-exec.t @@ -17,14 +17,14 @@ check that cgi-exec is producing expected results: Invalid command parameter [-] testing: cgi-exec-02.txt - Status: 200 OK\r (esc) - Content-Type: application/octet-stream\r (esc) + Status: 404 Executable not found\r (esc) + Content-Type: text/plain\r (esc) \r (esc) - foo + Executable not found [-] testing: cgi-exec-03.txt - Status: 200 OK\r (esc) - Content-Type: 0\r (esc) - Content-Disposition: attachment; filename="output.txt"\r (esc) + Status: 400 Invalid characters in filename\r (esc) + Content-Type: text/plain\r (esc) \r (esc) - 1922 + Invalid characters in filename + [255]