Message-ID: <CABCx3R3tCEcvnSC5ZvsYh=R0eZ5JMZJT0u-O4pkDmRzwY6hcJQ@mail.gmail.com>
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