[pacman-dev] [PATCH] Add optional sandboxing when downloading files

[Andrew Gregory]

On 08/30/21 at 11:37am, Remi Gacogne wrote:
> [re-sending because my initial e-mail was rejected by spam filters, sorry!]
> 
> Hi all,
> 
> I would like to get some feedback on a optional sandboxing feature I would like
> to implement in pacman. The gist of it is to use a separate process for some
> sensitive operations, like downloading files, and to restrict the privileges
> of that process to reduce the risk of an exploitable bug turning into a code
> execution with full root privileges. I'm not too worried about the pacman code
> itself, but downloading files over the internet involves parsing HTTP payloads,
> as well as parsing X.509 certificates and doing cryptographic operations when
> HTTPS is used, which is more error-prone.
> For what it's worth, Debian's APT has similar options hidden behind the
> APT::Sandbox::Seccomp and APT::Sandbox::User parameters.
> 
> The attached patch is clearly not in a finished state, but provides more details
> about the kind of changes needed to support this new feature. In a nutshell,
> what it currently does is that if the UseSandbox option is not set nothing
> changes. Otherwise internal and external downloads trigger the spawning of a
> new process, via fork(), whose result is communicated to the main process via a
> pipe, as is already done today for external downloads. Then:
> - If available, PR_SET_NO_NEW_PRIVS is called to ensure that the downloader will
>   not be able to gain new privileges in the future, for example by executing a
>   set-uid program.
> - If pacman has been built with libseccomp support and the kernel supports it, a
>   list of system calls that are clearly of no use to a regular process
>   downloading files is denied via a seccomp filter. That list is currently
>   hard-coded but it would make more sense to make it configurable.
> - If the SandboxUser option is set to a valid user, the downloader process
>   switches to that user via setgid, setgroups and setuid, dropping root
>   privileges. This part requires making the download folder owned by that user.
> - If pacman has been built with libcap support and the downloader process still
>   has privileged capabilities at this point, these are dropped.
> - If pacman has been built with landlock support (requires an updated
>   linux-api-headers, >= 5.13) and the kernel supports it, the whole filesystem
>   is set read-only with the exception of /tmp and DBPath. The list of writable
>   paths is currently hard-coded but it would make sense to make it configurable
>   as well.
> 
> Any feedback will be welcome. Please be aware that I tried to keep the new code
> in line with the coding style and contributing guidelines but I'm not familiar
> with pacman's source code so I'm sure I did not do everything the right way, and
> that this code can be improved. What I would like to know first is whether there
> is any interest in that feature, then to get feedback about the "big picture"
> part of the design. If there is indeed interest and my approach is somewhat
> sane, then I can work on making the code less ugly, write the much needed
> documentation for the new feature and related options, and improve the logging
> of errors.
> 
> Thanks!
> 
> Remi
> 
> Signed-off-by: Remi Gacogne <rgacogne at archlinux.org>
> ---
>  lib/libalpm/alpm.h         |  10 ++
>  lib/libalpm/alpm_sandbox.c | 341 +++++++++++++++++++++++++++++++++++++
>  lib/libalpm/alpm_sandbox.h |  31 ++++
>  lib/libalpm/dload.c        |  83 ++++++++-
>  lib/libalpm/handle.c       |  20 +++
>  lib/libalpm/handle.h       |   2 +
>  lib/libalpm/meson.build    |   1 +
>  meson.build                |  11 +-
>  src/pacman/conf.c          |  23 ++-
>  src/pacman/conf.h          |   2 +
>  src/pacman/pacman-conf.c   |   6 +
>  11 files changed, 526 insertions(+), 4 deletions(-)
>  create mode 100644 lib/libalpm/alpm_sandbox.c
>  create mode 100644 lib/libalpm/alpm_sandbox.h

This is a lot.  Let's focus on the portable user switching first; if that gets
merged we can look at adding the extra Linux-specific stuff.

> +	if(pid != -1)  {
> +		int wret;
> +		while((wret = waitpid(pid, &ret, 0)) == -1 && errno == EINTR);
> +		if(wret > 0) {
> +			while(read(err_fd[0], &ret, sizeof(ret)) == -1 && errno == EINTR);
> +		}
> +	} else {
> +		/* fork failed, make sure errno is preserved after cleanup */
> +		err = errno;
> +	}

This is just returning an int, is there a need to pass it by pipe instead of
just using the _Exit value?

I'm also wondering if we could simplify the configuration.  We could infer that
a sandbox should be used if a sandbox user is set or infer the user from the
cache folder being downloaded to.

apg