On Thu, Jun 01, 2017 at 02:26:16PM +0200, Cédric Bosdonnat wrote:
> virCommand is a version of virExec that doesn't fork, however it is
> just calling execve and doesn't honors setting uid/gid and pwd.
>
> This commit moves those pieces from virExec to virCommandExec and
> makes virExec use virCommandExec to avoid code duplication.
> ---
> src/util/vircommand.c | 92 ++++++++++++++++++++++++---------------------------
> 1 file changed, 43 insertions(+), 49 deletions(-)
>
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index e1bbc0526..aa97a5a10 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -481,21 +481,18 @@ virExec(virCommandPtr cmd)
> int childerr = -1;
> int tmpfd;
> char *binarystr = NULL;
> - const char *binary = NULL;
> int ret;
> struct sigaction waxon, waxoff;
> - gid_t *groups = NULL;
> - int ngroups;
>
> if (cmd->args[0][0] != '/') {
> - if (!(binary = binarystr = virFindFileInPath(cmd->args[0]))) {
> + if (!(binarystr = virFindFileInPath(cmd->args[0]))) {
> virReportSystemError(ENOENT,
> _("Cannot find '%s' in path"),
> cmd->args[0]);
> return -1;
> }
> - } else {
> - binary = cmd->args[0];
> + VIR_FREE(cmd->args[0]);
> + cmd->args[0] = binarystr;
> }
>
> if (childin < 0) {
> @@ -556,9 +553,6 @@ virExec(virCommandPtr cmd)
> childerr = null;
> }
>
> - if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0)
> - goto cleanup;
> -
> pid = virFork();
>
> if (pid < 0)
> @@ -577,9 +571,6 @@ virExec(virCommandPtr cmd)
>
> cmd->pid = pid;
>
> - VIR_FREE(binarystr);
> - VIR_FREE(groups);
> -
> return 0;
> }
>
> @@ -727,29 +718,6 @@ virExec(virCommandPtr cmd)
> }
> # endif
>
> - /* The steps above may need to do something privileged, so we delay
> - * setuid and clearing capabilities until the last minute.
> - */
> - if (cmd->uid != (uid_t)-1 || cmd->gid != (gid_t)-1 ||
> - cmd->capabilities || (cmd->flags & VIR_EXEC_CLEAR_CAPS)) {
> - VIR_DEBUG("Setting child uid:gid to %d:%d with caps %llx",
> - (int)cmd->uid, (int)cmd->gid, cmd->capabilities);
> - if (virSetUIDGIDWithCaps(cmd->uid, cmd->gid, groups, ngroups,
> - cmd->capabilities,
> - !!(cmd->flags & VIR_EXEC_CLEAR_CAPS)) < 0) {
> - goto fork_error;
> - }
> - }
> -
> - if (cmd->pwd) {
> - VIR_DEBUG("Running child in %s", cmd->pwd);
> - if (chdir(cmd->pwd) < 0) {
> - virReportSystemError(errno,
> - _("Unable to change to %s"), cmd->pwd);
> - goto fork_error;
> - }
> - }
> -
> if (virCommandHandshakeChild(cmd) < 0)
> goto fork_error;
With this change, the setuid + chdir will now take place after we have
done the handshake with the parent. I think this will cause potential
problems. First the child will still be runing privileged at the point
where the parent synchronizes, which may violate some assumptions.
Second, it means we loose any useful error reoprting when setuid or
chdir fail, which is pretty bad.
>
> @@ -771,15 +739,10 @@ virExec(virCommandPtr cmd)
> /* Close logging again to ensure no FDs leak to child */
> virLogReset();
This means that the reset logging before we've done the setuid/chdir,
so we loose any logging of these methods too
>
> - if (cmd->env)
> - execve(binary, cmd->args, cmd->env);
> - else
> - execv(binary, cmd->args);
> + if (virCommandExec(cmd) == -2)
> + goto fork_error;
>
> ret = errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE;
> - virReportSystemError(errno,
> - _("cannot execute binary %s"),
> - cmd->args[0]);
>
> fork_error:
> virDispatchError(NULL);
> @@ -789,9 +752,6 @@ virExec(virCommandPtr cmd)
> /* This is cleanup of parent process only - child
> should never jump here on error */
>
> - VIR_FREE(groups);
> - VIR_FREE(binarystr);
> -
> /* NB we don't virReportError() on any failures here
> because the code which jumped here already raised
> an error condition which we must not overwrite */
> @@ -2150,23 +2110,57 @@ virCommandProcessIO(virCommandPtr cmd)
> * in the hook after already forking / cloning, so does not attempt to
> * daemonize or preserve any FDs.
> *
> - * Returns -1 on any error executing the command.
> + * Returns -1 on any error executing the command, -2 if the error happen
> + * before running the command.
> + *
> * Will not return on success.
> */
> #ifndef WIN32
> int virCommandExec(virCommandPtr cmd)
> {
> + gid_t *groups = NULL;
> + int ngroups;
> +
> if (!cmd ||cmd->has_error == ENOMEM) {
> virReportOOMError();
> - return -1;
> + return -2;
> }
> if (cmd->has_error) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("invalid use of command API"));
> - return -1;
> + return -2;
> }
>
> - execve(cmd->args[0], cmd->args, cmd->env);
> + if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0) {
> + VIR_FREE(groups);
> + return -2;
> + }
> +
> + if (cmd->uid != (uid_t)-1 || cmd->gid != (gid_t)-1 ||
> + cmd->capabilities || (cmd->flags & VIR_EXEC_CLEAR_CAPS)) {
> + VIR_DEBUG("Setting child uid:gid to %d:%d with caps %llx",
> + (int)cmd->uid, (int)cmd->gid, cmd->capabilities);
> + if (virSetUIDGIDWithCaps(cmd->uid, cmd->gid, groups, ngroups,
> + cmd->capabilities,
> + !!(cmd->flags & VIR_EXEC_CLEAR_CAPS)) < 0) {
> + return -2;
> + }
> + }
> + VIR_FREE(groups);
> +
> + if (cmd->pwd) {
> + VIR_DEBUG("Running child in %s", cmd->pwd);
> + if (chdir(cmd->pwd) < 0) {
> + virReportSystemError(errno,
> + _("Unable to change to %s"), cmd->pwd);
> + return -2;
> + }
> + }
> +
> + if (cmd->env)
> + execve(cmd->args[0], cmd->args, cmd->env);
> + else
> + execv(cmd->args[0], cmd->args);
>
> virReportSystemError(errno,
> _("cannot execute binary %s"),
> --
> 2.12.2
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
