On Tue, May 30, 2017 at 7:59 PM, Kees Cook [off-list ref] wrote:
[...]

quoted

quoted

I see a few options:

1) keep what you have for v4, and hope other places don't use
__request_module. (I'm not a fan of this.)

Yes even if it is documented I wouldn't bet on it, though. :-)

Okay, we seem to agree: we'll not use #1.

quoted

quoted

2) switch the logic on autoload==1 from OR to AND: both the specified
caps _and_ CAP_SYS_MODULE are required. (This seems like it might make
autoload==1 less useful.)

That will restrict some userspace that works only with CAP_NET_ADMIN.

Nor #2.

quoted

quoted

3) use the request_module_cap() outlined above, which requires that
modules being loaded under a CAP_SYS_MODULE-aliased capability are at
least restricted to a subset of kernel module names.

This one tends to allow usability.

Right, discussed below...

quoted

quoted

4) same as 3 but also insert autoload==2 level that switches from OR
to AND (bumping existing ==2 to ==3).

I wouldn't expose autoload to callers, I think it is better if it
stays a property of the module subsystem. But lets use the bump idea,
please see below.

If we can't agree below, I think #4 would be a good way to allow for
both states.

Ok!

quoted

quoted

What do you think?

Ok so given that we already have modules_autoload_mode=2 disabled,
maybe we go with 3)  like this ?

int __request_module(bool wait, int required_cap, const char *prefix,
const char *name, ...);
#define request_module(mod...) \
        __request_module(true, -1, NULL, mod)
#define request_module_cap(required_cap, prefix, mod...) \
        __request_module(true, required_cap, prefix, mod)

and we require allow_cap and prefix to be set.

request_module_cap(CAP_NET_ADMIN, "netdev-", "%s", name) for
net/core/dev_ioctl.c:dev_load()

request_module_cap(CAP_NET_ADMIN, "tcp_", "%s", name) for
net/ipv4/tcp_cong.c  functions.


Then
__request_module()
  -> security_kernel_module_request(module_name, required_cap, prefix)
     -> may_autoload_module(current, module_name, required_cap, prefix)


And update may_autoload_module() as below ? we hard code CAP_NET_ADMIN
and CAP_SYS_MODULE inside and make them the only capabilities needed
for a privileged auto-load operation.

I still think making a specific exception for CAP_NET_ADMIN is not the
right solution, instead allowing for non-CAP_SYS_MODULE caps when
using a distinct prefix.

Alright! I would have loved to avoid capabilities game, but these
patches also use them... so worst scenario is the per-task can always
be set, "task->module_autoload_mode=2" and block it if necessary.

quoted

request_module_cap(CAP_SYS_MODULE, ...) or
request_module_cap(CAP_NET_ADMIN, ...) if the autoload should be a
privileged operation.

Kees will this work ?

Jessica,  Rusty,  Serge. What do you think ? I definitively think that
module_autoload should be contained only inside the module subsystem..

I'd change it like this:

quoted

+int may_autoload_module(struct task_struct *task, char *kmod_name,
+                       int require_cap, char *prefix)
+{
+       unsigned int autoload;
+       int module_require_cap = 0;

I'd initialize this to module_require_cap = CAP_SYS_MODULE;

Ok, please see below.

quoted

+
+       if (require_cap > 0) {
+               if (prefix == NULL || *prefix == '\0')
+                       return -EPERM;

Since an unprefixed module load should only be CAP_SYS_MODULE, change
the above "if" to:

    if (require_cap > 0 && prefix != NULL && *prefix != '\0')

quoted

+
+               /*
+                * We only allow CAP_SYS_MODULE or CAP_NET_ADMIN for
+                * 'netdev-%s' modules for backward compatibility.
+                * Please do not overload capabilities.
+                */
+               if (require_cap == CAP_SYS_MODULE ||
+                   require_cap == CAP_NET_ADMIN)
+                       module_require_cap = require_cap;
+               else
+                       return -EPERM;
+       }

And then drop all these checks, leaving only:

        module_require_cap = require_cap;

quoted

+
+       /* Get max value of sysctl and task "modules_autoload_mode" */
+       autoload = max_t(unsigned int, modules_autoload_mode,
+                        task->modules_autoload_mode);
+
+       /*
+        * If autoload is disabled then fail here and not bother at all
+        */
+       if (autoload == MODULES_AUTOLOAD_DISABLED)
+               return -EPERM;
+
+       /*
+        * If caller require capabilities then we may not allow
+        * automatic module loading. We should not bypass callers.
+        * This allows to support networking code that uses CAP_NET_ADMIN
+        * for some aliased 'netdev-%s' modules.
+        *
+        * Explicitly bump autoload here if necessary
+        */
+       if (module_require_cap && autoload == MODULES_AUTOLOAD_ALLOWED)
+               autoload = MODULES_AUTOLOAD_PRIVILEGED;

I don't see a reason to bump the autoload level.

quoted

+
+       if (autoload == MODULES_AUTOLOAD_ALLOWED)
+               return 0;

This test can be moved to above the AUTOLOAD_DISABLED test.

quoted

+       else if(autoload == MODULES_AUTOLOAD_PRIVILEGED) {
+               /*
+                * If module auto-load is a privileged operation then check
+                * if capabilities are set.
+                */
+               if (capable(CAP_SYS_MODULE) ||
+                   (module_require_cap && capable(module_require_cap)))
+                       return 0;
+       }

This test could drop the explicit CAP_SYS_MODULE test and just rely on
module_require_cap.

quoted

+
+       return -EPERM;
+}
+

