Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Florian Westphal]

Eric W. Biederman  wrote:
> If loading the conntrack module changes the semantics of packet
> processing when nothing is configured that is a bug in the conntrack
> module.

Thats the default behaviour since forever.

modprobe nf_conntrack_ipv4 -- module_init registers netfilter hooks
and starts doing connection tracking.

You might say 'its wrong' but thats how its been for over a decade.

If you have a suggestion on how to transition to a 'sane' behaviour,
then I'm all ears.

Note however, that conntrack doesn't need any configuration currently.

Its just there once module is loaded.
We could try hooking into nftables/iptables modules that use conntrack
info to make a decision, and thats what we do now in namespaces other
than init_net.

We still do it be default in iniet_net because someone could be
doing conntrack just for purpose of ctnetlink events (conntrack -E and
friends, or flow accouting and the like).

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Florian Westphal]

Eric W. Biederman  wrote:
> If loading the conntrack module changes the semantics of packet
> processing when nothing is configured that is a bug in the conntrack
> module.

Thats the default behaviour since forever.

modprobe nf_conntrack_ipv4 -- module_init registers netfilter hooks
and starts doing connection tracking.

You might say 'its wrong' but thats how its been for over a decade.

If you have a suggestion on how to transition to a 'sane' behaviour,
then I'm all ears.

Note however, that conntrack doesn't need any configuration currently.

Its just there once module is loaded.
We could try hooking into nftables/iptables modules that use conntrack
info to make a decision, and thats what we do now in namespaces other
than init_net.

We still do it be default in iniet_net because someone could be
doing conntrack just for purpose of ctnetlink events (conntrack -E and
friends, or flow accouting and the like).

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Eric W. Biederman]

"Mahesh Bandewar (महेश बंडेवार)"  writes:

