| Message ID | 20170512232259.10820-1-mahesh@bandewar.net |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote: > From: Mahesh Bandewar <maheshb@google.com> > > A process inside random user-ns should not load a module, which is > currently possible. As demonstrated in following scenario - > > Create namespaces; especially a user-ns and become root inside. > $ unshare -rfUp -- unshare -unm -- bash > > Try to load the bridge module. It should fail and this is expected! > # modprobe bridge > WARNING: Error inserting stp (/lib/modules/4.11.0-smp-DEV/kernel/net/802/stp.ko): Operation not permitted > FATAL: Error inserting bridge (/lib/modules/4.11.0-smp-DEV/kernel/net/bridge/bridge.ko): Operation not permitted > > Verify bridge module is not loaded. > # lsmod | grep bridge > # > > Now try to create a bridge inside this newly created net-ns which would > mean bridge module need to be loaded. > # ip link add br0 type bridge > # echo $? > 0 > # lsmod | grep bridge > bridge 110592 0 > stp 16384 1 bridge > llc 16384 2 bridge,stp > # > > After this patch - > # ip link add br0 type bridge > RTNETLINK answers: Operation not supported > # echo $? > 2 > # lsmod | grep bridge > # Well, it only loads this because the kernel asked for it to be loaded, right? > > Signed-off-by: Mahesh Bandewar <maheshb@google.com> > --- > kernel/kmod.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/kernel/kmod.c b/kernel/kmod.c > index 563f97e2be36..ac30157169b7 100644 > --- a/kernel/kmod.c > +++ b/kernel/kmod.c > @@ -133,6 +133,9 @@ int __request_module(bool wait, const char *fmt, ...) > #define MAX_KMOD_CONCURRENT 50 /* Completely arbitrary value - KAO */ > static int kmod_loop_msg; > > + if (!capable(CAP_SYS_MODULE)) > + return -EPERM; At first glance this looks right, but I'm worried what this will break that currently relies on this. There might be lots of systems that are used to this being the method that the needed module is requested. What about when userspace asks for a random char device and that module is then loaded? Does this patch break that functionality? thanks, greg k-h
On Sun, May 14, 2017 at 3:45 AM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote: >> From: Mahesh Bandewar <maheshb@google.com> >> [...] >> Now try to create a bridge inside this newly created net-ns which would >> mean bridge module need to be loaded. >> # ip link add br0 type bridge >> # echo $? >> 0 >> # lsmod | grep bridge >> bridge 110592 0 >> stp 16384 1 bridge >> llc 16384 2 bridge,stp >> # >> >> After this patch - >> # ip link add br0 type bridge >> RTNETLINK answers: Operation not supported >> # echo $? >> 2 >> # lsmod | grep bridge >> # > > Well, it only loads this because the kernel asked for it to be loaded, > right? > Yes, kernel asked for it because of a user action. >> >> Signed-off-by: Mahesh Bandewar <maheshb@google.com> >> --- >> kernel/kmod.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/kernel/kmod.c b/kernel/kmod.c >> index 563f97e2be36..ac30157169b7 100644 >> --- a/kernel/kmod.c >> +++ b/kernel/kmod.c >> @@ -133,6 +133,9 @@ int __request_module(bool wait, const char *fmt, ...) >> #define MAX_KMOD_CONCURRENT 50 /* Completely arbitrary value - KAO */ >> static int kmod_loop_msg; >> >> + if (!capable(CAP_SYS_MODULE)) >> + return -EPERM; > > At first glance this looks right, but I'm worried what this will break > that currently relies on this. There might be lots of systems that are > used to this being the method that the needed module is requested. What > about when userspace asks for a random char device and that module is > then loaded? Does this patch break that functionality? > Any module when loaded gets loaded system-wide as we can't allow module loading per-ns. To validate the behavior I was comparing it with insmod/modprobe, if that doesn't allow because of lack of this capability in default-ns, then this *indirect* method of loading module should not allow the same action and the behavior should be consistent. So with that logic if userspace asks for a random char-device if insmod/modprobe cannot load it, then this method should not load it either for the consistency, right? > thanks, > > greg k-h
On Sun, May 14, 2017 at 07:42:08PM -0700, Mahesh Bandewar (महेश बंडेवार) wrote: > On Sun, May 14, 2017 at 3:45 AM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote: > >> From: Mahesh Bandewar <maheshb@google.com> > >> > [...] > >> Now try to create a bridge inside this newly created net-ns which would > >> mean bridge module need to be loaded. > >> # ip link add br0 type bridge > >> # echo $? > >> 0 > >> # lsmod | grep bridge > >> bridge 110592 0 > >> stp 16384 1 bridge > >> llc 16384 2 bridge,stp > >> # > >> > >> After this patch - > >> # ip link add br0 type bridge > >> RTNETLINK answers: Operation not supported > >> # echo $? > >> 2 > >> # lsmod | grep bridge > >> # > > > > Well, it only loads this because the kernel asked for it to be loaded, > > right? > > > Yes, kernel asked for it because of a user action. Which is good, that's the way it is supposed to work. > >> Signed-off-by: Mahesh Bandewar <maheshb@google.com> > >> --- > >> kernel/kmod.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/kernel/kmod.c b/kernel/kmod.c > >> index 563f97e2be36..ac30157169b7 100644 > >> --- a/kernel/kmod.c > >> +++ b/kernel/kmod.c > >> @@ -133,6 +133,9 @@ int __request_module(bool wait, const char *fmt, ...) > >> #define MAX_KMOD_CONCURRENT 50 /* Completely arbitrary value - KAO */ > >> static int kmod_loop_msg; > >> > >> + if (!capable(CAP_SYS_MODULE)) > >> + return -EPERM; > > > > At first glance this looks right, but I'm worried what this will break > > that currently relies on this. There might be lots of systems that are > > used to this being the method that the needed module is requested. What > > about when userspace asks for a random char device and that module is > > then loaded? Does this patch break that functionality? > > > Any module when loaded gets loaded system-wide as we can't allow > module loading per-ns. That's the joys of "namespaces" :) > To validate the behavior I was comparing it > with insmod/modprobe, if that doesn't allow because of lack of this > capability in default-ns, then this *indirect* method of loading > module should not allow the same action and the behavior should be > consistent. So with that logic if userspace asks for a random > char-device if insmod/modprobe cannot load it, then this method should > not load it either for the consistency, right? No, that would break things that are expecting this type of functionality, right? What is the "problem" with loading kernel modules when userspace asks for the functionality involved in them? There has been some work with the LSM interface to disallow this if so desired, why not just use that instead? thanks, greg k-h
On Sun, May 14, 2017 at 7:42 PM, Mahesh Bandewar (महेश बंडेवार) <maheshb@google.com> wrote: > On Sun, May 14, 2017 at 3:45 AM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: >> On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote: >>> From: Mahesh Bandewar <maheshb@google.com> >>> > [...] >>> Now try to create a bridge inside this newly created net-ns which would >>> mean bridge module need to be loaded. >>> # ip link add br0 type bridge >>> # echo $? >>> 0 >>> # lsmod | grep bridge >>> bridge 110592 0 >>> stp 16384 1 bridge >>> llc 16384 2 bridge,stp >>> # >>> >>> After this patch - >>> # ip link add br0 type bridge >>> RTNETLINK answers: Operation not supported >>> # echo $? >>> 2 >>> # lsmod | grep bridge >>> # >> >> Well, it only loads this because the kernel asked for it to be loaded, >> right? >> > Yes, kernel asked for it because of a user action. > >>> >>> Signed-off-by: Mahesh Bandewar <maheshb@google.com> >>> --- >>> kernel/kmod.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/kernel/kmod.c b/kernel/kmod.c >>> index 563f97e2be36..ac30157169b7 100644 >>> --- a/kernel/kmod.c >>> +++ b/kernel/kmod.c >>> @@ -133,6 +133,9 @@ int __request_module(bool wait, const char *fmt, ...) >>> #define MAX_KMOD_CONCURRENT 50 /* Completely arbitrary value - KAO */ >>> static int kmod_loop_msg; >>> >>> + if (!capable(CAP_SYS_MODULE)) >>> + return -EPERM; >> >> At first glance this looks right, but I'm worried what this will break >> that currently relies on this. There might be lots of systems that are >> used to this being the method that the needed module is requested. What >> about when userspace asks for a random char device and that module is >> then loaded? Does this patch break that functionality? >> > Any module when loaded gets loaded system-wide as we can't allow > module loading per-ns. To validate the behavior I was comparing it > with insmod/modprobe, if that doesn't allow because of lack of this > capability in default-ns, then this *indirect* method of loading > module should not allow the same action and the behavior should be > consistent. So with that logic if userspace asks for a random > char-device if insmod/modprobe cannot load it, then this method should > not load it either for the consistency, right? This patch will break applications that expected modules being auto loaded. Try to use SCTP protocol if module is not loaded. Current kernels : SCTP is (auto) loaded, application can use SCTP just fine. After your patch : socket() will fail, unless application run by a privileged user. Some people will qualify this as a regression.
From: Mahesh Bandewar (महेश बंडेवार) <maheshb@google.com> Date: Sun, 14 May 2017 19:42:08 -0700 > Any module when loaded gets loaded system-wide as we can't allow > module loading per-ns. To validate the behavior I was comparing it > with insmod/modprobe, if that doesn't allow because of lack of this > capability in default-ns, then this *indirect* method of loading > module should not allow the same action and the behavior should be > consistent. So with that logic if userspace asks for a random > char-device if insmod/modprobe cannot load it, then this method should > not load it either for the consistency, right? A lot of us worry that the are decades of precedence for the current behavior. If the user asks for bridge statistics and the bridge module isn't loaded, it does get loaded and they see the statistics. Same goes for opening socket types of various protocols. Things really can break if we stop doing this.
On Mon, May 15, 2017 at 6:12 AM, Eric Dumazet <edumazet@google.com> wrote: > On Sun, May 14, 2017 at 7:42 PM, Mahesh Bandewar (महेश बंडेवार) > <maheshb@google.com> wrote: >> On Sun, May 14, 2017 at 3:45 AM, Greg Kroah-Hartman >> <gregkh@linuxfoundation.org> wrote: >>> On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote: >>>> From: Mahesh Bandewar <maheshb@google.com> >>>> >> [...] >>>> Now try to create a bridge inside this newly created net-ns which would >>>> mean bridge module need to be loaded. >>>> # ip link add br0 type bridge >>>> # echo $? >>>> 0 >>>> # lsmod | grep bridge >>>> bridge 110592 0 >>>> stp 16384 1 bridge >>>> llc 16384 2 bridge,stp >>>> # >>>> >>>> After this patch - >>>> # ip link add br0 type bridge >>>> RTNETLINK answers: Operation not supported >>>> # echo $? >>>> 2 >>>> # lsmod | grep bridge >>>> # >>> >>> Well, it only loads this because the kernel asked for it to be loaded, >>> right? >>> >> Yes, kernel asked for it because of a user action. >> >>>> >>>> Signed-off-by: Mahesh Bandewar <maheshb@google.com> >>>> --- >>>> kernel/kmod.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/kernel/kmod.c b/kernel/kmod.c >>>> index 563f97e2be36..ac30157169b7 100644 >>>> --- a/kernel/kmod.c >>>> +++ b/kernel/kmod.c >>>> @@ -133,6 +133,9 @@ int __request_module(bool wait, const char *fmt, ...) >>>> #define MAX_KMOD_CONCURRENT 50 /* Completely arbitrary value - KAO */ >>>> static int kmod_loop_msg; >>>> >>>> + if (!capable(CAP_SYS_MODULE)) >>>> + return -EPERM; >>> >>> At first glance this looks right, but I'm worried what this will break >>> that currently relies on this. There might be lots of systems that are >>> used to this being the method that the needed module is requested. What >>> about when userspace asks for a random char device and that module is >>> then loaded? Does this patch break that functionality? >>> >> Any module when loaded gets loaded system-wide as we can't allow >> module loading per-ns. To validate the behavior I was comparing it >> with insmod/modprobe, if that doesn't allow because of lack of this >> capability in default-ns, then this *indirect* method of loading >> module should not allow the same action and the behavior should be >> consistent. So with that logic if userspace asks for a random >> char-device if insmod/modprobe cannot load it, then this method should >> not load it either for the consistency, right? > > > This patch will break applications that expected modules being auto loaded. I would prefer that we continue to look at the autoloading restrictions series, since that will be more flexible and cover a wider set of cases: https://lkml.org/lkml/2017/4/19/1086 -Kees
diff --git a/kernel/kmod.c b/kernel/kmod.c index 563f97e2be36..ac30157169b7 100644 --- a/kernel/kmod.c +++ b/kernel/kmod.c @@ -133,6 +133,9 @@ int __request_module(bool wait, const char *fmt, ...) #define MAX_KMOD_CONCURRENT 50 /* Completely arbitrary value - KAO */ static int kmod_loop_msg; + if (!capable(CAP_SYS_MODULE)) + return -EPERM; + /* * We don't allow synchronous module loading from async. Module * init may invoke async_synchronize_full() which will end up