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
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