git: aefe30c54371 - main - cat: capsicumize it
Mateusz Guzik
mjguzik at gmail.com
Sat Jan 16 08:12:43 UTC 2021
I have to strongly disagree with this change.
truss -f cat /etc/motd immediately reveals most peculiar overhead
which comes with it.
Some examples:
- pdfork is called 3 times and fork 1 time, spawning 4 processes in total
- the file is opened twice:
5548: openat(AT_FDCWD,"/etc/motd",O_RDONLY,00) = 5 (0x5)
5548: cap_rights_limit(5,{ CAP_READ,CAP_FCNTL,CAP_FSTAT }) = 0 (0x0)
5548: openat(AT_FDCWD,"/etc/motd",O_RDONLY,00) = 7 (0x7)
5548: cap_rights_limit(7,{ CAP_READ,CAP_FCNTL,CAP_FSTAT }) = 0 (0x0)
- there is an enormous number of sendto/recvfrom instead of everything
happening in just one go
Key points:
- the functionality provided by casper definitely induces way more
overhead than it should.
- regardless of the above, I find patching tools like tail and cat in
this manner to be highly questionable. Ultimately whatever security
may or may not have been gained it always have to be gauged against
actual impact and it does not look it is worth it in this case.
Even if someone was to put cat in capability mode, for something as
trivial a opening one file, cat could just do it without all the other
overhead and then enter the sandbox.
That said, I think this change (and possibly similar changes to other
tooling) should be reverted. Regardless of what happens here, casper
needs a lot of work before it is deemed usable.
My $0,03.
On 1/15/21, Mariusz Zaborski <oshogbo at freebsd.org> wrote:
> The branch main has been updated by oshogbo:
>
> URL:
> https://cgit.FreeBSD.org/src/commit/?id=aefe30c5437159a5399bdbc1974d6fbf40f2ba0f
>
> commit aefe30c5437159a5399bdbc1974d6fbf40f2ba0f
> Author: Mariusz Zaborski <oshogbo at FreeBSD.org>
> AuthorDate: 2021-01-15 20:22:29 +0000
> Commit: Mariusz Zaborski <oshogbo at FreeBSD.org>
> CommitDate: 2021-01-15 20:23:42 +0000
>
> cat: capsicumize it
>
> Reviewed by: markj, arichardson
> Differential Revision: https://reviews.freebsd.org/D28083
> ---
> bin/cat/Makefile | 7 +++++
> bin/cat/cat.c | 89
> +++++++++++++++++++++++++++++++++++++++++++++++++---
> tools/build/Makefile | 1 +
> 3 files changed, 93 insertions(+), 4 deletions(-)
>
> diff --git a/bin/cat/Makefile b/bin/cat/Makefile
> index cba55d2870bb..abfdcbcfbb2e 100644
> --- a/bin/cat/Makefile
> +++ b/bin/cat/Makefile
> @@ -15,4 +15,11 @@ CFLAGS+=-DBOOTSTRAP_CAT
> HAS_TESTS=
> SUBDIR.${MK_TESTS}+= tests
>
> +.if ${MK_CASPER} != "no" && !defined(RESCUE) && !defined(BOOTSTRAPPING)
> +LIBADD+= casper
> +LIBADD+= cap_fileargs
> +LIBADD+= cap_net
> +CFLAGS+=-DWITH_CASPER
> +.endif
> +
> .include <bsd.prog.mk>
> diff --git a/bin/cat/cat.c b/bin/cat/cat.c
> index 40d91b243bd4..58f7c15cc9cb 100644
> --- a/bin/cat/cat.c
> +++ b/bin/cat/cat.c
> @@ -48,6 +48,7 @@ static char sccsid[] = "@(#)cat.c 8.2 (Berkeley)
> 4/27/95";
> #include <sys/cdefs.h>
> __FBSDID("$FreeBSD$");
>
> +#include <sys/capsicum.h>
> #include <sys/param.h>
> #include <sys/stat.h>
> #ifndef NO_UDOM_SUPPORT
> @@ -56,6 +57,7 @@ __FBSDID("$FreeBSD$");
> #include <netdb.h>
> #endif
>
> +#include <capsicum_helpers.h>
> #include <ctype.h>
> #include <err.h>
> #include <errno.h>
> @@ -68,9 +70,14 @@ __FBSDID("$FreeBSD$");
> #include <wchar.h>
> #include <wctype.h>
>
> +#include <libcasper.h>
> +#include <casper/cap_fileargs.h>
> +#include <casper/cap_net.h>
> +
> static int bflag, eflag, lflag, nflag, sflag, tflag, vflag;
> static int rval;
> static const char *filename;
> +static fileargs_t *fa;
>
> static void usage(void) __dead2;
> static void scanfiles(char *argv[], int cooked);
> @@ -80,6 +87,8 @@ static void cook_cat(FILE *);
> static void raw_cat(int);
>
> #ifndef NO_UDOM_SUPPORT
> +static cap_channel_t *capnet;
> +
> static int udom_open(const char *path, int flags);
> #endif
>
> @@ -112,6 +121,53 @@ static int udom_open(const char *path, int flags);
> #define SUPPORTED_FLAGS "belnstuv"
> #endif
>
> +#ifndef NO_UDOM_SUPPORT
> +static void
> +init_casper_net(cap_channel_t *casper)
> +{
> + cap_net_limit_t *limit;
> + int familylimit;
> +
> + capnet = cap_service_open(casper, "system.net");
> + if (capnet == NULL)
> + err(EXIT_FAILURE, "unable to create network service");
> +
> + limit = cap_net_limit_init(capnet, CAPNET_NAME2ADDR |
> + CAPNET_CONNECTDNS);
> + if (limit == NULL)
> + err(EXIT_FAILURE, "unable to create limits");
> +
> + familylimit = AF_LOCAL;
> + cap_net_limit_name2addr_family(limit, &familylimit, 1);
> +
> + if (cap_net_limit(limit) < 0)
> + err(EXIT_FAILURE, "unable to apply limits");
> +}
> +#endif
> +
> +static void
> +init_casper(int argc, char *argv[])
> +{
> + cap_channel_t *casper;
> + cap_rights_t rights;
> +
> + casper = cap_init();
> + if (casper == NULL)
> + err(EXIT_FAILURE, "unable to create Casper");
> +
> + fa = fileargs_cinit(casper, argc, argv, O_RDONLY, 0,
> + cap_rights_init(&rights, CAP_READ | CAP_FSTAT | CAP_FCNTL),
> + FA_OPEN | FA_REALPATH);
> + if (fa == NULL)
> + err(EXIT_FAILURE, "unable to create fileargs");
> +
> +#ifndef NO_UDOM_SUPPORT
> + init_casper_net(casper);
> +#endif
> +
> + cap_close(casper);
> +}
> +
> int
> main(int argc, char *argv[])
> {
> @@ -150,6 +206,7 @@ main(int argc, char *argv[])
> usage();
> }
> argv += optind;
> + argc -= optind;
>
> if (lflag) {
> stdout_lock.l_len = 0;
> @@ -160,6 +217,13 @@ main(int argc, char *argv[])
> err(EXIT_FAILURE, "stdout");
> }
>
> + init_casper(argc, argv);
> +
> + caph_cache_catpages();
> +
> + if (caph_enter_casper() < 0)
> + err(EXIT_FAILURE, "capsicum");
> +
> if (bflag || eflag || nflag || sflag || tflag || vflag)
> scanfiles(argv, 1);
> else
> @@ -196,7 +260,7 @@ scanfiles(char *argv[], int cooked __unused)
> fd = STDIN_FILENO;
> } else {
> filename = path;
> - fd = open(path, O_RDONLY);
> + fd = fileargs_open(fa, path);
> #ifndef NO_UDOM_SUPPORT
> if (fd < 0 && errno == EOPNOTSUPP)
> fd = udom_open(path, O_RDONLY);
> @@ -366,20 +430,25 @@ udom_open(const char *path, int flags)
> char rpath[PATH_MAX];
> int fd = -1;
> int error;
> + cap_rights_t rights;
>
> /*
> * Construct the unix domain socket address and attempt to connect.
> */
> bzero(&hints, sizeof(hints));
> hints.ai_family = AF_LOCAL;
> - if (realpath(path, rpath) == NULL)
> +
> + if (fileargs_realpath(fa, path, rpath) == NULL)
> return (-1);
> - error = getaddrinfo(rpath, NULL, &hints, &res0);
> +
> + error = cap_getaddrinfo(capnet, rpath, NULL, &hints, &res0);
> if (error) {
> warn("%s", gai_strerror(error));
> errno = EINVAL;
> return (-1);
> }
> + cap_rights_init(&rights, CAP_CONNECT, CAP_READ, CAP_WRITE,
> + CAP_SHUTDOWN, CAP_FSTAT, CAP_FCNTL);
> for (res = res0; res != NULL; res = res->ai_next) {
> fd = socket(res->ai_family, res->ai_socktype,
> res->ai_protocol);
> @@ -387,7 +456,11 @@ udom_open(const char *path, int flags)
> freeaddrinfo(res0);
> return (-1);
> }
> - error = connect(fd, res->ai_addr, res->ai_addrlen);
> + if (caph_rights_limit(fd, &rights) < 0) {
> + close(fd);
> + return (-1);
> + }
> + error = cap_connect(capnet, fd, res->ai_addr, res->ai_addrlen);
> if (error == 0)
> break;
> else {
> @@ -403,16 +476,24 @@ udom_open(const char *path, int flags)
> if (fd >= 0) {
> switch(flags & O_ACCMODE) {
> case O_RDONLY:
> + cap_rights_clear(&rights, CAP_WRITE);
> if (shutdown(fd, SHUT_WR) == -1)
> warn(NULL);
> break;
> case O_WRONLY:
> + cap_rights_clear(&rights, CAP_READ);
> if (shutdown(fd, SHUT_RD) == -1)
> warn(NULL);
> break;
> default:
> break;
> }
> +
> + cap_rights_clear(&rights, CAP_CONNECT, CAP_SHUTDOWN);
> + if (caph_rights_limit(fd, &rights) < 0) {
> + close(fd);
> + return (-1);
> + }
> }
> return (fd);
> }
> diff --git a/tools/build/Makefile b/tools/build/Makefile
> index 28257a2ea2e5..c0c1786d4bfa 100644
> --- a/tools/build/Makefile
> +++ b/tools/build/Makefile
> @@ -202,6 +202,7 @@ CLEANFILES+= subr_capability.c
> .endif
>
> CASPERINC+= ${SRCTOP}/lib/libcasper/services/cap_fileargs/cap_fileargs.h
> +CASPERINC+= ${SRCTOP}/lib/libcasper/services/cap_net/cap_net.h
>
> .if empty(SRCS)
> SRCS= dummy.c
>
--
Mateusz Guzik <mjguzik gmail.com>
More information about the dev-commits-src-all
mailing list