netdev - Re: [PATCH] kmod: don't load module unless req process has CAP_SYS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87r2zqj82a.fsf@xmission.com>
Date:   Mon, 15 May 2017 13:20:29 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Mahesh Bandewar (महेश बंडेवार) <maheshb@...gle.com>
Cc:     David Miller <davem@...emloft.net>, gregkh@...uxfoundation.org,
        mahesh@...dewar.net, mingo@...nel.org,
        linux-kernel@...r.kernel.org,
        linux-netdev <netdev@...r.kernel.org>, keescook@...omium.org,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

"Mahesh Bandewar (महेश बंडेवार)" <maheshb@...gle.com> writes:

> On Mon, May 15, 2017 at 6:52 AM, David Miller <davem@...emloft.net> wrote:
>> From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>> 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 <gregkh@...uxfoundation.org> 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 == &init_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