Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191107132755.8517-1-jonas@norrbonn.se> <20191107132755.8517-2-jonas@norrbonn.se>
 <CAF2d9jjRLZ07Qx0NJ9fi1iUpHn+qYEJ+cacKgBmeZ2FvZLObEQ@mail.gmail.com>
 <fff51fa7-5c42-7fa7-6208-d911b18bd91e@norrbonn.se> <CAF2d9jib=Qdn9uB=kKn4CTbqvqOiGs+FGh4427=o+UySLf=BwA@mail.gmail.com>
 <7a2038c8-d3a6-2144-f11d-965394d1b420@norrbonn.se>
In-Reply-To: <7a2038c8-d3a6-2144-f11d-965394d1b420@norrbonn.se>
From:   =?UTF-8?B?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH4KS14KS+4KSwKQ==?= 
        <maheshb@google.com>
Date:   Mon, 11 Nov 2019 17:29:52 -0800
Message-ID: <CAF2d9jiLcLHUrNveQRFyv_SpV+LVW+aUpMAD_MArd8wzeZnUWA@mail.gmail.com>
Subject: Re: [PATCH v3 1/6] rtnetlink: allow RTM_SETLINK to reference other namespaces
To:     Jonas Bonn <jonas@norrbonn.se>
Cc:     nicolas.dichtel@6wind.com, linux-netdev <netdev@vger.kernel.org>,
        linux-kernel@vger.kernel.org, David Miller <davem@davemloft.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Nov 9, 2019 at 6:17 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>
> Hi Mahesh,
>
> Thanks for the detailed response.  It provided valuable insight.
>
> On 08/11/2019 19:55, Mahesh Bandewar (=E0=A4=AE=E0=A4=B9=E0=A5=87=E0=A4=
=B6 =E0=A4=AC=E0=A4=82=E0=A4=A1=E0=A5=87=E0=A4=B5=E0=A4=BE=E0=A4=B0) wrote:
> > Hi Jonas, thanks for the response.
> >
> > On Fri, Nov 8, 2019 at 12:20 AM Jonas Bonn <jonas@norrbonn.se> wrote:
> >>
> >> Hi Mahesh,
> >>
> >> On 07/11/2019 21:36, Mahesh Bandewar (=E0=A4=AE=E0=A4=B9=E0=A5=87=E0=
=A4=B6 =E0=A4=AC=E0=A4=82=E0=A4=A1=E0=A5=87=E0=A4=B5=E0=A4=BE=E0=A4=B0) wro=
te:
> >>> On Thu, Nov 7, 2019 at 5:30 AM Jonas Bonn <jonas@norrbonn.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 th=
is
> >>>> +        * case, the device interface was assumed to be in the  _cur=
rent_
> >>>> +        * namespace.
> >>>> +        * If the device cannot be found in the target namespace the=
n we
> >>>> +        * assume that the request is to set the device in the curre=
nt
> >>>> +        * 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 th=
e
> >> 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 changin=
g
> >> namespaces) and I didn't find any other users by a quick search of oth=
er
> >> 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