From owner-dev-commits-src-main@freebsd.org Sat Jan 16 08:12:43 2021 Return-Path: Delivered-To: dev-commits-src-main@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 2D0F44D7F72; Sat, 16 Jan 2021 08:12:43 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com [IPv6:2a00:1450:4864:20::42f]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DHrQ30dj5z3H59; Sat, 16 Jan 2021 08:12:42 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wr1-x42f.google.com with SMTP id c5so11480505wrp.6; Sat, 16 Jan 2021 00:12:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=iTtT+ROStJcco86TGrTO241PzQPeruEKcjFX28QKpuk=; b=NDfhPEa4ffdrjADNWtfjPrffqO4/NeSqWtyDW050QH0KCUO2wsTVvTyOPxrZ/EW3OP vpAw43+/BS6NzMiQKQ9WsDiXtSb49IVsbFQtfQRjR1Pe+vCxTzQHCxrOQDuupqghbSNA Yp8ZchENbYF6T/IcM4h2Az//KwOcvJXAqCf25HjWgzKvh+gks3rLGHKx+CAN+qA0QDBh ypZiJoHU3i2JIQAp0E5bXp2B1eB0HCAIFEhrVMQLOzF33omJFCL4+BYv5gP0WYJz77ue 9BxG9gb/GIWdfiratcN0POD4kL+TYU6t6VfVVmCzpIrD0rxbbnBUESUUY9P+crUkA6Vf zP7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=iTtT+ROStJcco86TGrTO241PzQPeruEKcjFX28QKpuk=; b=AGLgFYYL7ojsekmqJfYwYmNJBENH0Q3Pr/upFFSVJAi0NzIWkRgKugOtKMUo5lX2Pi Hu7r7LQJEE1H2u738SKyG6ugdEMSxQd+MuUcvKAf1kyMmUHmsjSd2X55oHyhRJ9B/JY2 Jwec0WbLCIwN8mr+EsizYymqVUaL7JJjh2uSkpmdDVu5sMSIlDcpqS2sCR8TWcTijHNN LezwqpbbgQywR5DNmJZLZg2NY1uU5FqVW4OJMdkmUT3xTJcry2VpAb8b7qaNc/ydjff7 XXa1fJ632BRZcSIWiXNebip6rJust2sDPMKWKB6LYnzuQqTiSLwsTxoKmnqVGb3m/qYh KAXw== X-Gm-Message-State: AOAM531Q7nQW8TrnKnQUyxwunzCTzO/qMov+GWOya1YmmvijqnUJZmEW q2s/JgY80uy6Pps7QPku4e6/Syo3PyVj9I6bfPqRQ7jG X-Google-Smtp-Source: ABdhPJwBZTq3uFvo1JMDmcmRbA226cCLvXI+97GP/bbWgTVl8hwh/nIhequAhz9sY3Ut7ttXsS2/BYQj5/4OjpZ87FM= X-Received: by 2002:a5d:4e92:: with SMTP id e18mr17498028wru.66.1610784760615; Sat, 16 Jan 2021 00:12:40 -0800 (PST) MIME-Version: 1.0 Received: by 2002:adf:f811:0:0:0:0:0 with HTTP; Sat, 16 Jan 2021 00:12:39 -0800 (PST) In-Reply-To: <202101152023.10FKNgbO080104@gitrepo.freebsd.org> References: <202101152023.10FKNgbO080104@gitrepo.freebsd.org> From: Mateusz Guzik Date: Sat, 16 Jan 2021 09:12:39 +0100 Message-ID: Subject: Re: git: aefe30c54371 - main - cat: capsicumize it To: Mariusz Zaborski Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org, Mark Johnston , Alex Richardson Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4DHrQ30dj5z3H59 X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 16 Jan 2021 08:12:43 -0000 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 wrote: > The branch main has been updated by oshogbo: > > URL: > https://cgit.FreeBSD.org/src/commit/?id=aefe30c5437159a5399bdbc1974d6fbf40f2ba0f > > commit aefe30c5437159a5399bdbc1974d6fbf40f2ba0f > Author: Mariusz Zaborski > AuthorDate: 2021-01-15 20:22:29 +0000 > Commit: Mariusz Zaborski > 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 > 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 > __FBSDID("$FreeBSD$"); > > +#include > #include > #include > #ifndef NO_UDOM_SUPPORT > @@ -56,6 +57,7 @@ __FBSDID("$FreeBSD$"); > #include > #endif > > +#include > #include > #include > #include > @@ -68,9 +70,14 @@ __FBSDID("$FreeBSD$"); > #include > #include > > +#include > +#include > +#include > + > 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