Simplify ambient capability dropping in iproute2:ip tool.

Simplify ambient capability dropping in iproute2:ip tool.

[Tinkerer One]

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
 }

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

[Luca Boccassi]

[-- 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 --]

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

[Tinkerer One]

On Fri, Apr 29, 2022 at 2:56 AM Luca Boccassi <bluca@debian.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

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

[Tinkerer One]

Any further thoughts?

Thanks

On Fri, Apr 29, 2022 at 9:26 PM Tinkerer One <tinkerer@zappem.net> wrote:
>
> On Fri, Apr 29, 2022 at 2:56 AM Luca Boccassi <bluca@debian.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