[pacman-dev] [PATCHv2] Add optional 'SandboxUser' option to drop privileges before downloading files

[Andrew Gregory]

On 09/05/21 at 02:42pm, Remi Gacogne wrote:
> This second version contains only the portable part of the sandboxing,
> switching to a different user when a new option, 'SandboxUser', is set.
> 
> To sum up the changes from the last version:
> - The non-portable parts (preventing new privileges from being gained, dropping
>   capabilities, syscalls filtering and filesystem protection) are gone for now,
>   I'll propose them via smaller patches if this change is accepted.
> - The value returned by the sub-process is now passed by the exit status only,
>   instead of using a pipe.
> - The 'UseSandbox' option has been removed.
> - The alpm_sandbox.{c,h} files have been renamed to sandbox.{c,h}.
> - A short description of the 'SandboxUser' option has been added to
>   pacman.conf.5.
> 
> Cheers,
> 
> Remi
> 
> Signed-off-by: Remi Gacogne <rgacogne at archlinux.org>
> ---

Put notes here to avoid including them in the commit message.

>  doc/pacman.conf.5.asciidoc |   5 ++
>  lib/libalpm/alpm.h         |   5 ++
>  lib/libalpm/dload.c        | 102 ++++++++++++++++++++++++++++++++++++-
>  lib/libalpm/handle.c       |  13 +++++
>  lib/libalpm/handle.h       |   1 +
>  lib/libalpm/meson.build    |   1 +
>  lib/libalpm/sandbox.c      |  63 +++++++++++++++++++++++
>  lib/libalpm/sandbox.h      |  31 +++++++++++
>  src/pacman/conf.c          |  19 ++++++-
>  src/pacman/conf.h          |   1 +
>  src/pacman/pacman-conf.c   |   3 ++
>  11 files changed, 241 insertions(+), 3 deletions(-)
>  create mode 100644 lib/libalpm/sandbox.c
>  create mode 100644 lib/libalpm/sandbox.h
 
...

> +/* Download the requested files by launching a process inside a sandbox.
> + * Returns -1 if an error happened for a required file
> + * Returns 0 if a payload was actually downloaded
> + * Returns 1 if no files were downloaded and all errors were non-fatal
> + */
> +static int curl_download_internal_sandboxed(alpm_handle_t *handle,
> +		alpm_list_t *payloads /* struct dload_payload */,
> +		const char *localpath)
> +{
> +	int pid, err = 0, ret = -1;
> +	sigset_t oldblock;
> +	struct sigaction sa_ign = { .sa_handler = SIG_IGN }, oldint, oldquit;
> +
> +	sigaction(SIGINT, &sa_ign, &oldint);
> +	sigaction(SIGQUIT, &sa_ign, &oldquit);
> +	sigaddset(&sa_ign.sa_mask, SIGCHLD);
> +	sigprocmask(SIG_BLOCK, &sa_ign.sa_mask, &oldblock);
> +
> +	pid = fork();
> +
> +	/* child */
> +	if(pid == 0) {
> +		/* restore signal handling for the child to inherit */
> +		sigaction(SIGINT, &oldint, NULL);
> +		sigaction(SIGQUIT, &oldquit, NULL);
> +		sigprocmask(SIG_SETMASK, &oldblock, NULL);
> +
> +		/* cwd to the download directory */
> +		ret = chdir(localpath);
> +		if(ret != 0) {
> +			_alpm_log(handle, ALPM_LOG_WARNING, _("could not chdir to download directory %s\n"), localpath);
> +			ret = -1;
> +		} else {
> +			ret = alpm_sandbox_child(handle->sandboxuser);
> +			if (ret != 0) {
> +				_alpm_log(handle, ALPM_LOG_WARNING, _("sandboxing failed!\n"));
> +			}
> +
> +			ret = curl_download_internal(handle, payloads, localpath);
> +		}
> +
> +		/* pass the result back to the parent */
> +		if(ret == 0) {
> +			/* a payload was actually downloaded */
> +			_Exit(0);
> +		}
> +		else if(ret == 1) {
> +			/* no files were downloaded and all errors were non-fatal */
> +			_Exit(1);
> +		}
> +		else {
> +			/* an error happened for a required file */
> +			_Exit(2);
> +		}
> +	}
> +
> +	/* parent */
> +
> +	if(pid != -1)  {
> +		int wret;
> +		while((wret = waitpid(pid, &ret, 0)) == -1 && errno == EINTR);
> +		if(wret > 0) {
> +			if(!WIFEXITED(ret)) {
> +				/* the child did not terminate normally */
> +				ret = -1;
> +			}
> +			else {
> +				ret = WIFEXITED(ret);
> +				if(ret != 0 && ret != 1) {
> +					/* an error happened for a required file, or unexpected exit status */
> +					ret = -1;
> +				}
> +			}
> +		}
> +		else {
> +			/* waitpid failed */
> +			err = errno;
> +		}
> +	} else {
> +		/* fork failed, make sure errno is preserved after cleanup */
> +		err = errno;
> +	}
> +
> +	sigaction(SIGINT, &oldint, NULL);
> +	sigaction(SIGQUIT, &oldquit, NULL);
> +	sigprocmask(SIG_SETMASK, &oldblock, NULL);
> +
> +	if(err) {
> +		errno = err;
> +		ret = -1;
> +	}
> +	return ret;
> +}
> +

After thinking about this some more, I think this is far too simple.  Just
running download_internal in an unprivileged fork will break anything that
relies on side effects.  download_internal sets pm_errno, tracks server errors,
and calls a number of front-end callbacks.  Losing server error tracking across
multiple downloads isn't a big deal, but losing pm_errno is significant and we
have no way of knowing what kind of state changes the front-end callbacks might
be making.  I suspect this would massively break GUI front-ends.

apg