> On Mon, May 15, 2017 at 6:52 AM, David Miller  wrote:
>> From: Greg Kroah-Hartman 
>> Date: Mon, 15 May 2017 08:10:59 +0200
>>
>>> On Sun, May 14, 2017 at 08:57:34AM -0500, Eric W. Biederman wrote:
 Greg Kroah-Hartman  writes:

 diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
 index bcb0f610ee42..6b72528a4636 100644
 --- a/net/core/rtnetlink.c
 +++ b/net/core/rtnetlink.c
 @@ -2595,7 +2595,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct 
 nlmsghdr *nlh,

 if (!ops) {
  #ifdef CONFIG_MODULES
 -   if (kind[0]) {
 +   if (kind[0] && capable(CAP_NET_ADMIN)) {
 __rtnl_unlock();
 request_module("rtnl-link-%s", kind);
 rtnl_lock();
>>>
>>> I don't object to this if the networking developers don't mind the
>>> change in functionality.  They can handle the fallout :)
>>
>> As I've said in another email, I am pretty sure this can break things.
>
> The current behavior is already breaking things. e.g. unprivileged
> process can be root inside it's own user-ns. This will allow it to
> create IPtable rules causing contracking module to be loaded in
> default-ns affecting every flow on the server (not just the namespace
> that user or an unprivileged process is attached to). Cases that I
> mentioned above are just the tip of an iceberg.

If loading the conntrack module changes the semantics of packet
processing when nothing is configured that is a bug in the conntrack
module.

> In a non-namespace world this wouldn't happen as capability checks are
> performed correctly but the moment an unprivileged user can create
> it's own user-ns and becomes root inside, it could make use of these
> things and perform privileged operations in default-ns. So to protect
> "global namespace" from making such things happen, we have to protect
> using global capability check.
>
> Alternatively we can preserve the existing behavior by adding this
> check for non-default-user-ns only. e.g.

I believe last time this was discussed the compromise was that a prefix
would be prepended to request_module calls so that what each call
allows to be loaded would be limited in scope to what is sensible
in that location.

I don't think anyone made any arguments about increasing the
attack surface at that time.  So there may be reason to go back
and reexamine the decision on security grounds, but it needs
to be a clearly made argument.  Explaining to people the pros and cons
of the reason to perform the work.

> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 6e67315ec368..263f0d175091 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2595,7 +2595,9 @@ static int rtnl_newlink(struct sk_buff *skb,
> struct nlmsghdr *nlh,
>
> if (!ops) {
>  #ifdef CONFIG_MODULES
> -   if (kind[0]) {
> +   if (kind[0] &&
> +   ((net->user_ns == _user_ns) ||
> +capable(CAP_SYS_MODULE))) {
> __rtnl_unlock();
> request_module("rtnl-link-%s", kind);
> rtnl_lock();

This patch is definitely wrong.  CAP_NET_ADMIN had always guarded this
request_module call.  CAP_SYS_MODULE means you can request any module
you like dropping does not mean you can't request modules.

Adding a capable(CAP_NET_ADMIN) at this call site would be the least
breaking solution available, as it would only break things for callers
in non-initial network namespaces.  Your change would definitely things
for ordinary network administration tools with capabilities.

> if we have to do this in net-subsystem then it's not just this call
> site and there are lot more. But if this is an acceptable alternative,
> I can think of better implementation for all those sites.

Eric

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Eric W. Biederman]

"Mahesh Bandewar (महेश बंडेवार)"  writes:

> On Mon, May 15, 2017 at 6:52 AM, David Miller  wrote:
>> From: Greg Kroah-Hartman 
>> Date: Mon, 15 May 2017 08:10:59 +0200
>>
>>> On Sun, May 14, 2017 at 08:57:34AM -0500, Eric W. Biederman wrote:
 Greg Kroah-Hartman  writes:

 diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
 index bcb0f610ee42..6b72528a4636 100644
 --- a/net/core/rtnetlink.c
 +++ b/net/core/rtnetlink.c
 @@ -2595,7 +2595,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct 
 nlmsghdr *nlh,

 if (!ops) {
  #ifdef CONFIG_MODULES
 -   if (kind[0]) {
 +   if (kind[0] && capable(CAP_NET_ADMIN)) {
 __rtnl_unlock();
 request_module("rtnl-link-%s", kind);
 rtnl_lock();
>>>
>>> I don't object to this if the networking developers don't mind the
>>> change in functionality.  They can handle the fallout :)
>>
>> As I've said in another email, I am pretty sure this can break things.
>
> The current behavior is already breaking things. e.g. unprivileged
> process can be root inside it's own user-ns. This will allow it to
> create IPtable rules causing contracking module to be loaded in
> default-ns affecting every flow on the server (not just the namespace
> that user or an unprivileged process is attached to). Cases that I
> mentioned above are just the tip of an iceberg.

If loading the conntrack module changes the semantics of packet
processing when nothing is configured that is a bug in the conntrack
module.

> In a non-namespace world this wouldn't happen as capability checks are
> performed correctly but the moment an unprivileged user can create
> it's own user-ns and becomes root inside, it could make use of these
> things and perform privileged operations in default-ns. So to protect
> "global namespace" from making such things happen, we have to protect
> using global capability check.
>
> Alternatively we can preserve the existing behavior by adding this
> check for non-default-user-ns only. e.g.

I believe last time this was discussed the compromise was that a prefix
would be prepended to request_module calls so that what each call
allows to be loaded would be limited in scope to what is sensible
in that location.

I don't think anyone made any arguments about increasing the
attack surface at that time.  So there may be reason to go back
and reexamine the decision on security grounds, but it needs
to be a clearly made argument.  Explaining to people the pros and cons
of the reason to perform the work.

> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 6e67315ec368..263f0d175091 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2595,7 +2595,9 @@ static int rtnl_newlink(struct sk_buff *skb,
> struct nlmsghdr *nlh,
>
> if (!ops) {
>  #ifdef CONFIG_MODULES
> -   if (kind[0]) {
> +   if (kind[0] &&
> +   ((net->user_ns == _user_ns) ||
> +capable(CAP_SYS_MODULE))) {
> __rtnl_unlock();
> request_module("rtnl-link-%s", kind);
> rtnl_lock();

This patch is definitely wrong.  CAP_NET_ADMIN had always guarded this
request_module call.  CAP_SYS_MODULE means you can request any module
you like dropping does not mean you can't request modules.

Adding a capable(CAP_NET_ADMIN) at this call site would be the least
breaking solution available, as it would only break things for callers
in non-initial network namespaces.  Your change would definitely things
for ordinary network administration tools with capabilities.

> if we have to do this in net-subsystem then it's not just this call
> site and there are lot more. But if this is an acceptable alternative,
> I can think of better implementation for all those sites.

Eric

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[David Miller]

From: Mahesh Bandewar (महेश बंडेवार) 
Date: Mon, 15 May 2017 10:59:55 -0700

> The current behavior is already breaking things. e.g. unprivileged
> process can be root inside it's own user-ns. This will allow it to
> create IPtable rules causing contracking module to be loaded in
> default-ns affecting every flow on the server (not just the namespace
> that user or an unprivileged process is attached to). Cases that I
> mentioned above are just the tip of an iceberg.

Yes, that is certainly undesirable.

But is it really a module loading problem?  Perhaps we need to look
more deeply into how conntract behaves by default wrt. namespaces.

If we've given the user the ability to be root in his or her own
namespace, then we should let them do root stuff in there.

The only problem is when "doing root stuff in there" has an
undesirable impact upon the rest of the system.

And that's needs to be looked into on a facility by facility basis,
rather then just sprinkling "no module loading" test here and there,
or even unconditionally.

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[David Miller]

From: Mahesh Bandewar (महेश बंडेवार) 
Date: Mon, 15 May 2017 10:59:55 -0700

> The current behavior is already breaking things. e.g. unprivileged
> process can be root inside it's own user-ns. This will allow it to
> create IPtable rules causing contracking module to be loaded in
> default-ns affecting every flow on the server (not just the namespace
> that user or an unprivileged process is attached to). Cases that I
> mentioned above are just the tip of an iceberg.

Yes, that is certainly undesirable.

But is it really a module loading problem?  Perhaps we need to look
more deeply into how conntract behaves by default wrt. namespaces.

If we've given the user the ability to be root in his or her own
namespace, then we should let them do root stuff in there.

The only problem is when "doing root stuff in there" has an
undesirable impact upon the rest of the system.

And that's needs to be looked into on a facility by facility basis,
rather then just sprinkling "no module loading" test here and there,
or even unconditionally.

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[महेश बंडेवार]

On Mon, May 15, 2017 at 6:52 AM, David Miller  wrote:
> From: Greg Kroah-Hartman 
> Date: Mon, 15 May 2017 08:10:59 +0200
>
>> On Sun, May 14, 2017 at 08:57:34AM -0500, Eric W. Biederman wrote:
>>> Greg Kroah-Hartman  writes:
>>>
>>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>>> index bcb0f610ee42..6b72528a4636 100644
>>> --- a/net/core/rtnetlink.c
>>> +++ b/net/core/rtnetlink.c
>>> @@ -2595,7 +2595,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct 
>>> nlmsghdr *nlh,
>>>
>>> if (!ops) {
>>>  #ifdef CONFIG_MODULES
>>> -   if (kind[0]) {
>>> +   if (kind[0] && capable(CAP_NET_ADMIN)) {
>>> __rtnl_unlock();
>>> request_module("rtnl-link-%s", kind);
>>> rtnl_lock();
>>
>> I don't object to this if the networking developers don't mind the
>> change in functionality.  They can handle the fallout :)
>
> As I've said in another email, I am pretty sure this can break things.

The current behavior is already breaking things. e.g. unprivileged
process can be root inside it's own user-ns. This will allow it to
create IPtable rules causing contracking module to be loaded in
default-ns affecting every flow on the server (not just the namespace
that user or an unprivileged process is attached to). Cases that I
mentioned above are just the tip of an iceberg.

In a non-namespace world this wouldn't happen as capability checks are
performed correctly but the moment an unprivileged user can create
it's own user-ns and becomes root inside, it could make use of these
things and perform privileged operations in default-ns. So to protect
"global namespace" from making such things happen, we have to protect
using global capability check.

Alternatively we can preserve the existing behavior by adding this
check for non-default-user-ns only. e.g.

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 6e67315ec368..263f0d175091 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2595,7 +2595,9 @@ static int rtnl_newlink(struct sk_buff *skb,
struct nlmsghdr *nlh,

if (!ops) {
 #ifdef CONFIG_MODULES
-   if (kind[0]) {
+   if (kind[0] &&
+   ((net->user_ns == _user_ns) ||
+capable(CAP_SYS_MODULE))) {
__rtnl_unlock();
request_module("rtnl-link-%s", kind);
rtnl_lock();

if we have to do this in net-subsystem then it's not just this call
site and there are lot more. But if this is an acceptable alternative,
I can think of better implementation for all those sites.

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[महेश बंडेवार]

On Mon, May 15, 2017 at 6:52 AM, David Miller  wrote:
> From: Greg Kroah-Hartman 
> Date: Mon, 15 May 2017 08:10:59 +0200
>
>> On Sun, May 14, 2017 at 08:57:34AM -0500, Eric W. Biederman wrote:
>>> Greg Kroah-Hartman  writes:
>>>
>>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>>> index bcb0f610ee42..6b72528a4636 100644
>>> --- a/net/core/rtnetlink.c
>>> +++ b/net/core/rtnetlink.c
>>> @@ -2595,7 +2595,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct 
>>> nlmsghdr *nlh,
>>>
>>> if (!ops) {
>>>  #ifdef CONFIG_MODULES
>>> -   if (kind[0]) {
>>> +   if (kind[0] && capable(CAP_NET_ADMIN)) {
>>> __rtnl_unlock();
>>> request_module("rtnl-link-%s", kind);
>>> rtnl_lock();
>>
>> I don't object to this if the networking developers don't mind the
>> change in functionality.  They can handle the fallout :)
>
> As I've said in another email, I am pretty sure this can break things.

The current behavior is already breaking things. e.g. unprivileged
process can be root inside it's own user-ns. This will allow it to
create IPtable rules causing contracking module to be loaded in
default-ns affecting every flow on the server (not just the namespace
that user or an unprivileged process is attached to). Cases that I
mentioned above are just the tip of an iceberg.

In a non-namespace world this wouldn't happen as capability checks are
performed correctly but the moment an unprivileged user can create
it's own user-ns and becomes root inside, it could make use of these
things and perform privileged operations in default-ns. So to protect
"global namespace" from making such things happen, we have to protect
using global capability check.

Alternatively we can preserve the existing behavior by adding this
check for non-default-user-ns only. e.g.

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 6e67315ec368..263f0d175091 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2595,7 +2595,9 @@ static int rtnl_newlink(struct sk_buff *skb,
struct nlmsghdr *nlh,

if (!ops) {
 #ifdef CONFIG_MODULES
-   if (kind[0]) {
+   if (kind[0] &&
+   ((net->user_ns == _user_ns) ||
+capable(CAP_SYS_MODULE))) {
__rtnl_unlock();
request_module("rtnl-link-%s", kind);
rtnl_lock();

if we have to do this in net-subsystem then it's not just this call
site and there are lot more. But if this is an acceptable alternative,
I can think of better implementation for all those sites.

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Kees Cook]

On Mon, May 15, 2017 at 6:12 AM, Eric Dumazet  wrote:
> On Sun, May 14, 2017 at 7:42 PM, Mahesh Bandewar (महेश बंडेवार)
>  wrote:
>> On Sun, May 14, 2017 at 3:45 AM, Greg Kroah-Hartman
>>  wrote:
>>> On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
 From: Mahesh Bandewar 

>> [...]
   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
   bridge110592  0
   stp16384  1 bridge
   llc16384  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 
 ---
  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

-- 
Kees Cook
Pixel Security

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Kees Cook]

On Mon, May 15, 2017 at 6:12 AM, Eric Dumazet  wrote:
> On Sun, May 14, 2017 at 7:42 PM, Mahesh Bandewar (महेश बंडेवार)
>  wrote:
>> On Sun, May 14, 2017 at 3:45 AM, Greg Kroah-Hartman
>>  wrote:
>>> On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
 From: Mahesh Bandewar 

>> [...]
   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
   bridge110592  0
   stp16384  1 bridge
   llc16384  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 
 ---
  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

-- 
Kees Cook
Pixel Security

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[David Miller]

From: Greg Kroah-Hartman 
Date: Mon, 15 May 2017 08:10:59 +0200

> On Sun, May 14, 2017 at 08:57:34AM -0500, Eric W. Biederman wrote:
>> Greg Kroah-Hartman  writes:
>> 
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index bcb0f610ee42..6b72528a4636 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -2595,7 +2595,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct 
>> nlmsghdr *nlh,
>>  
>> if (!ops) {
>>  #ifdef CONFIG_MODULES
>> -   if (kind[0]) {
>> +   if (kind[0] && capable(CAP_NET_ADMIN)) {
>> __rtnl_unlock();
>> request_module("rtnl-link-%s", kind);
>> rtnl_lock();
> 
> I don't object to this if the networking developers don't mind the
> change in functionality.  They can handle the fallout :)

As I've said in another email, I am pretty sure this can break things.

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[David Miller]

From: Greg Kroah-Hartman 
Date: Mon, 15 May 2017 08:10:59 +0200

> On Sun, May 14, 2017 at 08:57:34AM -0500, Eric W. Biederman wrote:
>> Greg Kroah-Hartman  writes:
>> 
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index bcb0f610ee42..6b72528a4636 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -2595,7 +2595,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct 
>> nlmsghdr *nlh,
>>  
>> if (!ops) {
>>  #ifdef CONFIG_MODULES
>> -   if (kind[0]) {
>> +   if (kind[0] && capable(CAP_NET_ADMIN)) {
>> __rtnl_unlock();
>> request_module("rtnl-link-%s", kind);
>> rtnl_lock();
> 
> I don't object to this if the networking developers don't mind the
> change in functionality.  They can handle the fallout :)

As I've said in another email, I am pretty sure this can break things.

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[David Miller]

From: Mahesh Bandewar (महेश बंडेवार) 
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.

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[David Miller]

From: Mahesh Bandewar (महेश बंडेवार) 
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.

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Eric Dumazet]

On Sun, May 14, 2017 at 7:42 PM, Mahesh Bandewar (महेश बंडेवार)
 wrote:
> On Sun, May 14, 2017 at 3:45 AM, Greg Kroah-Hartman
>  wrote:
>> On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
>>> From: Mahesh Bandewar 
>>>
> [...]
>>>   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
>>>   bridge110592  0
>>>   stp16384  1 bridge
>>>   llc16384  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 
>>> ---
>>>  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.

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Eric Dumazet]

On Sun, May 14, 2017 at 7:42 PM, Mahesh Bandewar (महेश बंडेवार)
 wrote:
> On Sun, May 14, 2017 at 3:45 AM, Greg Kroah-Hartman
>  wrote:
>> On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
>>> From: Mahesh Bandewar 
>>>
> [...]
>>>   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
>>>   bridge110592  0
>>>   stp16384  1 bridge
>>>   llc16384  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 
>>> ---
>>>  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.

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Greg Kroah-Hartman]

On Sun, May 14, 2017 at 08:57:34AM -0500, Eric W. Biederman wrote:
> Greg Kroah-Hartman  writes:
> 
> > On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
> >> From: Mahesh Bandewar 
> >> 
> >> 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
> >>   bridge110592  0
> >>   stp16384  1 bridge
> >>   llc16384  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 
> >> ---
> >>  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?
> 
> For the specific example give I think we would be better served by
> adding a capability check at the call site.  In this case CAP_NET_ADMIN
> as those are the capabilities iproute traditionally has.
> 
> We have something similar in dev_load in already in the networking code.
> 
> This limits the people who can't load modules to root user in user
> namespaces.  I would be fine with any other code paths in a user
> namespace getting a similar treatment.
> 
> Eric
> 
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index bcb0f610ee42..6b72528a4636 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2595,7 +2595,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct 
> nlmsghdr *nlh,
>  
> if (!ops) {
>  #ifdef CONFIG_MODULES
> -   if (kind[0]) {
> +   if (kind[0] && capable(CAP_NET_ADMIN)) {
> __rtnl_unlock();
> request_module("rtnl-link-%s", kind);
> rtnl_lock();

I don't object to this if the networking developers don't mind the
change in functionality.  They can handle the fallout :)

thanks,

greg k-h

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Greg Kroah-Hartman]

On Sun, May 14, 2017 at 08:57:34AM -0500, Eric W. Biederman wrote:
> Greg Kroah-Hartman  writes:
> 
> > On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
> >> From: Mahesh Bandewar 
> >> 
> >> 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
> >>   bridge110592  0
> >>   stp16384  1 bridge
> >>   llc16384  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 
> >> ---
> >>  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?
> 
> For the specific example give I think we would be better served by
> adding a capability check at the call site.  In this case CAP_NET_ADMIN
> as those are the capabilities iproute traditionally has.
> 
> We have something similar in dev_load in already in the networking code.
> 
> This limits the people who can't load modules to root user in user
> namespaces.  I would be fine with any other code paths in a user
> namespace getting a similar treatment.
> 
> Eric
> 
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index bcb0f610ee42..6b72528a4636 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2595,7 +2595,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct 
> nlmsghdr *nlh,
>  
> if (!ops) {
>  #ifdef CONFIG_MODULES
> -   if (kind[0]) {
> +   if (kind[0] && capable(CAP_NET_ADMIN)) {
> __rtnl_unlock();
> request_module("rtnl-link-%s", kind);
> rtnl_lock();

I don't object to this if the networking developers don't mind the
change in functionality.  They can handle the fallout :)

thanks,

greg k-h

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Greg Kroah-Hartman]

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
>  wrote:
> > On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
> >> From: Mahesh Bandewar 
> >>
> [...]
> >>   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
> >>   bridge110592  0
> >>   stp16384  1 bridge
> >>   llc16384  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 
> >> ---
> >>  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

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Greg Kroah-Hartman]

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
>  wrote:
> > On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
> >> From: Mahesh Bandewar 
> >>
> [...]
> >>   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
> >>   bridge110592  0
> >>   stp16384  1 bridge
> >>   llc16384  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 
> >> ---
> >>  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

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[महेश बंडेवार]

On Sun, May 14, 2017 at 3:45 AM, Greg Kroah-Hartman
 wrote:
> On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
>> From: Mahesh Bandewar 
>>
[...]
>>   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
>>   bridge110592  0
>>   stp16384  1 bridge
>>   llc16384  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 
>> ---
>>  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

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[महेश बंडेवार]

On Sun, May 14, 2017 at 3:45 AM, Greg Kroah-Hartman
 wrote:
> On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
>> From: Mahesh Bandewar 
>>
[...]
>>   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
>>   bridge110592  0
>>   stp16384  1 bridge
>>   llc16384  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 
>> ---
>>  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

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Eric W. Biederman]

Greg Kroah-Hartman  writes:

> On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
>> From: Mahesh Bandewar 
>> 
>> 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
>>   bridge110592  0
>>   stp16384  1 bridge
>>   llc16384  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 
>> ---
>>  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?

For the specific example give I think we would be better served by
adding a capability check at the call site.  In this case CAP_NET_ADMIN
as those are the capabilities iproute traditionally has.

We have something similar in dev_load in already in the networking code.

This limits the people who can't load modules to root user in user
namespaces.  I would be fine with any other code paths in a user
namespace getting a similar treatment.

Eric


diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index bcb0f610ee42..6b72528a4636 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2595,7 +2595,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct 
nlmsghdr *nlh,
 
if (!ops) {
 #ifdef CONFIG_MODULES
-   if (kind[0]) {
+   if (kind[0] && capable(CAP_NET_ADMIN)) {
__rtnl_unlock();
request_module("rtnl-link-%s", kind);
rtnl_lock();

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Eric W. Biederman]

Greg Kroah-Hartman  writes:

> On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
>> From: Mahesh Bandewar 
>> 
>> 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
>>   bridge110592  0
>>   stp16384  1 bridge
>>   llc16384  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 
>> ---
>>  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?

For the specific example give I think we would be better served by
adding a capability check at the call site.  In this case CAP_NET_ADMIN
as those are the capabilities iproute traditionally has.

We have something similar in dev_load in already in the networking code.

This limits the people who can't load modules to root user in user
namespaces.  I would be fine with any other code paths in a user
namespace getting a similar treatment.

Eric


diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index bcb0f610ee42..6b72528a4636 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2595,7 +2595,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct 
nlmsghdr *nlh,
 
if (!ops) {
 #ifdef CONFIG_MODULES
-   if (kind[0]) {
+   if (kind[0] && capable(CAP_NET_ADMIN)) {
__rtnl_unlock();
request_module("rtnl-link-%s", kind);
rtnl_lock();

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Greg Kroah-Hartman]

On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
> From: Mahesh Bandewar 
> 
> 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
>   bridge110592  0
>   stp16384  1 bridge
>   llc16384  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 
> ---
>  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

Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Greg Kroah-Hartman]

On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
> From: Mahesh Bandewar 
> 
> 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
>   bridge110592  0
>   stp16384  1 bridge
>   llc16384  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 
> ---
>  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