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@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