Message-ID: <CAF2d9jiLcLHUrNveQRFyv_SpV+LVW+aUpMAD_MArd8wzeZnUWA@mail.gmail.com>
Date:   Mon, 11 Nov 2019 17:29:52 -0800
From:   Mahesh Bandewar (महेश बंडेवार) 
        <maheshb@...gle.com>
To:     Jonas Bonn <jonas@...rbonn.se>
Cc:     nicolas.dichtel@...nd.com, linux-netdev <netdev@...r.kernel.org>,
        linux-kernel@...r.kernel.org, David Miller <davem@...emloft.net>
Subject: Re: [PATCH v3 1/6] rtnetlink: allow RTM_SETLINK to reference other namespaces

On Sat, Nov 9, 2019 at 6:17 AM Jonas Bonn <jonas@...rbonn.se> wrote:
>
> Hi Mahesh,
>
> Thanks for the detailed response.  It provided valuable insight.
>
> On 08/11/2019 19:55, Mahesh Bandewar (महेश बंडेवार) wrote:
> > Hi Jonas, thanks for the response.
> >
> > On Fri, Nov 8, 2019 at 12:20 AM Jonas Bonn <jonas@...rbonn.se> wrote:
> >>
> >> Hi Mahesh,
> >>
> >> On 07/11/2019 21:36, Mahesh Bandewar (महेश बंडेवार) wrote:
> >>> On Thu, Nov 7, 2019 at 5:30 AM Jonas Bonn <jonas@...rbonn.se> wrote:
> >>>>
> >>>>
> >>>> +       /* A hack to preserve kernel<->userspace interface.
> >>>> +        * It was previously allowed to pass the IFLA_TARGET_NETNSID
> >>>> +        * attribute as a way to _set_ the network namespace.  In this
> >>>> +        * case, the device interface was assumed to be in the  _current_
> >>>> +        * namespace.
> >>>> +        * If the device cannot be found in the target namespace then we
> >>>> +        * assume that the request is to set the device in the current
> >>>> +        * namespace and thus we attempt to find the device there.
> >>>> +        */
> >>> Could this bypasses the ns_capable() check? i.e. if the target is
> >>> "foo" but your current ns is bar. The process may be "capable" is foo
> >>> but the interface is not found in foo but present in bar and ends up
> >>> modifying it (especially when you are not capable in bar)?
> >>
> >> I don't think so.  There was never any capable-check for the "current"
> >> namespace so there's no change in that regard.
>
> I was wrong on this point.  There IS a capable-check for the "current"
> net.  The code to create interfaces in 'other' namespaces was already in
> place before my patch and that code does the right thing with respect to
> checking NS capabilities on the "destination" and "link" nets.
>
> My patch is mostly just accounting for the "setlink" aspect of NEWLINK
> where the device already exists in a foreign namespace and needs to be
> searched for there.  Even in that code path, all the ns-capable checks
> are in place and the behaviour is the same as before.
>
> >>
> > not having capable-check seems wrong as we don't want random
> > not-capable processes to alter settings. However, it may be at the API
> > entry level, which will provide necessary protection (haven't
> > checked!). Having said that, this could be bad for the stuff that you
> > are implementing since I could be in "foo" and attempting to change
> > "bar". For this I must be capable in "bar" but the top-level capable
> > check will by default check me in "foo" as well which is not required
> > and could potentially block me from performing legal operation in
> > "bar".
> >
> > Not saying this is a problem, but without having an implementation to
> > use this would be hard to try. You would most likely have a way to
> > verify this, so please check it.
>
> The above shouldn't be an issue with the current implementation.
>
> >
> >> I do think there is an issue with this hack that I can't see any
> >> workaround for.  If the user specifies an interface (by name or index)
> >> for another namespace that doesn't exist, there's a potential problem if
> >> that name/index happens to exist in the "current" namespace.  In that
> >> case, one many end up inadvertently modifying the interface in the
> >> current namespace.  I don't see how to avoid that while maintaining the
> >> backwards compatibility.
> >>
> > This could very well be the case always for single digit ifindex
> > values. (We recently suffered a local scare because of something very
> > similar).
> >
> >> My absolute preference would be to drop this compat-hack altogether.
> >> iproute2 doesn't use a bare TARGET_NETNSID in this manner (for changing
> >> namespaces) and I didn't find any other users by a quick search of other
> >> prominent Netlink users:  systemd, network-manager, connman.  This
> >> compat-hack is there for the _potential ab-user_ of the interface, not
> >> for any known such.
> >>
> > what is forcing you keeping you keeping / implementing this hack? I
> > would also prefer simple solution without creating a potential problem
> > / vulnerability (problem: potentially modifying unintended interface,
> > vulnerability: potentially allow changing without proper credentials;
> > both not proven but are possibilities) down the line. One possibility
> > is to drop the compatibility hack and keep it as a backup if something
> > breaks / someone complains.
>
> OK, this would be my preference, too.  If we can work on the assumption
> that this isn't actually providing compatibility for anybody in
> practice, then we can drop it.  With that, the potential problem of
> inadvertently modifying the wrong device disappears.  There's no problem
> of being able to access a namespace that one isn't capable in, but
> leaving a hole through which the user may end up doing something
> unexpected is pretty ugly.
>
> I'll remove this and repost the series.
>
sgtm

thanks,
--mahesh..

> Thanks for your insight into this issue.  It was helpful.
>
> /Jonas

Powered by blists - more mailing lists