lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 13 May 2022 22:10:04 -0700 From: Tinkerer One <tinkerer@...pem.net> To: Luca Boccassi <bluca@...ian.org> Cc: netdev@...r.kernel.org Subject: Re: Simplify ambient capability dropping in iproute2:ip tool. Any further thoughts? Thanks On Fri, Apr 29, 2022 at 9:26 PM Tinkerer One <tinkerer@...pem.net> wrote: > > On Fri, Apr 29, 2022 at 2:56 AM Luca Boccassi <bluca@...ian.org> wrote: > > > > On Thu, 2022-04-28 at 20:17 -0700, Tinkerer One wrote: > > > Hi, > > > > > > This is expanded from https://github.com/shemminger/iproute2/issues/62 > > > which I'm told is not the way to report issues and offer fixes to > > > iproute2 etc. > > > > > > [I'm not subscribed to the netdev list, so please cc: me if you need more info.] > > > > > > The original change that added the drop_cap() code was: > > > > > > https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=ba2fc55b99f8363c80ce36681bc1ec97690b66f5 > > > > > > In an attempt to address some user feedback, the code was further > > > complicated by: > > > > > > https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=9b13cc98f5952f62b825461727c8170d37a4037d > > > > > > Another user issue was asked about here (a couple days ago): > > > > > > https://stackoverflow.com/questions/72015197/allow-non-root-user-of-container-to-execute-binaries-that-need-capabilities > > > > > > I looked into what was going on and found that lib/utils.c contains > > > some complicated code that seems to be trying to prevent Ambient > > > capabilities from being inherited except in specific cases > > > (ip/ip.c:main() calls drop_cap() except in the ip vrf exec case.). The > > > code clears all capabilities in order to prevent Ambient capabilities > > > from being available. The following change achieves suppression of > > > Ambient capabilities much more precisely. It also permits ip to not > > > need to be setuid-root or executed under sudo since it can now be > > > optionally empowered by file capabilities: > > > > > > diff --git a/lib/utils.c b/lib/utils.c > > > index 53d31006..681e4aee 100644 > > > --- a/lib/utils.c > > > +++ b/lib/utils.c > > > @@ -1555,25 +1555,10 @@ void drop_cap(void) > > > #ifdef HAVE_LIBCAP > > > /* don't harmstring root/sudo */ > > > if (getuid() != 0 && geteuid() != 0) { > > > - cap_t capabilities; > > > - cap_value_t net_admin = CAP_NET_ADMIN; > > > - cap_flag_t inheritable = CAP_INHERITABLE; > > > - cap_flag_value_t is_set; > > > - > > > - capabilities = cap_get_proc(); > > > - if (!capabilities) > > > - exit(EXIT_FAILURE); > > > - if (cap_get_flag(capabilities, net_admin, inheritable, > > > - &is_set) != 0) > > > + /* prevent any ambient capabilities from being inheritable */ > > > + if (cap_reset_ambient() != 0) { > > > exit(EXIT_FAILURE); > > > - /* apps with ambient caps can fork and call ip */ > > > - if (is_set == CAP_CLEAR) { > > > - if (cap_clear(capabilities) != 0) > > > - exit(EXIT_FAILURE); > > > - if (cap_set_proc(capabilities) != 0) > > > - exit(EXIT_FAILURE); > > > } > > > - cap_free(capabilities); > > > } > > > #endif > > > } > > > > The current setup is necessary, as the commit message says: > > > > "Users have reported a regression due to ip now dropping capabilities > > unconditionally. > > zerotier-one VPN and VirtualBox use ambient capabilities in their > > binary and then fork out to ip to set routes and links, and this > > does not work anymore. > > > > As a workaround, do not drop caps if CAP_NET_ADMIN (the most common > > capability used by ip) is set with the INHERITABLE flag. > > Users that want ip vrf exec to work do not need to set INHERITABLE, > > which will then only set when the calling program had privileges to > > give itself the ambient capability." > > That doesn't explain why my simplification isn't an improvement. > > As I see it, there are 4 different ways 'ip' can get invoked that > could potentially relate to capabilities: > > - the uid != 0 && euid !=0 test means the code is perfectly happy to > run via sudo or if the 'ip' program is setuid-root. The setuid-root > way is the workaround used in the stackoverflow post I referenced. In > this case, if you try it with sudo, or via setuid-root you'll find the > 'ip' program runs without any Inheritable process capabilities at all. > I'm guessing this is why the code needs that if () { .. } protection > to not "harmstring root/sudo". > > - the drop_cap() function isn't even called in the case of 'ip vrf > exec' so in that one case ambient capabilities (or any other form of > capability) are not dropped and ambient capabilities can be passed on > to any invoked child. > > - should drop_cap() be called for a non-root user it can inherit > capabilities in one of two ways: via the ambient setup referred to in > that commit message (manually achievable by: > > $ sudo capsh --user=`whoami` --inh=cap_net_admin --addamb=cap_net_admin -- > $ ./ip ... > $ exit # needed to escape capsh's ambient setup after the test > > ), or if the 'ip' program is given a file-inheritable capabilities and > the invoker of 'ip' has the corresponding process-inheritable > capabilities (like this: > > $ sudo setcap cap_net_admin=ie ./ip > $ sudo capsh --inh=cap_net_admin --user=`whoami` -- > $ ./ip ... > $ exit # needed to escape capsh's inheritable setup after the test > > ). > > All three of the above are preserved by my simplification because > cap_reset_ambient() doesn't drop permitted or effective capabilities > from the running program, ip. > > The fourth case, the one the upstream code doesn't support (which was > the case the stackoverflow poster cares about) is if the admin sets > permitted+effective file capabilities on their copy of 'ip' (inside > their docker container, or outside such a container for that matter). > In this case, the 'ip' program when run doesn't have any inheritable > capabilities, so in spite of the fact the 'ip' program starts running > with permitted and effective process capabilities (directly obtained > from those file capabilities) of CAP_NET_ADMIN, this particular code > inside drop_cap() causes the program to drop all capabilities and not > work. > > What the code I am attempting to simplify does is permits ip to be set > via this fourth method with: > > $ sudo setcap cap_net_admin=ep ip > > and it will then, just like the setuid-root case, run with permitted > and effective capabilities - which are the "real capabilities" after > all. > > Dropping ambient capabilities with the cap_reset_ambient() call, > preserves the permitted and effective capabilities, but prevents any > process exec'd by 'ip' itself from passing any non-root real > capabilities on to such a child. I was thinking this was why that > capability dropping code was there at all. However, using this > simplification, "sudo setcap cap_net_admin=ep ip" can be used to make > ip work. > > Is it really the intention that this fourth capability setup is not be > supported? > > Thanks for helping me understand. > > > > > Besides, giving setuid to ip itself would be very dangerous, and should > > definitely not be supported. I am not aware of any distribution that > > does it. If there is any, it should be removed. Even for the vrf exec > > case, on Debian/Ubuntu I've set it up so that the caps are not > > configured by default, but require admin action at install time to > > enable, with a clear warning about the possible risks and tradeoffs. > > > > -- > > Kind regards, > > Luca Boccassi
Powered by blists - more mailing lists