Messages in this thread | | | From | Mahesh Bandewar (महेश बंडेवार) <> | Date | Mon, 11 Nov 2019 17:29:52 -0800 | 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@norrbonn.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@norrbonn.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@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 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
| |