Re: Simplify ambient capability dropping in iproute2:ip tool.

[Luca Boccassi <bluca@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 4016 bytes --]

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."

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]