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 <[email protected]>
> ---
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