So, I would suggest:

Ok Kees, I will update based on your feedback, except for the
following, please see below and let me know :-)

int may_autoload_module(struct task_struct *task, char *kmod_name,
                       int require_cap, char *prefix)
{
        unsigned int autoload;
        int module_require_cap;

        if (autoload == MODULES_AUTOLOAD_DISABLED)
                return -EPERM;

        /* Get max value of sysctl and task "modules_autoload_mode" */
        autoload = max_t(unsigned int, modules_autoload_mode,
                        task->modules_autoload_mode);

        if (autoload == MODULES_AUTOLOAD_ALLOWED)
                return 0;

I don't think that the MODULES_AUTOLOAD_ALLOWED check here at this
place is the best thing to do.

If we remove the capable(CAP_NET_ADMIN) from net/core/dev_ioctl:dev_load()

http://elixir.free-electrons.com/linux/v4.12-rc3/source/net/core/dev_ioctl.c#L369

Or if future changes (accidental) remove that capable(CAP_NET_ADMIN)
and replace it with:
   request_module_cap(CAP_NET_ADMIN, "netdev-", "%s", name);


Then we will check the requested capability *after* autoload allowed
as it is in this example, it should be checked *before* the autoload
allowed:
        // Check required capability before this
        if (autoload == MODULES_AUTOLOAD_ALLOWED)
                return 0;

This way we are still safe we do not downgrade the required capability
that was requested by the calling subsystem based on
MODULES_AUTOLOAD_ALLOWED. If networking code or any other code thinks
that we need CAP_X to load a module then we should honor it. So we do
not break current usage by introducing the "modules_autoload_mode", it
should be set regardless of the autoload mode. Especially since
modules autoload mode is 0 by default. This avoids breaking current
rule to require CAP_NET_ADMIN for 'netdev?-%'

        /*
         * It should be impossible for autoload to have any other
         * value at this point, so explicitly reject all other states.
         */
        if (autoload != MODULES_AUTOLOAD_PRIVILEGED)
                return -EPERM;

        /* Verify that alternate capabilities requirements had a prefix. */
        if (require_cap > 0 && prefix != NULL && *prefix != '\0')
                module_require_cap = require_cap;
        else
                module_require_cap = CAP_SYS_MODULE;

        return capable(module_require_cap);

So with your code, but I really think that we should treat
MODULES_AUTOLOAD_ALLOWED with special care in regard of the passed
capabilities, so:


        module_require_cap = 0;

        if (autoload == MODULES_AUTOLOAD_DISABLED)
                return -EPERM;

        if (autoload == MODULES_AUTOLOAD_PRIVILEGED || require_cap > 0) {
                if (prefix != NULL && *prefix != '\0')
                        /*
                         * Allow non-CAP_SYS_MODULE caps when
                         * using a distinct prefix.
                         */
                        module_require_cap = require_cap;
                else
                        /*
                         * Otherwise always require CAP_SYS_MODULE if no
                         * valid prefix. Callers that do not provide a
valid prefix
                         * should not provide a require_cap > 0
                         */
                        module_require_cap = CAP_SYS_MODULE;
        }

        /* If autoload allowed and 'module_require_cap' was *never*
set, allow */
        if (module_require_cap == 0 && autoload == MODULES_AUTOLOAD_ALLOWED)
                return 0;

        return capable(module_require_cap) ? 0 : -EPERM;

}

Maybe you will agree :-) ?

BTW Kees, also in next version I won't remove the
capable(CAP_NET_ADMIN) check from [1]
even if there is the new request_module_cap(), I would like it to be
in a different patches, this way we go incremental
and maybe it is better to merge what we have now ?  and follow up
later, and of course if other maintainers agree too!

I just need a bit of free time to check again everything and will send
a v5 with all requested changes.


Thank you Kees for your time!

[1] http://elixir.free-electrons.com/linux/v4.12-rc3/source/net/core/dev_ioctl.c#L369
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html