[PATCH RFC procd] service: add "init_action" ubus method for /etc/init.d/ scripts

[Jo-Philipp Wich]

Hi,

in addition to the inline comments, I'd personally prefer to see such
functionality as rpcd plugin, not in procd itself.

Not all init scripts are procd enabled and not all procd processes have init
scripts, due to that I think that init script enumeration methods should
reside elsewhere since they're neither needed by procd itself nor by init
scripts starting procd processes.

~ Jo

On 6/22/20 5:14 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal at milecki.pl>
> 
> This methods allows executing /etc/init.d/ scripts with requested
> action. It's useful for all kind of UIs (e.g. LuCI) and custom apps.
> 
> The next step should be "init_list" method support listing all init.d
> scripts.
> 
> Signed-off-by: Rafał Miłecki <rafal at milecki.pl>
> ---
>  service/service.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 96 insertions(+), 1 deletion(-)
> 
> diff --git a/service/service.c b/service/service.c
> index fcf0215..47fc88a 100644
> --- a/service/service.c
> +++ b/service/service.c
> @@ -12,8 +12,11 @@
>   * GNU General Public License for more details.
>   */
>  
> -#include <sys/types.h>
> +#include <fcntl.h>
> +#include <linux/limits.h>
>  #include <sys/stat.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
>  #include <unistd.h>
>  
>  #include <libubox/blobmsg_json.h>
> @@ -292,6 +295,17 @@ static const struct blobmsg_policy container_console_policy[__CONTAINER_CONSOLE_
>  	[CONTAINER_CONSOLE_INSTANCE] = { "instance", BLOBMSG_TYPE_STRING },
>  };
>  
> +enum {
> +	INIT_ACTION_NAME,
> +	INIT_ACTION_ACTION,
> +	__INIT_MAX
> +};
> +
> +static const struct blobmsg_policy init_action_policy[] = {
> +	[INIT_ACTION_NAME] = { "name", BLOBMSG_TYPE_STRING },
> +	[INIT_ACTION_ACTION] = { "action", BLOBMSG_TYPE_STRING },
> +};
> +
>  static inline bool is_container_obj(struct ubus_object *obj)
>  {
>  	return (obj && (strcmp(obj->name, "container") == 0));
> @@ -785,6 +799,86 @@ err_console_fd:
>  }
>  
>  
> +struct service_init_context {
> +	struct uloop_process process;
> +	struct ubus_context *ctx;
> +	struct ubus_request_data req;
> +};
> +
> +static void service_init_action_cb(struct uloop_process *p, int stat)
> +{
> +	struct service_init_context *c = container_of(p, struct service_init_context, process);
> +
> +	ubus_complete_deferred_request(c->ctx, &c->req, UBUS_STATUS_OK);
> +
> +	free(c);
> +}
> +
> +static int service_init_action(struct ubus_context *ctx,
> +			       struct ubus_object *obj,
> +			       struct ubus_request_data *req,
> +			       const char *method,
> +			       struct blob_attr *msg)
> +{
> +	struct blob_attr *tb[__INIT_MAX];
> +	struct service_init_context *c;
> +	struct stat s;
> +	char script[PATH_MAX];
> +	const char *action;
> +	pid_t pid;
> +	int fd;
> +
> +	blobmsg_parse(init_action_policy, __INIT_MAX, tb, blobmsg_data(msg), blobmsg_data_len(msg));
> +
> +	if (!tb[INIT_ACTION_NAME] || !tb[INIT_ACTION_ACTION])
> +		return UBUS_STATUS_INVALID_ARGUMENT;
> +
> +	snprintf(script, sizeof(script), "/etc/init.d/%s", blobmsg_get_string(tb[INIT_ACTION_NAME]));

This allows executing arbitrary commands with root permissions due to a simple
path traversal, e.g. when passing a "name" attribute value like
"../../sbin/reboot". Granted, exploitability is somewhat reduced due to the
restricted amount of possible arguments, still you should probably at the very
least reject values with slashes.

> +	if (stat(script, &s) || !(s.st_mode & S_IXUSR))
> +		return UBUS_STATUS_NOT_FOUND;

I think we should also check that the script in question belongs to root and
that it is only writable by the owner, to avoid executing script code
modifiable by unprivileged users.

> +
> +	action = blobmsg_get_string(tb[INIT_ACTION_ACTION]);
> +	if (strcmp(action, "disable") &&
> +	    strcmp(action, "enable") &&
> +	    strcmp(action, "stop") &&
> +	    strcmp(action, "start") &&
> +	    strcmp(action, "restart") &&
> +	    strcmp(action, "reload"))
> +		return UBUS_STATUS_INVALID_ARGUMENT;
> +
> +	c = calloc(1, sizeof(*c));
> +	if (!c)
> +		return UBUS_STATUS_UNKNOWN_ERROR;
> +
> +	pid = fork();
> +	switch (pid) {
> +	case -1:
> +		free(c);
> +		return UBUS_STATUS_UNKNOWN_ERROR;
> +	case 0:
> +		/* Set stdin, stdout & stderr to /dev/null */
> +		fd = open("/dev/null", O_RDWR);
> +		if (fd >= 0) {
> +			dup2(fd, 0);
> +			dup2(fd, 1);
> +			dup2(fd, 2);
> +			close(fd);
> +		}
> +
> +		execl(script, script, action, NULL);
> +		exit(errno);
> +	default:
> +		c->ctx = ctx;
> +		c->process.pid = pid;
> +		c->process.cb = service_init_action_cb;
> +		uloop_process_add(&c->process);

An additional timeout might make sense here after which the invoked process is
forcibly killed. Non procd compliant init scripts might execute completely
unintended logic when invoked with one of the arguments above, there is no
guarantee that they return quickly, or return at all.

> +
> +		ubus_defer_request(ctx, req, &c->req);
> +
> +		return 0; /* Deferred */
> +	}
> +}
> +
>  static struct ubus_method main_object_methods[] = {
>  	UBUS_METHOD("set", service_handle_set, service_set_attrs),
>  	UBUS_METHOD("add", service_handle_set, service_set_attrs),
> @@ -797,6 +891,7 @@ static struct ubus_method main_object_methods[] = {
>  	UBUS_METHOD("validate", service_handle_validate, validate_policy),
>  	UBUS_METHOD("get_data", service_get_data, get_data_policy),
>  	UBUS_METHOD("state", service_handle_state, service_state_attrs),
> +	UBUS_METHOD("init_action", service_init_action, init_action_policy),
>  };
>  
>  static struct ubus_object_type main_object_type =
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20200622/d958e637/attachment-0001.sig>
-------------- next part --------------
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
http://lists.infradead.org/mailman/listinfo/openwrt-devel