[PATCH v4 next 0/3] modules: automatic module loading restrictions

[PATCH v4 next 0/3] modules: automatic module loading restrictions

[Djalal Harouni]

Hi List,

This is v4 of the automatic module load restriction series.

v1 and v2 implementation were presented as a stackable LSM [1] [2].
v3 was updated to be integrated with the core kernel inside the
capabilities subsystem as suggested by Kees Cook [3].

This v4 is even better, lot of documentation and code comments.

All suggestions were improved and fixed. Please see changelog for more
details.

These patches are against next-20170522

==============

Currently, an explicit call to load or unload kernel modules require
CAP_SYS_MODULE capability. However unprivileged users have always been
able to load some modules using the implicit auto-load operation. An
automatic module loading happens when programs request a kernel feature
from a module that is not loaded. In order to satisfy userspace, the
kernel then automatically load all these required modules.

Historically, the kernel was always able to automatically load modules
if they are not blacklisted. This is one of the most important and
transparent operations of Linux, it allows to provide numerous other
features as they are needed which is crucial for a better user experience.
However, as Linux is popular now and used for different appliances some
of these may need to control such operations. For such systems, recent
needs showed that in some cases allowing to control automatic module
loading is as important as the operation itself. Restricting unprivileged
programs or attackers that abuse this feature to load unused modules or
modules that contain bugs is a significant security measure.

This allows administrators or some special programs to have the
appropriate time to update and deny module autoloading in advance, then
blacklist the corresponding ones. Not doing so may affect the global state
of the machine, especially containers where some apps are moved from one
context to another and not having such mechanisms may allow to expose
and exploit the vulnerable parts to escape the container sandbox.

Embedded or IoT devices also started to ship as containers using generic
distros, some vendors do not have the appropriate time to make their own
OS, hence, using base images is getting popular. These setups may include
unnecessary modules that the final applications will not need. Untrusted
access may abuse the module auto-load feature to expose vulnerabilities.

As every code contains bugs or vulnerabilties, the following
vulnerabilities that affected some features that are often compiled as
modules could have been completely blocked, by restricting autoloading
modules if the system does not need them.

Past months:
* DCCP use after free CVE-2017-6074 [4]
  Unprivileged to local root.

* XFRM framework CVE-2017-7184 [5]
  As advertised it seems it was used to break Ubuntu on a security
  contest.

* n_hldc CVE-2017-2636
* L2TPv3 CVE-2016-10200

This is a short list.


To improve the current status, this series introduces a global
"modules_autoload_mode" sysctl flag, and a per-task one.

The sysctl controls modules auto-load feature and complements
"modules_disabled" which apply to all modules operations. This new flag
allows to control only automatic module loading and if it is allowed or
not, aligning in the process the implicit operation with the explicit one
where both now are covered by capabilities checks.

The three modes that "modules_autoload_mode" sysctl support allow to
provide restrictions on automatic module loading without breaking user
experience.

The sysctl flag is available at "/proc/sys/kernel/modules_autoload_mode"

*) When modules_autoload_mode is set to (0), the default, there are no
restrictions.

*) When modules_autoload_mode is set to (1), processes must have
CAP_SYS_MODULE to be able to trigger a module auto-load operation,
or CAP_NET_ADMIN for modules with a 'netdev-%s' alias.

*) When modules_autoload_mode is set to (2), automatic module loading is
disabled for all. Once set, this value can not be changed.

Notes on relation between "modules_disabled=0" and
"modules_autoload_mode=2":
1) Restricting automatic module loading does not interfere with
explicit module load or unload operations.

2) New features provided by modules can be made available without
rebooting the system.

3) A bad version of a module can be unloaded and replaced with a
better one without rebooting the system.


The original idea of module auto-load restriction comes from
'GRKERNSEC_MODHARDEN' config option.

==========================

The patches also support process trees, containers, and sandboxes by
providing an inherited per-task "modules_autoload_mode" flag that cannot be
re-enabled once disabled. This offers the following advantages:

1) Automatic module loading is still available to the rest of the
system.

2) It is easy to use in containers and sandboxes. DCCP example could
have been used to escape containers. The XFRM framework CVE-2017-7184
needs CAP_NET_ADMIN, but attackers may start to target CAP_NET_ADMIN,
a per-task flag will make it harder.

3) Suitable for desktop and more interactive Linux systems.

4) Will allow in future to implement a per user policy.
The user database format is old and not extensible, as discussed maybe
with a modern format we may achieve the following:

User=djalal
NewKernelFeatures=yes

Which means that that interactive user will be allowed to load extra
Linux features. Others, volatile accounts or guests can be easily
blocked from doing so.

5) CAP_NET_ADMIN is useful, it handles lot of operations, at same time it
started to look more like CAP_SYS_ADMIN which is overloaded. We need
CAP_NET_ADMIN, containers need it, but at same time maybe we do not
want programs running with it to load 'netdev-%s' modules. Having an
extra per-task flag allow to discharge a bit CAP_NET_ADMIN and clearly
target automatic module loading operations.


Usage:
        prctl(PR_SET_MODULES_AUTOLOAD_mode, value, 0, 0, 0).

The per-task "modules_autoload_mode" supports the following values:
0       There are no restrictions, usually the default unless set
        by parent.
1       The task must have CAP_SYS_MODULE to be able to trigger a
        module auto-load operation, or CAP_NET_ADMIN for modules with
        a 'netdev-%s' alias.
2       Automatic modules loading is disabled for the current task.

The mode may only be increased, never decreased, thus ensuring that once
applied, processes can never relax their setting. This make it easy for
developers and users to handle.

Note that even if the per-task "modules_autoload_mode" allows to auto-load
the corresponding modules, automatic module loading may still fail due to
the global sysctl "modules_autoload_mode". For more details please see
Documentation/sysctl/kernel.txt, section "modules_autoload_mode".

When a request to a kernel module is denied, the module name with the
corresponding process name and its pid are logged. Administrators can use
such information to explicitly load the appropriate modules.


# Testing:

##) Global sysctl "modules_autoload_mode"

Before patch:
$ lsmod | grep ipip -
$ sudo ip tunnel add mytun mode ipip remote 10.0.2.100 local 10.0.2.15 ttl 255
$ lsmod | grep ipip -
ipip                   16384  0
tunnel4                16384  1 ipip
ip_tunnel              28672  1 ipip
$ cat /proc/sys/kernel/modules_autoload_mode
0

After patch:
$ lsmod | grep ipip -
# echo 2 > /proc/sys/kernel/modules_autoload_mode
$ sudo ip tunnel add mytun mode ipip remote 10.0.2.100 local 10.0.2.15 ttl 255
add tunnel "tunl0" failed: No such device
$ dmesg
...
[ 1876.378389] module: automatic module loading of netdev-tunl0 by "ip"[1453] was denied
[ 1876.380994] module: automatic module loading of tunl0 by "ip"[1453] was denied
...
$ lsmod | grep ipip -


##) Per-task "modules_autoload_mode" flag

Here we use DCCP as an example since the public PoC was against it.

The following tool can be used to test the feature:
https://gist.githubusercontent.com/tixxdz/f6d77e5a45f9f8cfa4bcc0ab526ce5cf/raw/5f12f98e4dfc8a94f76b13dc290f077a153e74d8/pr_modules_autoload_mode_test.c

DCCP use after free CVE-2017-6074 (unprivileged to local root):
The code path can be triggered by unprivileged, using the trigger.c
program for DCCP use after free [6] and that was fixed by
commit 5edabca9d4cff7f "dccp: fix freeing skb too early for IPV6_RECVPKTINFO".

Before patch:
$ lsmod | grep dccp
$ strace ./dccp_trigger
...
socket(AF_INET6, SOCK_DCCP, IPPROTO_IP) = 3
...
$ lsmod | grep dccp
dccp_ipv6              24576  5
dccp_ipv4              24576  5 dccp_ipv6
dccp                  102400  2 dccp_ipv6,dccp_ipv4
$ grep Modules /proc/self/status
ModulesAutoloadMode:    0


After:
Set task "modules_autoload_mode" to 1, privileged mode.
$ lsmod | grep dccp
$ ./pr_set_no_new_privs
$ grep NoNewPrivs /proc/self/status
NoNewPrivs:     1
$ ./pr_modules_autoload_mode_test 1
$ grep Modules /proc/self/status
ModulesAutoloadMode:    1
$ strace ./dccp_trigger
...
socket(AF_INET6, SOCK_DCCP, IPPROTO_IP) = -1 ESOCKTNOSUPPORT (Socket type not supported)
...
$ lsmod | grep dccp
$ dmesg
...
[ 4662.171994] module: automatic module loading of net-pf-10-proto-0-type-6 by "dccp_trigger"[1759] was denied
[ 4662.177284] module: automatic module loading of net-pf-10-proto-0 by "dccp_trigger"[1759] was denied
[ 4662.180181] module: automatic module loading of net-pf-10-proto-0-type-6 by "dccp_trigger"[1759] was denied
[ 4662.181709] module: automatic module loading of net-pf-10-proto-0 by "dccp_trigger"[1759] was denied



As showed, this blocks automatic module loading per-task. This allows to
provide a usable system, where only some sandboxed apps or containers will be
restricted to trigger automatic module loading, other parts of the
system can continue to use the system as it is which is the case of the
desktop.


Finally we already have a use case for the prctl() interface to enforce
some systemd services [7], and we plan to use it for our containers and
sandboxes. That pull request will be updated if this feature is merged,
We will provide "ProtectKernelModulesMode=strict" as a new directive for
users that can be enforced to make sure that if their services/apps are
compromised they won't be able to abuse the module auto-load operation.


# Changes since v3:
*) Renamed the sysctl from "modules_autoload" to "modules_autoload_mode"
   and the prctl() operation flag to "PR_{SET|GET}_MODULES_AUTOLOAD_MODE"
   as it was requested.

   Suggested-by: Ben Hutchings <ben.hutchings-4yDnlxn2s6sWdaTGBSpHTA@public.gmane.org>


*) Updated __request_module() to take the capability that may allow to
   auto-load a module with the appropriate alias. This way we never
   parse aliases as it was requested by Rusty Russell. Security and
   SELinux hooks were updated too.

   Suggested-by: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>
   https://lkml.org/lkml/2017/4/24/7


*) Updated code to set prctl(PR_SET_MODULES_AUTOLOAD_MODE, 1, 0, 0, 0),
   the task must call prctl(PR_SET_NO_NEW_PRIVS, 1) before or run with
   CAP_SYS_ADMIN privileges in its namespace. If these are not true,
   -EACCES will be returned.

   Suggested-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
   https://lkml.org/lkml/2017/4/22/22


*) Remove task initialization logic and other cleanups
   Suggested-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>


*) Other code and documentation cleanups.
   

# Changes since v2:
*) Implemented as a core kernel feature inside capabilities subsystem
*) Renamed sysctl to "modules_autoload" to align with "modules_disabled"

   Suggested-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

*) Improved documentation.
*) Removed unused code.


# Changes since v1:
*) Renamed module to ModAutoRestrict
*) Improved documentation to explicity refer to module autoloading.
*) Switched to use the new task_security_alloc() hook.
*) Switched from rhash tables to use task->security since it is in
   linux-security/next branch now.
*) Check all parameters passed to prctl() syscall.
*) Many other bug fixes and documentation improvements.


Patches (3) Djalal Harouni:
 (1/3) modules:capabilities: allow __request_module() to take a capability argument
 (2/3) modules:capabilities: automatic module loading restriction
 (3/3) modules:capabilities: add a per-task modules auto-load mode

 Documentation/filesystems/proc.txt                 |   3 +
 Documentation/sysctl/kernel.txt                    |  51 +++++++++
 Documentation/userspace-api/index.rst              |   1 +
 .../userspace-api/modules_autoload_mode.rst        | 115 +++++++++++++++++++++
 fs/proc/array.c                                    |   6 ++
 include/linux/init_task.h                          |   8 ++
 include/linux/kmod.h                               |  15 +--
 include/linux/lsm_hooks.h                          |   4 +-
 include/linux/module.h                             |  41 +++++++-
 include/linux/sched.h                              |   5 +
 include/linux/security.h                           |   7 +-
 include/uapi/linux/prctl.h                         |   8 ++
 kernel/kmod.c                                      |  15 ++-
 kernel/module.c                                    |  93 +++++++++++++++++
 kernel/sysctl.c                                    |  40 +++++++
 net/core/dev_ioctl.c                               |  10 +-
 security/commoncap.c                               |  60 +++++++++++
 security/security.c                                |   4 +-
 security/selinux/hooks.c                           |   2 +-
 19 files changed, 470 insertions(+), 18 deletions(-)

        
References:        
[1] http://www.openwall.com/lists/kernel-hardening/2017/02/02/21
[2] http://www.openwall.com/lists/kernel-hardening/2017/04/09/1
[3] https://lkml.org/lkml/2017/4/19/1086
[4] http://www.openwall.com/lists/oss-security/2017/02/22/3
[5] http://www.openwall.com/lists/oss-security/2017/03/29/2
[6] https://github.com/xairy/kernel-exploits/tree/master/CVE-2017-6074
[7] https://github.com/systemd/systemd/pull/5736

[PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument

[Djalal Harouni]

This is a preparation patch for the module auto-load restriction feature.

In order to restrict module auto-load operations we need to check if the
caller has CAP_SYS_MODULE capability. This allows to align security
checks of automatic module loading with the checks of the explicit operations.

However for "netdev-%s" modules, they are allowed to be loaded if
CAP_NET_ADMIN is set. Therefore, in order to not break this assumption,
and allow userspace to only load "netdev-%s" modules with CAP_NET_ADMIN
capability which is considered a privileged operation, we have two
choices: 1) parse "netdev-%s" alias and check the capability or 2) hand
the capability form request_module() to security_kernel_module_request()
hook and let the capability subsystem decide.

After a discussion with Rusty Russell [1], the suggestion was to pass
the capability from request_module() to security_kernel_module_request()
for 'netdev-%s' modules that need CAP_NET_ADMIN.

The patch does not update request_module(), it updates the internal
__request_module() that will take an extra "allow_cap" argument. If
positive, then automatic module load operation can be allowed.

__request_module() will be only called by networking code which is the
exception to this, so we do not break userspace and CAP_NET_ADMIN can
continue to load 'netdev-%s' modules. Other kernel code should continue
to use request_module() which calls security_kernel_module_request() and
will check for CAP_SYS_MODULE capability in next patch. Allowing more
control on who can trigger automatic module loading.

This patch updates security_kernel_module_request() to take the
'allow_cap' argument and SELinux which is currently the only user of
security_kernel_module_request() hook.

Based on patch by Rusty Russell:
https://lkml.org/lkml/2017/4/26/735

Cc: Serge Hallyn <serge@hallyn.com>
Cc: Andy Lutomirski <luto@kernel.org>
Suggested-by: Rusty Russell <rusty@rustcorp.com.au>
Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>

[1] https://lkml.org/lkml/2017/4/24/7
---
 include/linux/kmod.h      | 15 ++++++++-------
 include/linux/lsm_hooks.h |  4 +++-
 include/linux/security.h  |  4 ++--
 kernel/kmod.c             | 15 +++++++++++++--
 net/core/dev_ioctl.c      | 10 +++++++++-
 security/security.c       |  4 ++--
 security/selinux/hooks.c  |  2 +-
 7 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index c4e441e..a314432 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -32,18 +32,19 @@
 extern char modprobe_path[]; /* for sysctl */
 /* modprobe exit status on success, -ve on error.  Return value
  * usually useless though. */
-extern __printf(2, 3)
-int __request_module(bool wait, const char *name, ...);
-#define request_module(mod...) __request_module(true, mod)
-#define request_module_nowait(mod...) __request_module(false, mod)
+extern __printf(3, 4)
+int __request_module(bool wait, int allow_cap, const char *name, ...);
 #define try_then_request_module(x, mod...) \
-	((x) ?: (__request_module(true, mod), (x)))
+	((x) ?: (__request_module(true, -1, mod), (x)))
 #else
-static inline int request_module(const char *name, ...) { return -ENOSYS; }
-static inline int request_module_nowait(const char *name, ...) { return -ENOSYS; }
+static inline __printf(3, 4)
+int __request_module(bool wait, int allow_cap, const char *name, ...)
+{ return -ENOSYS; }
 #define try_then_request_module(x, mod...) (x)
 #endif
 
+#define request_module(mod...) __request_module(true, -1, mod)
+#define request_module_nowait(mod...) __request_module(false, -1, mod)
 
 struct cred;
 struct file;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index f7914d9..7688f79 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -578,6 +578,8 @@
  *	Ability to trigger the kernel to automatically upcall to userspace for
  *	userspace to load a kernel module with the given name.
  *	@kmod_name name of the module requested by the kernel
+ *	@allow_cap capability that allows to automatically load a kernel
+ *	module.
  *	Return 0 if successful.
  * @kernel_read_file:
  *	Read a file specified by userspace.
@@ -1516,7 +1518,7 @@ union security_list_options {
 	void (*cred_transfer)(struct cred *new, const struct cred *old);
 	int (*kernel_act_as)(struct cred *new, u32 secid);
 	int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
-	int (*kernel_module_request)(char *kmod_name);
+	int (*kernel_module_request)(char *kmod_name, int allow_cap);
 	int (*kernel_read_file)(struct file *file, enum kernel_read_file_id id);
 	int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
 				     enum kernel_read_file_id id);
diff --git a/include/linux/security.h b/include/linux/security.h
index 549cb82..2f4c9d3 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -325,7 +325,7 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
 void security_transfer_creds(struct cred *new, const struct cred *old);
 int security_kernel_act_as(struct cred *new, u32 secid);
 int security_kernel_create_files_as(struct cred *new, struct inode *inode);
-int security_kernel_module_request(char *kmod_name);
+int security_kernel_module_request(char *kmod_name, int allow_cap);
 int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
 int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
 				   enum kernel_read_file_id id);
@@ -926,7 +926,7 @@ static inline int security_kernel_create_files_as(struct cred *cred,
 	return 0;
 }
 
-static inline int security_kernel_module_request(char *kmod_name)
+static inline int security_kernel_module_request(char *kmod_name, int allow_cap)
 {
 	return 0;
 }
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 563f97e..15c96e8 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -110,6 +110,7 @@ static int call_modprobe(char *module_name, int wait)
 /**
  * __request_module - try to load a kernel module
  * @wait: wait (or not) for the operation to complete
+ * @allow_cap: if positive, may allow modprobe if this capability is set.
  * @fmt: printf style format string for the name of the module
  * @...: arguments as specified in the format string
  *
@@ -120,10 +121,20 @@ static int call_modprobe(char *module_name, int wait)
  * must check that the service they requested is now available not blindly
  * invoke it.
  *
+ * If "allow_cap" is positive, The security subsystem will trust the caller
+ * that "allow_cap" may allow to load some modules with a specific alias,
+ * the security subsystem will make some exceptions based on that. This is
+ * primally useful for backward compatibility. A permission check should not
+ * be that strict and userspace should be able to continue to trigger module
+ * auto-loading if needed.
+ *
  * If module auto-loading support is disabled then this function
  * becomes a no-operation.
+ *
+ * This function should not be directly used by other subsystems, for that
+ * please call request_module().
  */
-int __request_module(bool wait, const char *fmt, ...)
+int __request_module(bool wait, int allow_cap, const char *fmt, ...)
 {
 	va_list args;
 	char module_name[MODULE_NAME_LEN];
@@ -150,7 +161,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	if (ret >= MODULE_NAME_LEN)
 		return -ENAMETOOLONG;
 
-	ret = security_kernel_module_request(module_name);
+	ret = security_kernel_module_request(module_name, allow_cap);
 	if (ret)
 		return ret;
 
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index b94b1d2..c494351 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -366,8 +366,16 @@ void dev_load(struct net *net, const char *name)
 	rcu_read_unlock();
 
 	no_module = !dev;
+	/*
+	 * First do the CAP_NET_ADMIN check, then let the security
+	 * subsystem checks know that this can be allowed since this is
+	 * a "netdev-%s" module and CAP_NET_ADMIN is set.
+	 *
+	 * For this exception call __request_module().
+	 */
 	if (no_module && capable(CAP_NET_ADMIN))
-		no_module = request_module("netdev-%s", name);
+		no_module = __request_module(true, CAP_NET_ADMIN,
+					     "netdev-%s", name);
 	if (no_module && capable(CAP_SYS_MODULE))
 		request_module("%s", name);
 }
diff --git a/security/security.c b/security/security.c
index 714433e..cedb790 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1021,9 +1021,9 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode)
 	return call_int_hook(kernel_create_files_as, 0, new, inode);
 }
 
-int security_kernel_module_request(char *kmod_name)
+int security_kernel_module_request(char *kmod_name, int allow_cap)
 {
-	return call_int_hook(kernel_module_request, 0, kmod_name);
+	return call_int_hook(kernel_module_request, 0, kmod_name, allow_cap);
 }
 
 int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 158f6a0..85eeff6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3842,7 +3842,7 @@ static int selinux_kernel_create_files_as(struct cred *new, struct inode *inode)
 	return ret;
 }
 
-static int selinux_kernel_module_request(char *kmod_name)
+static int selinux_kernel_module_request(char *kmod_name, int allow_cap)
 {
 	struct common_audit_data ad;
 
-- 
2.10.2

[PATCH v4 next 2/3] modules:capabilities: automatic module loading restriction

[Djalal Harouni]

Currently, an explicit call to load or unload kernel modules require
CAP_SYS_MODULE capability. However unprivileged users have always been
able to load some modules using the implicit auto-load operation. An
automatic module loading happens when programs request a kernel feature
from a module that is not loaded. In order to satisfy userspace, the
kernel then automatically load all these required modules.

Historically, the kernel was always able to automatically load modules
if they are not blacklisted. This is one of the most important and
transparent operations of Linux, it allows to provide numerous other
features as they are needed which is crucial for a better user experience.
However, as Linux is popular now and used for different appliances some
of these may need to control such operations. For such systems, recent
needs showed that in some cases allowing to control automatic module
loading is as important as the operation itself. Restricting unprivileged
programs or attackers that abuse this feature to load unused modules or
modules that contain bugs is a significant security measure.

This allows administrators or some special programs to have the
appropriate time to update and deny module autoloading in advance, then
blacklist the corresponding ones. Not doing so may affect the global state
of the machine, especially containers where some apps are moved from one
context to another and not having such mechanisms may allow to expose
and exploit the vulnerable parts to escape the container sandbox.

Embedded or IoT devices also started to ship as containers using generic
distros, some vendors do not have the appropriate time to make their own
OS, hence, using base images is getting popular. These setups may include
unnecessary modules that the final applications will not need. Untrusted
access may abuse the module auto-load feature to expose vulnerabilities.

As every code contains bugs or vulnerabilties, the following
vulnerabilities that affected some features that are often compiled as
modules could have been completely blocked, by restricting autoloading
modules if the system does not need them.

Past months:
* DCCP use after free CVE-2017-6074 [1]
  Unprivileged to local root.

* XFRM framework CVE-2017-7184 [2]
  As advertised it seems it was used to break Ubuntu on a security
  contest.

* n_hldc CVE-2017-2636
* L2TPv3 CVE-2016-10200

This is a short list. Fixing this is a high priority.

To improve the current status, this patch introduces "modules_autoload_mode"
kernel sysctl flag. The flag controls modules auto-load feature and
complements "modules_disabled" which apply to all modules operations.
This new flag allows to control only automatic module loading and if it is
allowed or not, aligning in the process the implicit operation with the
explicit one where both now are covered by capabilities checks.

The three modes that "modules_autoload_mode" support allow to provide
restrictions on automatic module loading without breaking user
experience.

The sysctl flag is available at "/proc/sys/kernel/modules_autoload_mode"

When modules_autoload_mode is set to (0), the default, there are no
restrictions.

When modules_autoload_mode is set to (1), processes must have
CAP_SYS_MODULE to be able to trigger a module auto-load operation,
or CAP_NET_ADMIN for modules with a 'netdev-%s' alias.

When modules_autoload_mode is set to (2), automatic module loading is
disabled for all. Once set, this value can not be changed.

Notes on relation between "modules_disabled=0" and
"modules_autoload_mode=2":
1) Restricting automatic module loading does not interfere with
explicit module load or unload operations.

2) New features provided by modules can be made available without
rebooting the system.

3) A bad version of a module can be unloaded and replaced with a
better one without rebooting the system.

The original idea of module auto-load restriction comes from
'GRKERNSEC_MODHARDEN' config option.

Testing
-------

Example 1)

Before:
$ lsmod | grep ipip -
$ sudo ip tunnel add mytun mode ipip remote 10.0.2.100 local 10.0.2.15 ttl 255
$ lsmod | grep ipip -
ipip                   16384  0
tunnel4                16384  1 ipip
ip_tunnel              28672  1 ipip
$ cat /proc/sys/kernel/modules_autoload_mode
0

After:
$ lsmod | grep ipip -
# echo 2 > /proc/sys/kernel/modules_autoload_mode
$ sudo ip tunnel add mytun mode ipip remote 10.0.2.100 local 10.0.2.15 ttl 255
add tunnel "tunl0" failed: No such device
$ dmesg
...
[ 1876.378389] module: automatic module loading of netdev-tunl0 by "ip"[1453] was denied
[ 1876.380994] module: automatic module loading of tunl0 by "ip"[1453] was denied
...
$ lsmod | grep ipip -
$

Example 2)

DCCP use after free CVE-2017-6074:
The code path can be triggered by unprivileged, using the trigger.c
program for DCCP use after free [3] and that was fixed by
commit 5edabca9d4cff7f "dccp: fix freeing skb too early for IPV6_RECVPKTINFO".

Before:
$ lsmod | grep dccp
$ strace ./dccp_trigger
...
socket(AF_INET6, SOCK_DCCP, IPPROTO_IP) = 3
...
$ lsmod | grep dccp
dccp_ipv6              24576  5
dccp_ipv4              24576  5 dccp_ipv6
dccp                  102400  2 dccp_ipv6,dccp_ipv4

After:
Only privileged:
# echo 1 > /proc/sys/kernel/modules_autoload_mode
$ lsmod | grep dccp
$ strace ./dccp_trigger
...
socket(AF_INET6, SOCK_DCCP, IPPROTO_IP) = -1 ESOCKTNOSUPPORT (Socket type not supported)
...
$ lsmod | grep dccp
$ dmesg
...
[  175.945063] module: automatic module loading of net-pf-10-proto-0-type-6 by "dccp_trigger"[1390] was denied
[  175.947952] module: automatic module loading of net-pf-10-proto-0 by "dccp_trigger"[1390] was denied
[  175.956061] module: automatic module loading of net-pf-10-proto-0-type-6 by "dccp_trigger"[1390] was denied
[  175.959733] module: automatic module loading of net-pf-10-proto-0 by "dccp_trigger"[1390] was denied

$ sudo strace ./dccp_trigger
...
socket(AF_INET6, SOCK_DCCP, IPPROTO_IP) = 3
...
$ lsmod | grep dccp
dccp_ipv6              24576  6
dccp_ipv4              24576  5 dccp_ipv6
dccp                  102400  2 dccp_ipv6,dccp_ipv4

Disable automatic module loading:
$ lsmod | grep dccp
$ su - root
# echo 2 > /proc/sys/kernel/modules_autoload_mode
# strace ./dccp_trigger
...
socket(AF_INET6, SOCK_DCCP, IPPROTO_IP) = -1 ESOCKTNOSUPPORT (Socket type not supported)
...
$ lsmod | grep dccp
$ dmesg
...
[  126.596545] module: automatic module loading of net-pf-10-proto-0-type-6 by "dccp_trigger"[1291] was denied
[  126.598800] module: automatic module loading of net-pf-10-proto-0 by "dccp_trigger"[1291] was denied
[  126.601264] module: automatic module loading of net-pf-10-proto-0-type-6 by "dccp_trigger"[1291] was denied
[  126.602839] module: automatic module loading of net-pf-10-proto-0 by "dccp_trigger"[1291] was denied

As an example, this blocks abuses, DCCP still can be explicilty loaded by
an administrator using modprobe, at same time automatic module loading is
disabled forever.

[1] http://www.openwall.com/lists/oss-security/2017/02/22/3
[2] http://www.openwall.com/lists/oss-security/2017/03/29/2
[3] https://github.com/xairy/kernel-exploits/tree/master/CVE-2017-6074

Cc: Ben Hutchings <ben.hutchings@codethink.co.uk>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: James Morris <james.l.morris@oracle.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Andy Lutomirski <luto@kernel.org>
Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
---
 Documentation/sysctl/kernel.txt | 51 +++++++++++++++++++++++++++++++++++++++++
 include/linux/module.h          | 19 ++++++++++++++-
 include/linux/security.h        |  3 ++-
 kernel/module.c                 | 42 +++++++++++++++++++++++++++++++++
 kernel/sysctl.c                 | 40 ++++++++++++++++++++++++++++++++
 security/commoncap.c            | 24 +++++++++++++++++++
 6 files changed, 177 insertions(+), 2 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index bac23c1..3cc6592 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -43,6 +43,7 @@ show up in /proc/sys/kernel:
 - l2cr                        [ PPC only ]
 - modprobe                    ==> Documentation/debugging-modules.txt
 - modules_disabled
+- modules_autoload_mode
 - msg_next_id		      [ sysv ipc ]
 - msgmax
 - msgmnb
@@ -411,6 +412,56 @@ to false.  Generally used with the "kexec_load_disabled" toggle.
 
 ==============================================================
 
+modules_autoload_mode:
+
+A sysctl to control if modules auto-load feature is allowed or not.
+This sysctl complements "modules_disabled" which is for all module
+operations where this flag applies only to automatic module loading.
+Automatic module loading happens when programs request a kernel
+feature that is implemented by an unloaded module, the kernel
+automatically runs the program pointed by "modprobe" sysctl in order
+to load the corresponding module.
+
+Historically, the kernel was always able to automatically load modules
+if they are not blacklisted. This is one of the most important and
+transparent operations of Linux, it allows to provide numerous other
+features as they are needed which is crucial for a better user experience.
+However, as Linux is popular now and used for different appliances some
+of these may need to control such operations. For such systems, recent
+needs showed that in some cases allowing to control automatic module
+loading is as important as the operation itself. Restricting unprivileged
+programs or attackers that abuse this feature to load unused modules or
+modules that contain bugs is a significant security measure.
+
+The three modes that "modules_autoload_mode" support allow to provide
+restrictions on automatic module loading without breaking user
+experience.
+
+When modules_autoload_mode is set to (0), the default, there are no
+restrictions.
+
+When modules_autoload_mode is set to (1), processes must have
+CAP_SYS_MODULE to be able to trigger a module auto-load operation,
+or CAP_NET_ADMIN for modules with a 'netdev-%s' alias.
+
+When modules_autoload_mode is set to (2), automatic module loading is
+disabled for all. Once set, this value can not be changed.
+
+
+Notes on relation between "modules_disabled=0" and
+"modules_autoload_mode=2":
+1) Restricting automatic module loading does not interfere with
+explicit module load or unload operations.
+2) New features provided by modules can be made available without
+rebooting the system.
+3) A bad version of a module can be unloaded and replaced with a
+better one without rebooting the system.
+
+The original idea of module auto-load restriction comes from
+grsecurity 'GRKERNSEC_MODHARDEN' config option.
+
+==============================================================
+
 msg_next_id, sem_next_id, and shm_next_id:
 
 These three toggles allows to specify desired id for next allocated IPC
diff --git a/include/linux/module.h b/include/linux/module.h
index 21f5639..9b64896 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -261,7 +261,16 @@ struct notifier_block;
 
 #ifdef CONFIG_MODULES
 
-extern int modules_disabled; /* for sysctl */
+enum {
+	MODULES_AUTOLOAD_ALLOWED	= 0,
+	MODULES_AUTOLOAD_PRIVILEGED	= 1,
+	MODULES_AUTOLOAD_DISABLED	= 2,
+};
+
+extern int modules_disabled; /* sysctl for explicit module load/unload */
+extern int modules_autoload_mode; /* sysctl for automatic module loading */
+extern const int modules_autoload_max; /* max value for modules_autoload_mode */
+
 /* Get/put a kernel symbol (calls must be symmetric) */
 void *__symbol_get(const char *symbol);
 void *__symbol_get_gpl(const char *symbol);
@@ -497,6 +506,9 @@ bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr);
 bool is_module_percpu_address(unsigned long addr);
 bool is_module_text_address(unsigned long addr);
 
+/* Determine whether a module auto-load operation is permitted. */
+int may_autoload_module(char *kmod_name, int allow_cap);
+
 static inline bool within_module_core(unsigned long addr,
 				      const struct module *mod)
 {
@@ -641,6 +653,11 @@ static inline bool is_livepatch_module(struct module *mod)
 
 #else /* !CONFIG_MODULES... */
 
+static inline int may_autoload_module(char *kmod_name, int allow_cap)
+{
+	return -ENOSYS;
+}
+
 static inline struct module *__module_address(unsigned long addr)
 {
 	return NULL;
diff --git a/include/linux/security.h b/include/linux/security.h
index 2f4c9d3..90fe0cb 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -101,6 +101,7 @@ extern int cap_task_setscheduler(struct task_struct *p);
 extern int cap_task_setioprio(struct task_struct *p, int ioprio);
 extern int cap_task_setnice(struct task_struct *p, int nice);
 extern int cap_vm_enough_memory(struct mm_struct *mm, long pages);
+extern int cap_kernel_module_request(char *kmod_name, int allow_cap);
 
 struct msghdr;
 struct sk_buff;
@@ -928,7 +929,7 @@ static inline int security_kernel_create_files_as(struct cred *cred,
 
 static inline int security_kernel_module_request(char *kmod_name, int allow_cap)
 {
-	return 0;
+	return cap_kernel_module_request(kmod_name, allow_cap);
 }
 
 static inline int security_kernel_read_file(struct file *file,
diff --git a/kernel/module.c b/kernel/module.c
index 4a3665f..ce7a146 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -282,6 +282,8 @@ module_param(sig_enforce, bool_enable_only, 0644);
 
 /* Block module loading/unloading? */
 int modules_disabled = 0;
+int modules_autoload_mode = MODULES_AUTOLOAD_ALLOWED;
+const int modules_autoload_max = MODULES_AUTOLOAD_DISABLED;
 core_param(nomodule, modules_disabled, bint, 0);
 
 /* Waiting for a module to finish initializing? */
@@ -4296,6 +4298,46 @@ struct module *__module_text_address(unsigned long addr)
 }
 EXPORT_SYMBOL_GPL(__module_text_address);
 
+/**
+ * may_autoload_module - Determine whether a module auto-load operation
+ * is permitted
+ * @kmod_name: The module name
+ * @allow_cap: if positive, may allow to auto-load the module if this capability
+ * is set
+ *
+ * Determine whether a module auto-load operation is allowed or not. The check
+ * uses the sysctl "modules_autoload_mode" value.
+ *
+ * This allows to have more control on automatic module loading, and align it
+ * with explicit load/unload module operations. The kernel contains several
+ * modules, some of them are not updated often and may contain bugs and
+ * vulnerabilities.
+ *
+ * The "allow_cap" is passed by callers to explicitly note that the module has
+ * the appropriate alias and that the "allow_cap" capability is set. This is
+ * for backward compatibility, the aim is to have a clear picture where:
+ *
+ * 1) Implicit module loading is allowed
+ * 2) Implicit module loading as with the explicit one requires CAP_SYS_MODULE.
+ * 3) Implicit module loading as with the explicit one can be disabled.
+ *
+ * Returns 0 if the module request is allowed or -EPERM if not.
+ */
+int may_autoload_module(char *kmod_name, int allow_cap)
+{
+	if (modules_autoload_mode == MODULES_AUTOLOAD_ALLOWED)
+		return 0;
+	else if (modules_autoload_mode == MODULES_AUTOLOAD_PRIVILEGED) {
+		/* Check CAP_SYS_MODULE then allow_cap if valid */
+		if (capable(CAP_SYS_MODULE) ||
+		    (allow_cap > 0 && capable(allow_cap)))
+		       return 0;
+	}
+
+	/* MODULES_AUTOLOAD_DISABLED or not enough caps */
+	return -EPERM;
+}
+
 /* Don't grab lock, we're oopsing. */
 void print_modules(void)
 {
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 4dfba1a..727c6a7 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -186,6 +186,11 @@ static int proc_taint(struct ctl_table *table, int write,
 			       void __user *buffer, size_t *lenp, loff_t *ppos);
 #endif
 
+#ifdef CONFIG_MODULES
+static int modules_autoload_dointvec_minmax(struct ctl_table *table, int write,
+				void __user *buffer, size_t *lenp, loff_t *ppos);
+#endif
+
 #ifdef CONFIG_PRINTK
 static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
 				void __user *buffer, size_t *lenp, loff_t *ppos);
@@ -661,6 +666,16 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &one,
 		.extra2		= &one,
 	},
+	{
+		.procname	= "modules_autoload_mode",
+		.data		= &modules_autoload_mode,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		/* Handle pinning to max value */
+		.proc_handler	= modules_autoload_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= (void *)&modules_autoload_max,
+	},
 #endif
 #ifdef CONFIG_UEVENT_HELPER
 	{
@@ -2334,6 +2349,31 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
 }
 #endif
 
+#ifdef CONFIG_MODULES
+static int modules_autoload_dointvec_minmax(struct ctl_table *table, int write,
+				void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table t;
+
+	/*
+	 * Only CAP_SYS_MODULE in init user namespace are allowed to change this
+	 */
+	if (write && !capable(CAP_SYS_MODULE))
+		return -EPERM;
+
+	t = *table;
+	/*
+	 * If "modules_autoload_mode" already equals max value
+	 * MODULES_AUTOLOAD_DISABLED, then modules autoload is disabled
+	 * and can not be changed anymore.
+	 */
+	if (modules_autoload_mode == modules_autoload_max)
+		t.extra1 = t.extra2;
+
+	return proc_dointvec_minmax(&t, write, buffer, lenp, ppos);
+}
+#endif
+
 struct do_proc_dointvec_minmax_conv_param {
 	int *min;
 	int *max;
diff --git a/security/commoncap.c b/security/commoncap.c
index 7abebd7..d629d28 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1069,6 +1069,29 @@ int cap_mmap_file(struct file *file, unsigned long reqprot,
 	return 0;
 }
 
+/**
+ * cap_kernel_module_request - Determine whether a module auto-load is permitted
+ * @kmod_name: The module name
+ * @allow_cap: if positive, may allow to auto-load the module if this capability
+ * is set
+ *
+ * Determine whether a module should be automatically loaded.
+ * Returns 0 if the module request should be allowed, -EPERM if not.
+ */
+int cap_kernel_module_request(char *kmod_name, int allow_cap)
+{
+	int ret;
+	char comm[sizeof(current->comm)];
+
+	ret = may_autoload_module(kmod_name, allow_cap);
+	if (ret < 0)
+		pr_notice_ratelimited(
+			"module: automatic module loading of %.64s by \"%s\"[%d] was denied\n",
+			kmod_name, get_task_comm(comm, current), current->pid);
+
+	return ret;
+}
+
 #ifdef CONFIG_SECURITY
 
 struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
@@ -1090,6 +1113,7 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(task_setioprio, cap_task_setioprio),
 	LSM_HOOK_INIT(task_setnice, cap_task_setnice),
 	LSM_HOOK_INIT(vm_enough_memory, cap_vm_enough_memory),
+	LSM_HOOK_INIT(kernel_module_request, cap_kernel_module_request),
 };
 
 void __init capability_add_hooks(void)
-- 
2.10.2

[PATCH v4 next 3/3] modules:capabilities: add a per-task modules auto-load mode

[Djalal Harouni]

Previous patches added the global sysctl "modules_autoload_mode". This patch
make it possible to support process trees, containers, and sandboxes by
providing an inherited per-task "modules_autoload_mode" flag that cannot be
re-enabled once disabled. This allows to restrict automatic module loading
without affecting the rest of the system.

Why we need this ?

Usually a request to a kernel feature that is implemented by a module
that is not loaded may trigger automatic module loading feature,
allowing to transparently satisfy userspace, and provide numeours
features as they are needed. In this case an implicit kernel module load
operation happens.

In most cases to load or unload a kernel module, an explicit operation
happens where programs are required to have CAP_SYS_MODULE capability to
perform so. However, in general with implicit module loading, no
capabilities are required as automatic module loading is one of the most
important and transparent operations of Linux.

Recent vulnerabilities showed that automatic module loading can be
abused in order to expose more bugs. Some of these vulnerabilities are:

* DCCP use after free CVE-2017-6074 [1]
  Unprivileged to local root PoC.

* XFRM framework CVE-2017-7184 [2]
  As advertised it seems it was used to break Ubuntu at a security
  contest.

* n_hldc CVE-2017-2636
* L2TPv3 CVE-2016-10200

Currently most of Linux code is in a form of modules, and not all
modules are written or maintained in the same way. In a container or
sandbox world, apps can be moved from one context to another or from
one Linux system to another one, the ability to restrict some of these
apps to load extra kernel modules will prevent exposing some kernel
interfaces that have not been updated withing such systems.

The DCCP vulnerability CVE-2017-6074 that can be triggered by
unprivileged, or CVE-2017-7184 in the XFRM framework are some recent
real examples. CVE-2017-7184 was used to break Ubuntu at a security
contest. Ubuntu is more of desktop distro, using a global switch to
disable automatic module loading will harm users. Actually this design
will always end up being ignored by such kind of systems that need to
offer a competitive and interactive solution for their users.

>From this and from observing how apps are being run, this patch
introduces a per-task "modules_autoload_mode" to restrict automatic
module loading. This offers the following advantages:

1) Automatic module loading is still available to the rest of the
system.

2) It is easy to use in containers and sandboxes. DCCP example could
have been used to escape containers. The XFRM framework CVE-2017-7184
needs CAP_NET_ADMIN, but attackers may start to target CAP_NET_ADMIN,
a per-task flag will make it harder.

3) Suitable for desktop and more interactive Linux systems.

4) Will allow in future to implement a per user policy.
The user database format is old and not extensible, as discussed maybe
with a modern format we may achieve the following:

User=djalal
NewKernelFeatures=yes

Which means that that interactive user will be allowed to load extra
Linux features. Others, volatile accounts or guests can be easily
blocked from doing so.

5) CAP_NET_ADMIN is useful, it handles lot of operations, at same time it
started to look more like CAP_SYS_ADMIN which is overloaded. We need
CAP_NET_ADMIN, containers need it, but at same time maybe we do not
want programs running with it to load 'netdev-%s' modules. Having an
extra per-task flag allow to discharge a bit CAP_NET_ADMIN and clearly
target automatic module loading operations.

Usage:
------

To set the per-task "modules_autoload_mode":

        prctl(PR_SET_MODULES_AUTOLOAD_MODE, mode, 0, 0, 0);

When a module auto-load request is triggered by current task, then the
operation has first to satisfy the per-task access mode before attempting
to implicitly load the module. Once set, this setting is inherited across
fork, clone and execve.

Prior to use, the task must call prctl(PR_SET_NO_NEW_PRIVS, 1) or run with
CAP_SYS_ADMIN privileges in its namespace.  If these are not true, -EACCES
will be returned.  This requirement ensures that unprivileged programs cannot
affect the behaviour or surprise privileged children.

The per-task "modules_autoload_mode" supports the following values:
0       There are no restrictions, usually the default unless set
        by parent.
1       The task must have CAP_SYS_MODULE to be able to trigger a
        module auto-load operation, or CAP_NET_ADMIN for modules with
        a 'netdev-%s' alias.
2       Automatic modules loading is disabled for the current task.

The mode may only be increased, never decreased, thus ensuring that once
applied, processes can never relax their setting. This make it easy for
developers and users to handle.

Note that even if the per-task "modules_autoload_mode" allows to auto-load
the corresponding modules, automatic module loading may still fail due to
the global sysctl "modules_autoload_mode". For more details please see
Documentation/sysctl/kernel.txt, section "modules_autoload_mode".

When a request to a kernel module is denied, the module name with the
corresponding process name and its pid are logged. Administrators can use
such information to explicitly load the appropriate modules.

The original idea of module auto-load restriction comes from
'GRKERNSEC_MODHARDEN' config option.

Testing per-task or per container setup
---------------------------------------

The following tool can be used to test the feature:
https://gist.githubusercontent.com/tixxdz/f6d77e5a45f9f8cfa4bcc0ab526ce5cf/raw/5f12f98e4dfc8a94f76b13dc290f077a153e74d8/pr_modules_autoload_mode_test.c

Example 1)

Before patch:
$ lsmod | grep ipip -
$ sudo ip tunnel add mytun mode ipip remote 10.0.2.100 local 10.0.2.15 ttl 255
$ lsmod | grep ipip -
ipip                   16384  0
tunnel4                16384  1 ipip
ip_tunnel              28672  1 ipip
$ grep Modules /proc/self/status
ModulesAutoloadMode:    0

After patch:
Set task "modules_autoload_mode" to disabled.
$ lsmod | grep ipip -
$ grep Modules /proc/self/status
ModulesAutoloadMode:    0
$ su - root
# ./pr_modules_autoload_mode_test 2
task modules_autoload_mode: 2
# grep Modules /proc/self/status
ModulesAutoloadMode:    2
# ip tunnel add mytun mode ipip remote 10.0.2.100 local 10.0.2.15 ttl 255
add tunnel "tunl0" failed: No such device
...
[  634.954652] module: automatic module loading of netdev-tunl0 by "ip"[1560] was denied
[  634.955775] module: automatic module loading of tunl0 by "ip"[1560] was denied
...

Example 2)

Sample with XFRM tunnel mode.

Before patch:
$ lsmod | grep xfrm -
$ grep Modules /proc/self/status
ModulesAutoloadMode:    0
$ sudo ip xfrm state add src 10.0.2.100 dst 10.0.1.100 proto esp spi $id1 \
> reqid $id2 mode tunnel auth "hmac(sha256)" $key1 enc "cbc(aes)" $key2
$ lsmod | grep xfrm
xfrm4_mode_tunnel      16384  2

After patch:
Set task "modules_autoload_mode" to disabled.
$ lsmod | grep xfrm -
$ grep Modules /proc/self/status
ModulesAutoloadMode:    0
$ su - root
# ./pr_modules_autoload_mode_test 2
task modules_autoload_mode: 2
# grep Modules /proc/self/status
ModulesAutoloadMode:    2
# ip xfrm state add src 10.0.2.100 dst 10.0.1.100 proto esp spi $id1 \
> reqid $id2 mode tunnel auth "hmac(sha256)" $key1 enc "cbc(aes)" $key2
RTNETLINK answers: Protocol not supported
...
[ 3458.139490] module: automatic module loading of xfrm-mode-2-1 by "ip"[1506] was denied
...

Example 3)

Here we use DCCP as an example since the public PoC was against it.

DCCP use after free CVE-2017-6074 (unprivileged to local root):
The code path can be triggered by unprivileged, using the trigger.c
program for DCCP use after free [3] and that was fixed by
commit 5edabca9d4cff7f "dccp: fix freeing skb too early for IPV6_RECVPKTINFO".

Before patch:
$ lsmod | grep dccp
$ strace ./dccp_trigger
...
socket(AF_INET6, SOCK_DCCP, IPPROTO_IP) = 3
...
$ lsmod | grep dccp
dccp_ipv6              24576  5
dccp_ipv4              24576  5 dccp_ipv6
dccp                  102400  2 dccp_ipv6,dccp_ipv4
$ grep Modules /proc/self/status
ModulesAutoloadMode:    0

After patch:

Set task "modules_autoload_mode" to 1, privileged mode.
$ lsmod | grep dccp
$ ./pr_set_no_new_privs
$ grep NoNewPrivs /proc/self/status
NoNewPrivs:     1
$ ./pr_modules_autoload_mode_test 1
$ grep Modules /proc/self/status
ModulesAutoloadMode:    1
$ strace ./dccp_trigger
...
socket(AF_INET6, SOCK_DCCP, IPPROTO_IP) = -1 ESOCKTNOSUPPORT (Socket type not supported)
...
$ lsmod | grep dccp
$ dmesg
...
[ 4662.171994] module: automatic module loading of net-pf-10-proto-0-type-6 by "dccp_trigger"[1759] was denied
[ 4662.177284] module: automatic module loading of net-pf-10-proto-0 by "dccp_trigger"[1759] was denied
[ 4662.180181] module: automatic module loading of net-pf-10-proto-0-type-6 by "dccp_trigger"[1759] was denied
[ 4662.181709] module: automatic module loading of net-pf-10-proto-0 by "dccp_trigger"[1759] was denied

Set task "modules_autoload_mode" to 2, disabled mode.
$ lsmod | grep dccp
$ su - root
# ./pr_modules_autoload_mode_test 2
task modules_autoload_mode: 2
# grep Modules /proc/self/status
ModulesAutoloadMode:    2
# strace ./dccp_trigger
...
socket(AF_INET6, SOCK_DCCP, IPPROTO_IP) = -1 ESOCKTNOSUPPORT (Socket type not supported)
...
...
[ 5154.218740] module: automatic module loading of net-pf-10-proto-0-type-6 by "dccp_trigger"[1873] was denied
[ 5154.219828] module: automatic module loading of net-pf-10-proto-0 by "dccp_trigger"[1873] was denied
[ 5154.221814] module: automatic module loading of net-pf-10-proto-0-type-6 by "dccp_trigger"[1873] was denied
[ 5154.222731] module: automatic module loading of net-pf-10-proto-0 by "dccp_trigger"[1873] was denied

As showed, this blocks automatic module loading per-task. This allows to
provide a usable system, where only some sandboxed apps or containers will be
restricted to trigger automatic module loading, other parts of the
system can continue to use the system as it is which is the case of the
desktop.

[1] http://www.openwall.com/lists/oss-security/2017/02/22/3
[2] http://www.openwall.com/lists/oss-security/2017/03/29/2
[3] https://github.com/xairy/kernel-exploits/tree/master/CVE-2017-6074

Cc: Ben Hutchings <ben.hutchings@codethink.co.uk>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: James Morris <james.l.morris@oracle.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
---
 Documentation/filesystems/proc.txt                 |   3 +
 Documentation/userspace-api/index.rst              |   1 +
 .../userspace-api/modules_autoload_mode.rst        | 115 +++++++++++++++++++++
 fs/proc/array.c                                    |   6 ++
 include/linux/init_task.h                          |   8 ++
 include/linux/module.h                             |  26 ++++-
 include/linux/sched.h                              |   5 +
 include/uapi/linux/prctl.h                         |   8 ++
 kernel/module.c                                    |  61 ++++++++++-
 security/commoncap.c                               |  38 ++++++-
 10 files changed, 263 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/userspace-api/modules_autoload_mode.rst

diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index adba21b..58127f0 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -194,6 +194,7 @@ read the file /proc/PID/status:
   CapBnd: ffffffffffffffff
   NoNewPrivs:     0
   Seccomp:        0
+  ModulesAutoloadMode:    0
   voluntary_ctxt_switches:        0
   nonvoluntary_ctxt_switches:     1
 
@@ -267,6 +268,8 @@ Table 1-2: Contents of the status files (as of 4.8)
  CapBnd                      bitmap of capabilities bounding set
  NoNewPrivs                  no_new_privs, like prctl(PR_GET_NO_NEW_PRIV, ...)
  Seccomp                     seccomp mode, like prctl(PR_GET_SECCOMP, ...)
+ ModulesAutoloadMode         modules auto-load mode, like
+                             prctl(PR_GET_MODULES_AUTOLOAD_MODE, ...)
  Cpus_allowed                mask of CPUs on which this process may run
  Cpus_allowed_list           Same as previous, but in "list format"
  Mems_allowed                mask of memory nodes allowed to this process
diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
index 7b2eb1b..bfd51b7 100644
--- a/Documentation/userspace-api/index.rst
+++ b/Documentation/userspace-api/index.rst
@@ -17,6 +17,7 @@ place where this information is gathered.
    :maxdepth: 2
 
    no_new_privs
+   modules_autoload_mode
    seccomp_filter
    unshare
 
diff --git a/Documentation/userspace-api/modules_autoload_mode.rst b/Documentation/userspace-api/modules_autoload_mode.rst
new file mode 100644
index 0000000..7355b00
--- /dev/null
+++ b/Documentation/userspace-api/modules_autoload_mode.rst
@@ -0,0 +1,115 @@
+======================================
+Per-task module auto-load restrictions
+======================================
+
+
+Introduction
+============
+
+Usually a request to a kernel feature that is implemented by a module
+that is not loaded may trigger automatic module loading feature, allowing
+to transparently satisfy userspace, and provide numerous other features
+as they are needed. In this case an implicit kernel module load
+operation happens.
+
+In most cases to load or unload a kernel module, an explicit operation
+happens where programs are required to have ``CAP_SYS_MODULE`` capability
+to perform so. However, with implicit module loading, no capabilities are
+required, or only ``CAP_NET_ADMIN`` in rare cases where the module has the
+'netdev-%s' alias. Historically this was always the case as automatic
+module loading is one of the most important and transparent operations
+of Linux, users expect that their programs just work, yet, recent cases
+showed that this can be abused by unprivileged users or attackers to load
+modules that were not updated, or modules that contain bugs and
+vulnerabilities.
+
+Currently most of Linux code is in a form of modules, hence, allowing to
+control automatic module loading in some cases is as important as the
+operation itself, especially in the context where Linux is used in
+different appliances.
+
+Restricting automatic module loading allows administratros to have the
+appropriate time to update or deny module autoloading in advance. In a
+container or sandbox world where apps can be moved from one context to
+another, the ability to restrict some containers or apps to load extra
+kernel modules will prevent exposing some kernel interfaces that may not
+receive the same care as some other parts of the core. The DCCP vulnerability
+CVE-2017-6074 that can be triggered by unprivileged, or CVE-2017-7184
+in the XFRM framework are some real examples where users or programs are
+able to expose such kernel interfaces and escape their sandbox.
+
+The per-task ``modules_autoload_mode`` allow to restrict automatic module
+loading per task, preventing the kernel from exposing more of its
+interface. This is particularly useful for containers and sandboxes as
+noted above, they are restricted from affecting the rest of the system
+without affecting its functionality, automatic module loading is still
+available for others.
+
+
+Usage
+=====
+
+When the kernel is compiled with modules support ``CONFIG_MODULES``, then:
+
+``PR_SET_MODULES_AUTOLOAD_MODE``:
+        Set the current task ``modules_autoload_mode``. When a module
+        auto-load request is triggered by current task, then the
+        operation has first to satisfy the per-task access mode before
+        attempting to implicitly load the module. As an example,
+        automatic loading of modules that contain bugs or vulnerabilities
+        can be restricted and unprivileged users can no longer abuse such
+        interfaces. Once set, this setting is inherited across ``fork(2)``,
+        ``clone(2)`` and ``execve(2)``.
+
+        Prior to use, the task must call ``prctl(PR_SET_NO_NEW_PRIVS, 1)``
+        or run with ``CAP_SYS_ADMIN`` privileges in its namespace.  If
+        these are not true, ``-EACCES`` will be returned.  This requirement
+        ensures that unprivileged programs cannot affect the behaviour or
+        surprise privileged children.
+
+        Usage:
+                ``prctl(PR_SET_MODULES_AUTOLOAD_MODE, mode, 0, 0, 0);``
+
+        The 'mode' argument supports the following values:
+        0       There are no restrictions, usually the default unless set
+                by parent.
+        1       The task must have ``CAP_SYS_MODULE`` to be able to trigger a
+                module auto-load operation, or ``CAP_NET_ADMIN`` for modules
+                with a 'netdev-%s' alias.
+        2       Automatic modules loading is disabled for the current task.
+
+        The mode may only be increased, never decreased, thus ensuring
+        that once applied, processes can never relax their setting.
+
+
+        Returned values:
+        0               On success.
+        ``-EINVAL``     If 'mode' is not valid, or the operation is not
+                        supported.
+        ``-EACCES``     If task does not have ``CAP_SYS_ADMIN`` in its namespace
+                        or is not running with ``no_new_privs``.
+        ``-EPERM``      If 'mode' is less strict than current task
+                        ``modules_autoload_mode``.
+
+
+        Note that even if the per-task ``modules_autoload_mode`` allows to
+        auto-load the corresponding modules, automatic module loading
+        may still fail due to the global sysctl ``modules_autoload_mode``.
+        For more details please see Documentation/sysctl/kernel.txt,
+        section "modules_autoload_mode".
+
+
+        When a request to a kernel module is denied, the module name with the
+        corresponding process name and its pid are logged. Administrators can
+        use such information to explicitly load the appropriate modules.
+
+
+``PR_GET_MODULES_AUTOLOAD_MODE``:
+        Return the current task ``modules_autoload_mode``.
+
+        Usage:
+                ``prctl(PR_GET_MODULES_AUTOLOAD_MODE, 0, 0, 0, 0);``
+
+        Returned values:
+        mode            The task's ``modules_autoload_mode``
+        ``-ENOSYS``     If the kernel was compiled without ``CONFIG_MODULES``.
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 88c3555..b2113e9 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -88,6 +88,7 @@
 #include <linux/string_helpers.h>
 #include <linux/user_namespace.h>
 #include <linux/fs_struct.h>
+#include <linux/module.h>
 
 #include <asm/pgtable.h>
 #include <asm/processor.h>
@@ -346,10 +347,15 @@ static inline void task_cap(struct seq_file *m, struct task_struct *p)
 
 static inline void task_seccomp(struct seq_file *m, struct task_struct *p)
 {
+	int autoload = task_modules_autoload_mode(p);
+
 	seq_put_decimal_ull(m, "NoNewPrivs:\t", task_no_new_privs(p));
 #ifdef CONFIG_SECCOMP
 	seq_put_decimal_ull(m, "\nSeccomp:\t", p->seccomp.mode);
 #endif
+	if (autoload != -ENOSYS)
+		seq_put_decimal_ull(m, "\nModulesAutoloadMode:\t", autoload);
+
 	seq_putc(m, '\n');
 }
 
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index e049526..97fbb08 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -159,6 +159,13 @@ extern struct cred init_cred;
 # define INIT_CGROUP_SCHED(tsk)
 #endif
 
+#ifdef CONFIG_MODULES
+# define INIT_MODULES_AUTOLOAD_MODE(tsk)				\
+	.modules_autoload_mode = 0,
+#else
+# define INIT_MODULES_AUTOLOAD_MODE(tsk)
+#endif
+
 #ifdef CONFIG_PERF_EVENTS
 # define INIT_PERF_EVENTS(tsk)						\
 	.perf_event_mutex = 						\
@@ -257,6 +264,7 @@ extern struct cred init_cred;
 	.tasks		= LIST_HEAD_INIT(tsk.tasks),			\
 	INIT_PUSHABLE_TASKS(tsk)					\
 	INIT_CGROUP_SCHED(tsk)						\
+	INIT_MODULES_AUTOLOAD_MODE(tsk)					\
 	.ptraced	= LIST_HEAD_INIT(tsk.ptraced),			\
 	.ptrace_entry	= LIST_HEAD_INIT(tsk.ptrace_entry),		\
 	.real_parent	= &tsk,						\
diff --git a/include/linux/module.h b/include/linux/module.h
index 9b64896..9f6ec47 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -13,6 +13,7 @@
 #include <linux/kmod.h>
 #include <linux/init.h>
 #include <linux/elf.h>
+#include <linux/sched.h>
 #include <linux/stringify.h>
 #include <linux/kobject.h>
 #include <linux/moduleparam.h>
@@ -507,7 +508,16 @@ bool is_module_percpu_address(unsigned long addr);
 bool is_module_text_address(unsigned long addr);
 
 /* Determine whether a module auto-load operation is permitted. */
-int may_autoload_module(char *kmod_name, int allow_cap);
+int may_autoload_module(struct task_struct *task, char *kmod_name, int allow_cap);
+
+/* Set modules_autoload_mode of current task */
+int task_set_modules_autoload_mode(unsigned long value);
+
+/* Read task's modules_autoload_mode */
+static inline int task_modules_autoload_mode(struct task_struct *task)
+{
+	return task->modules_autoload_mode;
+}
 
 static inline bool within_module_core(unsigned long addr,
 				      const struct module *mod)
@@ -653,11 +663,23 @@ static inline bool is_livepatch_module(struct module *mod)
 
 #else /* !CONFIG_MODULES... */
 
-static inline int may_autoload_module(char *kmod_name, int allow_cap)
+static inline int may_autoload_module(struct task_struct *task, char *kmod_name,
+				      int allow_cap)
 {
 	return -ENOSYS;
 }
 
+int task_set_modules_autoload_mode(unsigned long value)
+{
+	return -ENOSYS;
+}
+
+static inline int task_modules_autoload_mode(struct task_struct *task)
+{
+	return -ENOSYS;
+}
+
+static inline bool within_module_core(unsigned long addr,
 static inline struct module *__module_address(unsigned long addr)
 {
 	return NULL;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index c533851..031a369 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -613,6 +613,11 @@ struct task_struct {
 
 	struct restart_block		restart_block;
 
+#ifdef CONFIG_MODULES
+	/* per-task modules auto-load mode */
+	unsigned			modules_autoload_mode:2;
+#endif
+
 	pid_t				pid;
 	pid_t				tgid;
 
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index a8d0759..bf73607 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -197,4 +197,12 @@ struct prctl_mm_map {
 # define PR_CAP_AMBIENT_LOWER		3
 # define PR_CAP_AMBIENT_CLEAR_ALL	4
 
+/*
+ * Control the per-task modules auto-load mode
+ *
+ * See Documentation/prctl/modules_autoload_mode.txt for more details.
+ */
+#define PR_SET_MODULES_AUTOLOAD_MODE	48
+#define PR_GET_MODULES_AUTOLOAD_MODE	49
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/module.c b/kernel/module.c
index ce7a146..8739e4c 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -4301,12 +4301,15 @@ EXPORT_SYMBOL_GPL(__module_text_address);
 /**
  * may_autoload_module - Determine whether a module auto-load operation
  * is permitted
+ * @task: The task performing the request
  * @kmod_name: The module name
  * @allow_cap: if positive, may allow to auto-load the module if this capability
  * is set
  *
- * Determine whether a module auto-load operation is allowed or not. The check
- * uses the sysctl "modules_autoload_mode" value.
+ * Determine whether a module auto-load operation is allowed or not. First we
+ * check if the task is allowed to perform the module auto-load request, we
+ * check per-task "modules_autoload_mode", if the access is not denied, then
+ * we check the global sysctl "modules_autoload_mode".
  *
  * This allows to have more control on automatic module loading, and align it
  * with explicit load/unload module operations. The kernel contains several
@@ -4323,11 +4326,14 @@ EXPORT_SYMBOL_GPL(__module_text_address);
  *
  * Returns 0 if the module request is allowed or -EPERM if not.
  */
-int may_autoload_module(char *kmod_name, int allow_cap)
+int may_autoload_module(struct task_struct *task, char *kmod_name, int allow_cap)
 {
-	if (modules_autoload_mode == MODULES_AUTOLOAD_ALLOWED)
+	unsigned int autoload = max_t(unsigned int, modules_autoload_mode,
+				      task->modules_autoload_mode);
+
+	if (autoload == MODULES_AUTOLOAD_ALLOWED)
 		return 0;
-	else if (modules_autoload_mode == MODULES_AUTOLOAD_PRIVILEGED) {
+	else if (autoload == MODULES_AUTOLOAD_PRIVILEGED) {
 		/* Check CAP_SYS_MODULE then allow_cap if valid */
 		if (capable(CAP_SYS_MODULE) ||
 		    (allow_cap > 0 && capable(allow_cap)))
@@ -4338,6 +4344,51 @@ int may_autoload_module(char *kmod_name, int allow_cap)
 	return -EPERM;
 }
 
+/**
+ * task_set_modules_autoload_mode - Set per-task modules auto-load mode
+ * @value: Value to set "modules_autoload_mode" of current task
+ *
+ * Set current task "modules_autoload_mode". The task has to have
+ * CAP_SYS_ADMIN in its namespace or be running with no_new_privs. This
+ * avoids scenarios where unprivileged tasks can affect the behaviour of
+ * privilged children by restricting module features.
+ *
+ * The task's "modules_autoload_mode" may only be increased, never decreased.
+ *
+ * Returns 0 on success, -EINVAL if @value is not valid, -EACCES if task does
+ * not have CAP_SYS_ADMIN in its namespace or is not running with no_new_privs,
+ * and finally -EPERM if @value is less strict than current task
+ * "modules_autoload_mode".
+ *
+ */
+int task_set_modules_autoload_mode(unsigned long value)
+{
+	if (value > MODULES_AUTOLOAD_DISABLED)
+		return -EINVAL;
+
+	/*
+	 * To set task "modules_autoload_mode" requires that the task has
+	 * CAP_SYS_ADMIN in its namespace or be running with no_new_privs.
+	 * This avoids scenarios where unprivileged tasks can affect the
+	 * behaviour of privileged children by restricting module features.
+	 */
+	if (!task_no_new_privs(current) &&
+	    security_capable_noaudit(current_cred(), current_user_ns(),
+				     CAP_SYS_ADMIN) != 0)
+		return -EACCES;
+
+	/*
+	 * The "modules_autoload_mode" may only be increased, never decreased,
+	 * ensuring that once applied, processes can never relax their settings.
+	 */
+	if (current->modules_autoload_mode > value)
+		return -EPERM;
+	else if (current->modules_autoload_mode < value)
+		current->modules_autoload_mode = value;
+
+	return 0;
+}
+
 /* Don't grab lock, we're oopsing. */
 void print_modules(void)
 {
diff --git a/security/commoncap.c b/security/commoncap.c
index d629d28..dbf0d51 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -886,6 +886,36 @@ static int cap_prctl_drop(unsigned long cap)
 	return commit_creds(new);
 }
 
+/*
+ * Implement PR_SET_MODULES_AUTOLOAD_MODE.
+ *
+ * Returns 0 on success, -ve on error.
+ */
+static int pr_set_modules_autoload_mode(unsigned long arg2, unsigned long arg3,
+					unsigned long arg4, unsigned long arg5)
+{
+	if (arg3 || arg4 || arg5)
+		return -EINVAL;
+
+	return task_set_modules_autoload_mode(arg2);
+}
+
+/*
+ * Implement PR_GET_MODULES_AUTOLOAD_MODE.
+ *
+ * Return current task "modules_autoload_mode", -ve on error.
+ */
+static inline int pr_get_modules_autoload_mode(unsigned long arg2,
+					       unsigned long arg3,
+					       unsigned long arg4,
+					       unsigned long arg5)
+{
+	if (arg3 || arg4 || arg5)
+		return -EINVAL;
+
+	return task_modules_autoload_mode(current);
+}
+
 /**
  * cap_task_prctl - Implement process control functions for this security module
  * @option: The process control function requested
@@ -1016,6 +1046,12 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 			return commit_creds(new);
 		}
 
+	case PR_SET_MODULES_AUTOLOAD_MODE:
+		return pr_set_modules_autoload_mode(arg2, arg3, arg4, arg5);
+
+	case PR_GET_MODULES_AUTOLOAD_MODE:
+		return pr_get_modules_autoload_mode(arg2, arg3, arg4, arg5);
+
 	default:
 		/* No functionality available - continue with default */
 		return -ENOSYS;
@@ -1083,7 +1119,7 @@ int cap_kernel_module_request(char *kmod_name, int allow_cap)
 	int ret;
 	char comm[sizeof(current->comm)];
 
-	ret = may_autoload_module(kmod_name, allow_cap);
+	ret = may_autoload_module(current, kmod_name, allow_cap);
 	if (ret < 0)
 		pr_notice_ratelimited(
 			"module: automatic module loading of %.64s by \"%s\"[%d] was denied\n",
-- 
2.10.2

Re: [PATCH v4 next 0/3] modules: automatic module loading restrictions

[Solar Designer]

Hi Djalal,

Thank you for your work on this!

On Mon, May 22, 2017 at 01:57:03PM +0200, Djalal Harouni wrote:
> *) When modules_autoload_mode is set to (2), automatic module loading is
> disabled for all. Once set, this value can not be changed.

What purpose does this securelevel-like property ("Once set, this value
can not be changed.") serve here?  I think this mode 2 is needed, but
without this extra property, which is bypassable by e.g. explicitly
loaded kernel modules anyway (and that's OK).

I'm sorry if this has been discussed before.

Alexander

Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions

[Djalal Harouni]

Hi Alexander,

On Mon, May 22, 2017 at 2:08 PM, Solar Designer <solar-cxoSlKxDwOJWk0Htik3J/w@public.gmane.org> wrote:
> Hi Djalal,
>
> Thank you for your work on this!
>
> On Mon, May 22, 2017 at 01:57:03PM +0200, Djalal Harouni wrote:
>> *) When modules_autoload_mode is set to (2), automatic module loading is
>> disabled for all. Once set, this value can not be changed.
>
> What purpose does this securelevel-like property ("Once set, this value
> can not be changed.") serve here?  I think this mode 2 is needed, but
> without this extra property, which is bypassable by e.g. explicitly
> loaded kernel modules anyway (and that's OK).

My reasoning about "Once set, this value can not be changed" is mainly for:

If you have some systems where modules are not updated for any given
reason, then the only one who will be able to load a module is an
administrator, basically this is a shortcut for:

* Apps/services can run with CAP_NET_ADMIN but they are not allowed to
auto-load 'netdev' modules.

* Explicitly loading modules can be guarded by seccomp filters *per*
app, so even if these apps have
  CAP_SYS_MODULE they won't be able to explicitly load modules, one
has to remount some sysctl /proc/ entries read-only here and remove
CAP_SYS_ADMIN for all apps anyway.

This mainly serves the purpose of these systems that do not receive
updates, if I don't want to expose those kernel interfaces what should
I do ? then if I want to unload old versions and replace them with new
ones what operation should be allowed ? and only real root of the
system can do it. Hence, the "Once set, this value can not be changed"
is more of a shortcut, also the idea was put in my mind based on how
"modules_disabled" is disabled forever, and some other interfaces. I
would say: it is easy to handle a transition from 1) "hey this system
is still up to date, some features should be exposed" to 2) "this
system is not up to date anymore, only root should expose some
features..."

Hmm, I am not sure if this answers your question ? :-)

I definitively don't want to fall into "modules_disabled" trap where
is it too strict! "Once set, this value can not be changed" means for
some users do not set it otherwise the system is unusable...

Maybe an extra "4" mode for that ? better get it right.

Thanks!

-- 
tixxdz

Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions

[Solar Designer]

On Mon, May 22, 2017 at 03:49:15PM +0200, Djalal Harouni wrote:
> On Mon, May 22, 2017 at 2:08 PM, Solar Designer <solar-cxoSlKxDwOJWk0Htik3J/w@public.gmane.org> wrote:
> > On Mon, May 22, 2017 at 01:57:03PM +0200, Djalal Harouni wrote:
> >> *) When modules_autoload_mode is set to (2), automatic module loading is
> >> disabled for all. Once set, this value can not be changed.
> >
> > What purpose does this securelevel-like property ("Once set, this value
> > can not be changed.") serve here?  I think this mode 2 is needed, but
> > without this extra property, which is bypassable by e.g. explicitly
> > loaded kernel modules anyway (and that's OK).
> 
> My reasoning about "Once set, this value can not be changed" is mainly for:
> 
> If you have some systems where modules are not updated for any given
> reason, then the only one who will be able to load a module is an
> administrator, basically this is a shortcut for:
> 
> * Apps/services can run with CAP_NET_ADMIN but they are not allowed to
> auto-load 'netdev' modules.
> 
> * Explicitly loading modules can be guarded by seccomp filters *per*
> app, so even if these apps have
>   CAP_SYS_MODULE they won't be able to explicitly load modules, one
> has to remount some sysctl /proc/ entries read-only here and remove
> CAP_SYS_ADMIN for all apps anyway.
> 
> This mainly serves the purpose of these systems that do not receive
> updates, if I don't want to expose those kernel interfaces what should
> I do ? then if I want to unload old versions and replace them with new
> ones what operation should be allowed ? and only real root of the
> system can do it. Hence, the "Once set, this value can not be changed"
> is more of a shortcut, also the idea was put in my mind based on how
> "modules_disabled" is disabled forever, and some other interfaces. I
> would say: it is easy to handle a transition from 1) "hey this system
> is still up to date, some features should be exposed" to 2) "this
> system is not up to date anymore, only root should expose some
> features..."
> 
> Hmm, I am not sure if this answers your question ? :-)

This answers my question, but in a way that I summarize as "there's no
good reason to include this securelevel-like property".

> I definitively don't want to fall into "modules_disabled" trap where
> is it too strict! "Once set, this value can not be changed" means for
> some users do not set it otherwise the system is unusable...
> 
> Maybe an extra "4" mode for that ? better get it right.

I think you should simply exclude this property from mode 2.

The module autoloading restrictions aren't meant to reduce root's
powers; they're only meant to protect processes from shooting themselves
and the system in the foot inadvertently (confused deputy).

modules_disabled may be different in that respect, although with the
rest of the kernel lacking securelevel-like support the point is moot.

We had working securelevel in 2.0.34 through 2.0.40 inclusive, but
we've lost it in 2.1+ with cap-bound apparently never becoming as
complete a replacement for it and having been lost/broken further in
2.6.25+.  I regret this, but that's a different story.  Like I say,
module autoloading doesn't even fit in with those restrictions - it's
about a totally different threat model.

Alexander

Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions

[Djalal Harouni]

On Mon, May 22, 2017 at 6:43 PM, Solar Designer <solar-cxoSlKxDwOJWk0Htik3J/w@public.gmane.org> wrote:
> On Mon, May 22, 2017 at 03:49:15PM +0200, Djalal Harouni wrote:
>> On Mon, May 22, 2017 at 2:08 PM, Solar Designer <solar-cxoSlKxDwOJWk0Htik3J/w@public.gmane.org> wrote:
>> > On Mon, May 22, 2017 at 01:57:03PM +0200, Djalal Harouni wrote:
>> >> *) When modules_autoload_mode is set to (2), automatic module loading is
>> >> disabled for all. Once set, this value can not be changed.
>> >
>> > What purpose does this securelevel-like property ("Once set, this value
>> > can not be changed.") serve here?  I think this mode 2 is needed, but
>> > without this extra property, which is bypassable by e.g. explicitly
>> > loaded kernel modules anyway (and that's OK).
>>
>> My reasoning about "Once set, this value can not be changed" is mainly for:
>>
>> If you have some systems where modules are not updated for any given
>> reason, then the only one who will be able to load a module is an
>> administrator, basically this is a shortcut for:
>>
>> * Apps/services can run with CAP_NET_ADMIN but they are not allowed to
>> auto-load 'netdev' modules.
>>
>> * Explicitly loading modules can be guarded by seccomp filters *per*
>> app, so even if these apps have
>>   CAP_SYS_MODULE they won't be able to explicitly load modules, one
>> has to remount some sysctl /proc/ entries read-only here and remove
>> CAP_SYS_ADMIN for all apps anyway.
>>
>> This mainly serves the purpose of these systems that do not receive
>> updates, if I don't want to expose those kernel interfaces what should
>> I do ? then if I want to unload old versions and replace them with new
>> ones what operation should be allowed ? and only real root of the
>> system can do it. Hence, the "Once set, this value can not be changed"
>> is more of a shortcut, also the idea was put in my mind based on how
>> "modules_disabled" is disabled forever, and some other interfaces. I
>> would say: it is easy to handle a transition from 1) "hey this system
>> is still up to date, some features should be exposed" to 2) "this
>> system is not up to date anymore, only root should expose some
>> features..."
>>
>> Hmm, I am not sure if this answers your question ? :-)
>
> This answers my question, but in a way that I summarize as "there's no
> good reason to include this securelevel-like property".
>

Hmm, sorry I did forget to add in my previous comment that with such
systems, CAP_SYS_MODULE can be used to reset the
"modules_autoload_mode" sysctl back from mode 2 to mode 1, even if we
disable it privileged tasks can be triggered to overwrite the sysctl
flag and get it back unless /proc is read-only... that's one of the
points, it should not be so easy to relax it.



>> I definitively don't want to fall into "modules_disabled" trap where
>> is it too strict! "Once set, this value can not be changed" means for
>> some users do not set it otherwise the system is unusable...
>>
>> Maybe an extra "4" mode for that ? better get it right.
>
> I think you should simply exclude this property from mode 2.
>

Ok, maybe my comment above answers this ?

What I was referring to here, is to have one small window where it is
disable for privileged and that securelevel-like like property or
disable definitively are separated. I don't have a strong opinion
here, having a usable system is important.


> The module autoloading restrictions aren't meant to reduce root's
> powers; they're only meant to protect processes from shooting themselves
> and the system in the foot inadvertently (confused deputy).
>
> modules_disabled may be different in that respect, although with the
> rest of the kernel lacking securelevel-like support the point is moot.
>
> We had working securelevel in 2.0.34 through 2.0.40 inclusive, but
> we've lost it in 2.1+ with cap-bound apparently never becoming as
> complete a replacement for it and having been lost/broken further in
> 2.6.25+.  I regret this, but that's a different story.  Like I say,
> module autoloading doesn't even fit in with those restrictions - it's
> about a totally different threat model.
>

Ok, thanks for the information, so yes it seems we do not have such a
consistent way, but this did not block Yama LSM and other sysctl to
implement their own cases, maybe it did show that it is not that easy
to have a generic securelevel mechanism ? and what we currently have
is more practical ? I can't tell here. But we definitively want to
block privileged tasks to revert the sysctl mode if the administrator
do not want automatic module loading.

Thanks!

-- 
tixxdz

Re: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument

[Kees Cook]

On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
> This is a preparation patch for the module auto-load restriction feature.
>
> In order to restrict module auto-load operations we need to check if the
> caller has CAP_SYS_MODULE capability. This allows to align security
> checks of automatic module loading with the checks of the explicit operations.
>
> However for "netdev-%s" modules, they are allowed to be loaded if
> CAP_NET_ADMIN is set. Therefore, in order to not break this assumption,
> and allow userspace to only load "netdev-%s" modules with CAP_NET_ADMIN
> capability which is considered a privileged operation, we have two
> choices: 1) parse "netdev-%s" alias and check the capability or 2) hand
> the capability form request_module() to security_kernel_module_request()
> hook and let the capability subsystem decide.
>
> After a discussion with Rusty Russell [1], the suggestion was to pass
> the capability from request_module() to security_kernel_module_request()
> for 'netdev-%s' modules that need CAP_NET_ADMIN.
>
> The patch does not update request_module(), it updates the internal
> __request_module() that will take an extra "allow_cap" argument. If
> positive, then automatic module load operation can be allowed.

I find this refactor slightly confusing. I would expect to collapse
the existing caps checks in net/core/dev_ioctl.c and
net/ipv4/tcp_cong.c, and make this a "required cap" argument, and to
add a new non-__ function instead of requiring callers use
__request_module.

request_module_capable(int cap_required, fmt, args);

adjust __request_module() for the new arg, and when cap_required !=
-1, perform a cap check.

Then make request_module pass -1 to __request_module(), and change
dev_ioctl.c (and tcp_cong.c) from:

        if (no_module && capable(CAP_NET_ADMIN))
                no_module = request_module("netdev-%s", name);
        if (no_module && capable(CAP_SYS_MODULE))
                request_module("%s", name);

to:

        if (no_module)
                no_module = request_module_capable(CAP_NET_ADMIN,
"netdev-%s", name);
        if (no_module)
                no_module = request_module_capable(CAP_SYS_MODULE, "%s", name);

that'll make the code cleaner, too.

> __request_module() will be only called by networking code which is the
> exception to this, so we do not break userspace and CAP_NET_ADMIN can
> continue to load 'netdev-%s' modules. Other kernel code should continue
> to use request_module() which calls security_kernel_module_request() and
> will check for CAP_SYS_MODULE capability in next patch. Allowing more
> control on who can trigger automatic module loading.
>
> This patch updates security_kernel_module_request() to take the
> 'allow_cap' argument and SELinux which is currently the only user of
> security_kernel_module_request() hook.
>
> Based on patch by Rusty Russell:
> https://lkml.org/lkml/2017/4/26/735
>
> Cc: Serge Hallyn <serge@hallyn.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Suggested-by: Rusty Russell <rusty@rustcorp.com.au>
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
>
> [1] https://lkml.org/lkml/2017/4/24/7
> ---
>  include/linux/kmod.h      | 15 ++++++++-------
>  include/linux/lsm_hooks.h |  4 +++-
>  include/linux/security.h  |  4 ++--
>  kernel/kmod.c             | 15 +++++++++++++--
>  net/core/dev_ioctl.c      | 10 +++++++++-
>  security/security.c       |  4 ++--
>  security/selinux/hooks.c  |  2 +-
>  7 files changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index c4e441e..a314432 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -32,18 +32,19 @@
>  extern char modprobe_path[]; /* for sysctl */
>  /* modprobe exit status on success, -ve on error.  Return value
>   * usually useless though. */
> -extern __printf(2, 3)
> -int __request_module(bool wait, const char *name, ...);
> -#define request_module(mod...) __request_module(true, mod)
> -#define request_module_nowait(mod...) __request_module(false, mod)
> +extern __printf(3, 4)
> +int __request_module(bool wait, int allow_cap, const char *name, ...);
>  #define try_then_request_module(x, mod...) \
> -       ((x) ?: (__request_module(true, mod), (x)))
> +       ((x) ?: (__request_module(true, -1, mod), (x)))
>  #else
> -static inline int request_module(const char *name, ...) { return -ENOSYS; }
> -static inline int request_module_nowait(const char *name, ...) { return -ENOSYS; }
> +static inline __printf(3, 4)
> +int __request_module(bool wait, int allow_cap, const char *name, ...)
> +{ return -ENOSYS; }
>  #define try_then_request_module(x, mod...) (x)
>  #endif
>
> +#define request_module(mod...) __request_module(true, -1, mod)
> +#define request_module_nowait(mod...) __request_module(false, -1, mod)
>
>  struct cred;
>  struct file;
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index f7914d9..7688f79 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -578,6 +578,8 @@
>   *     Ability to trigger the kernel to automatically upcall to userspace for
>   *     userspace to load a kernel module with the given name.
>   *     @kmod_name name of the module requested by the kernel
> + *     @allow_cap capability that allows to automatically load a kernel
> + *     module.

I would describe this as "required to load".

>   *     Return 0 if successful.
>   * @kernel_read_file:
>   *     Read a file specified by userspace.
> @@ -1516,7 +1518,7 @@ union security_list_options {
>         void (*cred_transfer)(struct cred *new, const struct cred *old);
>         int (*kernel_act_as)(struct cred *new, u32 secid);
>         int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
> -       int (*kernel_module_request)(char *kmod_name);
> +       int (*kernel_module_request)(char *kmod_name, int allow_cap);
>         int (*kernel_read_file)(struct file *file, enum kernel_read_file_id id);
>         int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
>                                      enum kernel_read_file_id id);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 549cb82..2f4c9d3 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -325,7 +325,7 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
>  void security_transfer_creds(struct cred *new, const struct cred *old);
>  int security_kernel_act_as(struct cred *new, u32 secid);
>  int security_kernel_create_files_as(struct cred *new, struct inode *inode);
> -int security_kernel_module_request(char *kmod_name);
> +int security_kernel_module_request(char *kmod_name, int allow_cap);
>  int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
>  int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
>                                    enum kernel_read_file_id id);
> @@ -926,7 +926,7 @@ static inline int security_kernel_create_files_as(struct cred *cred,
>         return 0;
>  }
>
> -static inline int security_kernel_module_request(char *kmod_name)
> +static inline int security_kernel_module_request(char *kmod_name, int allow_cap)
>  {
>         return 0;
>  }
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 563f97e..15c96e8 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -110,6 +110,7 @@ static int call_modprobe(char *module_name, int wait)
>  /**
>   * __request_module - try to load a kernel module
>   * @wait: wait (or not) for the operation to complete
> + * @allow_cap: if positive, may allow modprobe if this capability is set.
>   * @fmt: printf style format string for the name of the module
>   * @...: arguments as specified in the format string
>   *
> @@ -120,10 +121,20 @@ static int call_modprobe(char *module_name, int wait)
>   * must check that the service they requested is now available not blindly
>   * invoke it.
>   *
> + * If "allow_cap" is positive, The security subsystem will trust the caller
> + * that "allow_cap" may allow to load some modules with a specific alias,
> + * the security subsystem will make some exceptions based on that. This is
> + * primally useful for backward compatibility. A permission check should not
> + * be that strict and userspace should be able to continue to trigger module
> + * auto-loading if needed.
> + *
>   * If module auto-loading support is disabled then this function
>   * becomes a no-operation.
> + *
> + * This function should not be directly used by other subsystems, for that
> + * please call request_module().
>   */
> -int __request_module(bool wait, const char *fmt, ...)
> +int __request_module(bool wait, int allow_cap, const char *fmt, ...)
>  {
>         va_list args;
>         char module_name[MODULE_NAME_LEN];
> @@ -150,7 +161,7 @@ int __request_module(bool wait, const char *fmt, ...)
>         if (ret >= MODULE_NAME_LEN)
>                 return -ENAMETOOLONG;
>
> -       ret = security_kernel_module_request(module_name);
> +       ret = security_kernel_module_request(module_name, allow_cap);
>         if (ret)
>                 return ret;
>
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index b94b1d2..c494351 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -366,8 +366,16 @@ void dev_load(struct net *net, const char *name)
>         rcu_read_unlock();
>
>         no_module = !dev;
> +       /*
> +        * First do the CAP_NET_ADMIN check, then let the security
> +        * subsystem checks know that this can be allowed since this is
> +        * a "netdev-%s" module and CAP_NET_ADMIN is set.
> +        *
> +        * For this exception call __request_module().
> +        */
>         if (no_module && capable(CAP_NET_ADMIN))
> -               no_module = request_module("netdev-%s", name);
> +               no_module = __request_module(true, CAP_NET_ADMIN,
> +                                            "netdev-%s", name);
>         if (no_module && capable(CAP_SYS_MODULE))
>                 request_module("%s", name);
>  }
> diff --git a/security/security.c b/security/security.c
> index 714433e..cedb790 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1021,9 +1021,9 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode)
>         return call_int_hook(kernel_create_files_as, 0, new, inode);
>  }
>
> -int security_kernel_module_request(char *kmod_name)
> +int security_kernel_module_request(char *kmod_name, int allow_cap)
>  {
> -       return call_int_hook(kernel_module_request, 0, kmod_name);
> +       return call_int_hook(kernel_module_request, 0, kmod_name, allow_cap);
>  }
>
>  int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 158f6a0..85eeff6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3842,7 +3842,7 @@ static int selinux_kernel_create_files_as(struct cred *new, struct inode *inode)
>         return ret;
>  }
>
> -static int selinux_kernel_module_request(char *kmod_name)
> +static int selinux_kernel_module_request(char *kmod_name, int allow_cap)
>  {
>         struct common_audit_data ad;
>
> --
> 2.10.2
>

Otherwise, looks good!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v4 next 2/3] modules:capabilities: automatic module loading restriction

[Kees Cook]

On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
> [...]
> diff --git a/kernel/module.c b/kernel/module.c
> index 4a3665f..ce7a146 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -282,6 +282,8 @@ module_param(sig_enforce, bool_enable_only, 0644);
>
>  /* Block module loading/unloading? */
>  int modules_disabled = 0;
> +int modules_autoload_mode = MODULES_AUTOLOAD_ALLOWED;
> +const int modules_autoload_max = MODULES_AUTOLOAD_DISABLED;
>  core_param(nomodule, modules_disabled, bint, 0);
>
>  /* Waiting for a module to finish initializing? */
> @@ -4296,6 +4298,46 @@ struct module *__module_text_address(unsigned long addr)
>  }
>  EXPORT_SYMBOL_GPL(__module_text_address);
>
> +/**
> + * may_autoload_module - Determine whether a module auto-load operation
> + * is permitted
> + * @kmod_name: The module name
> + * @allow_cap: if positive, may allow to auto-load the module if this capability
> + * is set
> + *
> + * Determine whether a module auto-load operation is allowed or not. The check
> + * uses the sysctl "modules_autoload_mode" value.
> + *
> + * This allows to have more control on automatic module loading, and align it
> + * with explicit load/unload module operations. The kernel contains several
> + * modules, some of them are not updated often and may contain bugs and
> + * vulnerabilities.
> + *
> + * The "allow_cap" is passed by callers to explicitly note that the module has
> + * the appropriate alias and that the "allow_cap" capability is set. This is
> + * for backward compatibility, the aim is to have a clear picture where:
> + *
> + * 1) Implicit module loading is allowed
> + * 2) Implicit module loading as with the explicit one requires CAP_SYS_MODULE.
> + * 3) Implicit module loading as with the explicit one can be disabled.
> + *
> + * Returns 0 if the module request is allowed or -EPERM if not.
> + */
> +int may_autoload_module(char *kmod_name, int allow_cap)
> +{
> +       if (modules_autoload_mode == MODULES_AUTOLOAD_ALLOWED)
> +               return 0;
> +       else if (modules_autoload_mode == MODULES_AUTOLOAD_PRIVILEGED) {
> +               /* Check CAP_SYS_MODULE then allow_cap if valid */
> +               if (capable(CAP_SYS_MODULE) ||
> +                   (allow_cap > 0 && capable(allow_cap)))

With the allow_cap check already happening in my suggestion for
__request_module(), it's not needed here. (In fact, it's not even
really needed to plumb this into the hook, I don't think?

Regardless, I remain a fan. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions

[Kees Cook]

On Mon, May 22, 2017 at 12:55 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
> On Mon, May 22, 2017 at 6:43 PM, Solar Designer <solar@openwall.com> wrote:
>> On Mon, May 22, 2017 at 03:49:15PM +0200, Djalal Harouni wrote:
>>> On Mon, May 22, 2017 at 2:08 PM, Solar Designer <solar@openwall.com> wrote:
>>> > On Mon, May 22, 2017 at 01:57:03PM +0200, Djalal Harouni wrote:
>>> >> *) When modules_autoload_mode is set to (2), automatic module loading is
>>> >> disabled for all. Once set, this value can not be changed.
>>> >
>>> > What purpose does this securelevel-like property ("Once set, this value
>>> > can not be changed.") serve here?  I think this mode 2 is needed, but
>>> > without this extra property, which is bypassable by e.g. explicitly
>>> > loaded kernel modules anyway (and that's OK).
>>>
>>> My reasoning about "Once set, this value can not be changed" is mainly for:
>>>
>>> If you have some systems where modules are not updated for any given
>>> reason, then the only one who will be able to load a module is an
>>> administrator, basically this is a shortcut for:
>>>
>>> * Apps/services can run with CAP_NET_ADMIN but they are not allowed to
>>> auto-load 'netdev' modules.
>>>
>>> * Explicitly loading modules can be guarded by seccomp filters *per*
>>> app, so even if these apps have
>>>   CAP_SYS_MODULE they won't be able to explicitly load modules, one
>>> has to remount some sysctl /proc/ entries read-only here and remove
>>> CAP_SYS_ADMIN for all apps anyway.
>>>
>>> This mainly serves the purpose of these systems that do not receive
>>> updates, if I don't want to expose those kernel interfaces what should
>>> I do ? then if I want to unload old versions and replace them with new
>>> ones what operation should be allowed ? and only real root of the
>>> system can do it. Hence, the "Once set, this value can not be changed"
>>> is more of a shortcut, also the idea was put in my mind based on how
>>> "modules_disabled" is disabled forever, and some other interfaces. I
>>> would say: it is easy to handle a transition from 1) "hey this system
>>> is still up to date, some features should be exposed" to 2) "this
>>> system is not up to date anymore, only root should expose some
>>> features..."
>>>
>>> Hmm, I am not sure if this answers your question ? :-)
>>
>> This answers my question, but in a way that I summarize as "there's no
>> good reason to include this securelevel-like property".
>>
>
> Hmm, sorry I did forget to add in my previous comment that with such
> systems, CAP_SYS_MODULE can be used to reset the
> "modules_autoload_mode" sysctl back from mode 2 to mode 1, even if we
> disable it privileged tasks can be triggered to overwrite the sysctl
> flag and get it back unless /proc is read-only... that's one of the
> points, it should not be so easy to relax it.

I'm on the fence. For modules_disabled and Yama, it was tied to
CAP_SYS_ADMIN, basically designed to be a at-boot setting that could
not later be undone by an attacker gaining that privilege, keeping
them out of either kernel memory or existing user process memory.
Here, it's CAP_SYS_MODULE... it's hard to imagine the situation where
a CAP_SYS_MODULE-capable process could write to this sysctl but NOT
issue direct modprobe requests, but it's _possible_ via crazy symlink
games to trick capable processes into writing to sysctls. We've seen
this multiple times before, and it's a way for attackers to turn a
single privileged write into a privileged exec.

I might turn the question around, though: why would we want to have it
changeable at this setting?

I'm fine leaving that piece off, either way.

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions

[Andy Lutomirski]

On Mon, May 22, 2017 at 4:07 PM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, May 22, 2017 at 12:55 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
>> On Mon, May 22, 2017 at 6:43 PM, Solar Designer <solar@openwall.com> wrote:
>>> On Mon, May 22, 2017 at 03:49:15PM +0200, Djalal Harouni wrote:
>>>> On Mon, May 22, 2017 at 2:08 PM, Solar Designer <solar@openwall.com> wrote:
>>>> > On Mon, May 22, 2017 at 01:57:03PM +0200, Djalal Harouni wrote:
>>>> >> *) When modules_autoload_mode is set to (2), automatic module loading is
>>>> >> disabled for all. Once set, this value can not be changed.
>>>> >
>>>> > What purpose does this securelevel-like property ("Once set, this value
>>>> > can not be changed.") serve here?  I think this mode 2 is needed, but
>>>> > without this extra property, which is bypassable by e.g. explicitly
>>>> > loaded kernel modules anyway (and that's OK).
>>>>
>>>> My reasoning about "Once set, this value can not be changed" is mainly for:
>>>>
>>>> If you have some systems where modules are not updated for any given
>>>> reason, then the only one who will be able to load a module is an
>>>> administrator, basically this is a shortcut for:
>>>>
>>>> * Apps/services can run with CAP_NET_ADMIN but they are not allowed to
>>>> auto-load 'netdev' modules.
>>>>
>>>> * Explicitly loading modules can be guarded by seccomp filters *per*
>>>> app, so even if these apps have
>>>>   CAP_SYS_MODULE they won't be able to explicitly load modules, one
>>>> has to remount some sysctl /proc/ entries read-only here and remove
>>>> CAP_SYS_ADMIN for all apps anyway.
>>>>
>>>> This mainly serves the purpose of these systems that do not receive
>>>> updates, if I don't want to expose those kernel interfaces what should
>>>> I do ? then if I want to unload old versions and replace them with new
>>>> ones what operation should be allowed ? and only real root of the
>>>> system can do it. Hence, the "Once set, this value can not be changed"
>>>> is more of a shortcut, also the idea was put in my mind based on how
>>>> "modules_disabled" is disabled forever, and some other interfaces. I
>>>> would say: it is easy to handle a transition from 1) "hey this system
>>>> is still up to date, some features should be exposed" to 2) "this
>>>> system is not up to date anymore, only root should expose some
>>>> features..."
>>>>
>>>> Hmm, I am not sure if this answers your question ? :-)
>>>
>>> This answers my question, but in a way that I summarize as "there's no
>>> good reason to include this securelevel-like property".
>>>
>>
>> Hmm, sorry I did forget to add in my previous comment that with such
>> systems, CAP_SYS_MODULE can be used to reset the
>> "modules_autoload_mode" sysctl back from mode 2 to mode 1, even if we
>> disable it privileged tasks can be triggered to overwrite the sysctl
>> flag and get it back unless /proc is read-only... that's one of the
>> points, it should not be so easy to relax it.
>
> I'm on the fence. For modules_disabled and Yama, it was tied to
> CAP_SYS_ADMIN, basically designed to be a at-boot setting that could
> not later be undone by an attacker gaining that privilege, keeping
> them out of either kernel memory or existing user process memory.
> Here, it's CAP_SYS_MODULE... it's hard to imagine the situation where
> a CAP_SYS_MODULE-capable process could write to this sysctl but NOT
> issue direct modprobe requests, but it's _possible_ via crazy symlink
> games to trick capable processes into writing to sysctls. We've seen
> this multiple times before, and it's a way for attackers to turn a
> single privileged write into a privileged exec.
>
> I might turn the question around, though: why would we want to have it
> changeable at this setting?
>
> I'm fine leaving that piece off, either way.

I think that having the un-resettable mode is unnecessary.  We should
have option that disables loading modules entirely and cannot be
unset.  (That means no explicit loads and not implicit loads.)  Maybe
we already have this.  Otherwise, tightening caps needed for implicit
loads should just be a normal yes/no setting IMO.

Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions

[Kees Cook]

On Mon, May 22, 2017 at 4:38 PM, Andy Lutomirski <luto@kernel.org> wrote:
> I think that having the un-resettable mode is unnecessary.  We should
> have option that disables loading modules entirely and cannot be
> unset.  (That means no explicit loads and not implicit loads.)  Maybe
> we already have this.  Otherwise, tightening caps needed for implicit
> loads should just be a normal yes/no setting IMO.

Yup, /proc/sys/kernel/modules_disabled already does this.

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions

[Solar Designer]

> >>> On Mon, May 22, 2017 at 2:08 PM, Solar Designer <solar@openwall.com> wrote:
> >>> > On Mon, May 22, 2017 at 01:57:03PM +0200, Djalal Harouni wrote:
> >>> >> *) When modules_autoload_mode is set to (2), automatic module loading is
> >>> >> disabled for all. Once set, this value can not be changed.
> >>> >
> >>> > What purpose does this securelevel-like property ("Once set, this value
> >>> > can not be changed.") serve here?  I think this mode 2 is needed, but
> >>> > without this extra property, which is bypassable by e.g. explicitly
> >>> > loaded kernel modules anyway (and that's OK).

On Mon, May 22, 2017 at 04:07:56PM -0700, Kees Cook wrote:
> I'm on the fence. For modules_disabled and Yama, it was tied to
> CAP_SYS_ADMIN, basically designed to be a at-boot setting that could
> not later be undone by an attacker gaining that privilege, keeping
> them out of either kernel memory or existing user process memory.
> Here, it's CAP_SYS_MODULE... it's hard to imagine the situation where
> a CAP_SYS_MODULE-capable process could write to this sysctl but NOT
> issue direct modprobe requests, but it's _possible_ via crazy symlink
> games to trick capable processes into writing to sysctls. We've seen
> this multiple times before, and it's a way for attackers to turn a
> single privileged write into a privileged exec.

OK, tricking a process via crazy symlink games is finally a potentially
valid reason.  The question then becomes: are there perhaps so many
other important sysctl's, disk files, etc. (which the vulnerable capable
process could similarly be tricked into writing) so that specifically
resetting modules_autoload_mode isn't particularly lucrative?  I think
that the answer to that is usually yes.  Another related question: do we
really want to inconsistently single out a handful of sysctl's for this
kind of extra protection?  I think not.

I agree there are some other settings where being unable to reset them
makes sense, but I think this isn't one of those.

> I might turn the question around, though: why would we want to have it
> changeable at this setting?

Convenience for the sysadmin - being able to correct one's error (e.g.,
wrong order of shell commands), respond to new findings (thought module
autoloading was unneeded after some point, then found out some software
relies on it), change one's mind, reuse a system differently than
originally intended without a forced reboot.

> I'm fine leaving that piece off, either way.

I'm also fine with either decision.  I just thought I'd point out what
looked weird to me.

I think this is an important patch that should get in, but primarily
for modules_autoload_mode=1, which many distros could make the default
(and maybe the kernel eventually should?)

For modules_autoload_mode=2, we already seem to have the equivalent of
modprobe=/bin/true (or does it differ subtly, maybe in return values?),
which I already use at startup on a GPU box like this (preloading
modules so that the OpenCL backends wouldn't need the autoloading):

nvidia-smi
nvidia-modprobe -u -c=0
#modprobe nvidia_uvm
#modprobe fglrx

sysctl -w kernel.modprobe=/bin/true
sysctl -w kernel.hotplug=/bin/true

but it's good to also have this supported more explicitly and more
consistently through modules_autoload_mode=2 while we're at it.  So I
support having this mode as well.  I just question the need to have it
non-resettable.

Alexander

Re: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument

[Djalal Harouni]

On Tue, May 23, 2017 at 12:20 AM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tixxdz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> This is a preparation patch for the module auto-load restriction feature.
>>
>> In order to restrict module auto-load operations we need to check if the
>> caller has CAP_SYS_MODULE capability. This allows to align security
>> checks of automatic module loading with the checks of the explicit operations.
>>
>> However for "netdev-%s" modules, they are allowed to be loaded if
>> CAP_NET_ADMIN is set. Therefore, in order to not break this assumption,
>> and allow userspace to only load "netdev-%s" modules with CAP_NET_ADMIN
>> capability which is considered a privileged operation, we have two
>> choices: 1) parse "netdev-%s" alias and check the capability or 2) hand
>> the capability form request_module() to security_kernel_module_request()
>> hook and let the capability subsystem decide.
>>
>> After a discussion with Rusty Russell [1], the suggestion was to pass
>> the capability from request_module() to security_kernel_module_request()
>> for 'netdev-%s' modules that need CAP_NET_ADMIN.
>>
>> The patch does not update request_module(), it updates the internal
>> __request_module() that will take an extra "allow_cap" argument. If
>> positive, then automatic module load operation can be allowed.
>
> I find this refactor slightly confusing. I would expect to collapse
> the existing caps checks in net/core/dev_ioctl.c and
> net/ipv4/tcp_cong.c, and make this a "required cap" argument, and to
> add a new non-__ function instead of requiring callers use
> __request_module.
>
> request_module_capable(int cap_required, fmt, args);
>
> adjust __request_module() for the new arg, and when cap_required !=
> -1, perform a cap check.
>
> Then make request_module pass -1 to __request_module(), and change
> dev_ioctl.c (and tcp_cong.c) from:
>
>         if (no_module && capable(CAP_NET_ADMIN))
>                 no_module = request_module("netdev-%s", name);
>         if (no_module && capable(CAP_SYS_MODULE))
>                 request_module("%s", name);
>
> to:
>
>         if (no_module)
>                 no_module = request_module_capable(CAP_NET_ADMIN,
> "netdev-%s", name);
>         if (no_module)
>                 no_module = request_module_capable(CAP_SYS_MODULE, "%s", name);
>
> that'll make the code cleaner, too.

The refactoring in the patch is more for backward compatibility with
CAP_NET_ADMIN,
as discussed here: https://lkml.org/lkml/2017/4/26/147

I think if there is an interface request_module_capable() , then code
will use it. The DCCP code path did not check capabilities at all and
called request_module(), other code does the same.

A new interface can be abused, the result of this: we may break
"modules_autoload_mode" in mode 0 and 1. In the long term code will
want to change may_autoload_module() to also allow mode 1 to load a
module with CAP_NET_ADMIN or other caps in its own userns, resulting
in "modules_autoload_mode == 0 == 1". Without userns in the game we
may just see request_module_capable(CAP_SYS_ADMIN, ...)  . There is
already some code maybe phonet sockets ? that require CAP_SYS_ADMIN to
get the appropriate protocol.... and no one will be able to review all
this code or track new patches with request_module_capable() callers.

Kernel modules are global resources, and this patchset makes sure to
treat them that way. It aligns explicit and implicit module loading
operations with CAP_SYS_MODULE. The description is using PRIVILEGED in
regard for CAP_NET_ADMIN backward compatibility only. Capabilities
just got more useful thanks to the ambient caps, but please lets make
sure that inherited caps do not break "modules_autoload_mode=1" and
make it act like "modules_autoload_mode=0". We end up handing the
permission checks from the module subsystem to the ones that are
requesting it, the module subsystem should not blindly trust
request_module() calls that come from outdated modules.

I don't mind refactoring the code so it is better, but IMO we should
avoid exposing or playing the capability game unless there is a good
reason. If CAP_SYS_MODULE is not set, then my devices should not
autoload *old* modules without me, it is easy to set and to track.

I would also add, that the per-task module auto-load flag is using the
same *may_autoload_module()* checks for one reason: consistency. Some
capability usage in the kernel is not consistent, these patches make
sure to not fall into that trap. If you set the global sysctl for your
secure server, or the per-task for containers and sandboxes for
cluster nodes, desktops or whatever, the modules auto-load feature
will act only in one way and based on CAP_SYS_MODULE.

Does this make sense ?


>
>> __request_module() will be only called by networking code which is the
>> exception to this, so we do not break userspace and CAP_NET_ADMIN can
>> continue to load 'netdev-%s' modules. Other kernel code should continue
>> to use request_module() which calls security_kernel_module_request() and
>> will check for CAP_SYS_MODULE capability in next patch. Allowing more
>> control on who can trigger automatic module loading.
>>
>> This patch updates security_kernel_module_request() to take the
>> 'allow_cap' argument and SELinux which is currently the only user of
>> security_kernel_module_request() hook.
>>
>> Based on patch by Rusty Russell:
>> https://lkml.org/lkml/2017/4/26/735
>>
>> Cc: Serge Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
>> Cc: Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Suggested-by: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>
>> Suggested-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Signed-off-by: Djalal Harouni <tixxdz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> [1] https://lkml.org/lkml/2017/4/24/7
>> ---
>>  include/linux/kmod.h      | 15 ++++++++-------
>>  include/linux/lsm_hooks.h |  4 +++-
>>  include/linux/security.h  |  4 ++--
>>  kernel/kmod.c             | 15 +++++++++++++--
>>  net/core/dev_ioctl.c      | 10 +++++++++-
>>  security/security.c       |  4 ++--
>>  security/selinux/hooks.c  |  2 +-
>>  7 files changed, 38 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
>> index c4e441e..a314432 100644
>> --- a/include/linux/kmod.h
>> +++ b/include/linux/kmod.h
>> @@ -32,18 +32,19 @@
>>  extern char modprobe_path[]; /* for sysctl */
>>  /* modprobe exit status on success, -ve on error.  Return value
>>   * usually useless though. */
>> -extern __printf(2, 3)
>> -int __request_module(bool wait, const char *name, ...);
>> -#define request_module(mod...) __request_module(true, mod)
>> -#define request_module_nowait(mod...) __request_module(false, mod)
>> +extern __printf(3, 4)
>> +int __request_module(bool wait, int allow_cap, const char *name, ...);
>>  #define try_then_request_module(x, mod...) \
>> -       ((x) ?: (__request_module(true, mod), (x)))
>> +       ((x) ?: (__request_module(true, -1, mod), (x)))
>>  #else
>> -static inline int request_module(const char *name, ...) { return -ENOSYS; }
>> -static inline int request_module_nowait(const char *name, ...) { return -ENOSYS; }
>> +static inline __printf(3, 4)
>> +int __request_module(bool wait, int allow_cap, const char *name, ...)
>> +{ return -ENOSYS; }
>>  #define try_then_request_module(x, mod...) (x)
>>  #endif
>>
>> +#define request_module(mod...) __request_module(true, -1, mod)
>> +#define request_module_nowait(mod...) __request_module(false, -1, mod)
>>
>>  struct cred;
>>  struct file;
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index f7914d9..7688f79 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -578,6 +578,8 @@
>>   *     Ability to trigger the kernel to automatically upcall to userspace for
>>   *     userspace to load a kernel module with the given name.
>>   *     @kmod_name name of the module requested by the kernel
>> + *     @allow_cap capability that allows to automatically load a kernel
>> + *     module.
>
> I would describe this as "required to load".

Ok, will fix it.


>>   *     Return 0 if successful.
>>   * @kernel_read_file:
>>   *     Read a file specified by userspace.
>> @@ -1516,7 +1518,7 @@ union security_list_options {
>>         void (*cred_transfer)(struct cred *new, const struct cred *old);
>>         int (*kernel_act_as)(struct cred *new, u32 secid);
>>         int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
>> -       int (*kernel_module_request)(char *kmod_name);
>> +       int (*kernel_module_request)(char *kmod_name, int allow_cap);
>>         int (*kernel_read_file)(struct file *file, enum kernel_read_file_id id);
>>         int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
>>                                      enum kernel_read_file_id id);
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 549cb82..2f4c9d3 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -325,7 +325,7 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
>>  void security_transfer_creds(struct cred *new, const struct cred *old);
>>  int security_kernel_act_as(struct cred *new, u32 secid);
>>  int security_kernel_create_files_as(struct cred *new, struct inode *inode);
>> -int security_kernel_module_request(char *kmod_name);
>> +int security_kernel_module_request(char *kmod_name, int allow_cap);
>>  int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
>>  int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
>>                                    enum kernel_read_file_id id);
>> @@ -926,7 +926,7 @@ static inline int security_kernel_create_files_as(struct cred *cred,
>>         return 0;
>>  }
>>
>> -static inline int security_kernel_module_request(char *kmod_name)
>> +static inline int security_kernel_module_request(char *kmod_name, int allow_cap)
>>  {
>>         return 0;
>>  }
>> diff --git a/kernel/kmod.c b/kernel/kmod.c
>> index 563f97e..15c96e8 100644
>> --- a/kernel/kmod.c
>> +++ b/kernel/kmod.c
>> @@ -110,6 +110,7 @@ static int call_modprobe(char *module_name, int wait)
>>  /**
>>   * __request_module - try to load a kernel module
>>   * @wait: wait (or not) for the operation to complete
>> + * @allow_cap: if positive, may allow modprobe if this capability is set.
>>   * @fmt: printf style format string for the name of the module
>>   * @...: arguments as specified in the format string
>>   *
>> @@ -120,10 +121,20 @@ static int call_modprobe(char *module_name, int wait)
>>   * must check that the service they requested is now available not blindly
>>   * invoke it.
>>   *
>> + * If "allow_cap" is positive, The security subsystem will trust the caller
>> + * that "allow_cap" may allow to load some modules with a specific alias,
>> + * the security subsystem will make some exceptions based on that. This is
>> + * primally useful for backward compatibility. A permission check should not
>> + * be that strict and userspace should be able to continue to trigger module
>> + * auto-loading if needed.
>> + *
>>   * If module auto-loading support is disabled then this function
>>   * becomes a no-operation.
>> + *
>> + * This function should not be directly used by other subsystems, for that
>> + * please call request_module().
>>   */
>> -int __request_module(bool wait, const char *fmt, ...)
>> +int __request_module(bool wait, int allow_cap, const char *fmt, ...)
>>  {
>>         va_list args;
>>         char module_name[MODULE_NAME_LEN];
>> @@ -150,7 +161,7 @@ int __request_module(bool wait, const char *fmt, ...)
>>         if (ret >= MODULE_NAME_LEN)
>>                 return -ENAMETOOLONG;
>>
>> -       ret = security_kernel_module_request(module_name);
>> +       ret = security_kernel_module_request(module_name, allow_cap);
>>         if (ret)
>>                 return ret;
>>
>> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
>> index b94b1d2..c494351 100644
>> --- a/net/core/dev_ioctl.c
>> +++ b/net/core/dev_ioctl.c
>> @@ -366,8 +366,16 @@ void dev_load(struct net *net, const char *name)
>>         rcu_read_unlock();
>>
>>         no_module = !dev;
>> +       /*
>> +        * First do the CAP_NET_ADMIN check, then let the security
>> +        * subsystem checks know that this can be allowed since this is
>> +        * a "netdev-%s" module and CAP_NET_ADMIN is set.
>> +        *
>> +        * For this exception call __request_module().
>> +        */
>>         if (no_module && capable(CAP_NET_ADMIN))
>> -               no_module = request_module("netdev-%s", name);
>> +               no_module = __request_module(true, CAP_NET_ADMIN,
>> +                                            "netdev-%s", name);
>>         if (no_module && capable(CAP_SYS_MODULE))
>>                 request_module("%s", name);
>>  }
>> diff --git a/security/security.c b/security/security.c
>> index 714433e..cedb790 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -1021,9 +1021,9 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode)
>>         return call_int_hook(kernel_create_files_as, 0, new, inode);
>>  }
>>
>> -int security_kernel_module_request(char *kmod_name)
>> +int security_kernel_module_request(char *kmod_name, int allow_cap)
>>  {
>> -       return call_int_hook(kernel_module_request, 0, kmod_name);
>> +       return call_int_hook(kernel_module_request, 0, kmod_name, allow_cap);
>>  }
>>
>>  int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 158f6a0..85eeff6 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -3842,7 +3842,7 @@ static int selinux_kernel_create_files_as(struct cred *new, struct inode *inode)
>>         return ret;
>>  }
>>
>> -static int selinux_kernel_module_request(char *kmod_name)
>> +static int selinux_kernel_module_request(char *kmod_name, int allow_cap)
>>  {
>>         struct common_audit_data ad;
>>
>> --
>> 2.10.2
>>
>
> Otherwise, looks good!
>

Thanks Kees, please let me know if you agree on the refactoring bits.


-- 
tixxdz

Re: [PATCH v4 next 0/3] modules: automatic module loading restrictions

[Djalal Harouni]

On Tue, May 23, 2017 at 1:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
> On Mon, May 22, 2017 at 4:07 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Mon, May 22, 2017 at 12:55 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
>>> On Mon, May 22, 2017 at 6:43 PM, Solar Designer <solar@openwall.com> wrote:
>>>> On Mon, May 22, 2017 at 03:49:15PM +0200, Djalal Harouni wrote:
>>>>> On Mon, May 22, 2017 at 2:08 PM, Solar Designer <solar@openwall.com> wrote:
>>>>> > On Mon, May 22, 2017 at 01:57:03PM +0200, Djalal Harouni wrote:
>>>>> >> *) When modules_autoload_mode is set to (2), automatic module loading is
>>>>> >> disabled for all. Once set, this value can not be changed.
>>>>> >
>>>>> > What purpose does this securelevel-like property ("Once set, this value
>>>>> > can not be changed.") serve here?  I think this mode 2 is needed, but
>>>>> > without this extra property, which is bypassable by e.g. explicitly
>>>>> > loaded kernel modules anyway (and that's OK).
>>>>>
>>>>> My reasoning about "Once set, this value can not be changed" is mainly for:
>>>>>
>>>>> If you have some systems where modules are not updated for any given
>>>>> reason, then the only one who will be able to load a module is an
>>>>> administrator, basically this is a shortcut for:
>>>>>
>>>>> * Apps/services can run with CAP_NET_ADMIN but they are not allowed to
>>>>> auto-load 'netdev' modules.
>>>>>
>>>>> * Explicitly loading modules can be guarded by seccomp filters *per*
>>>>> app, so even if these apps have
>>>>>   CAP_SYS_MODULE they won't be able to explicitly load modules, one
>>>>> has to remount some sysctl /proc/ entries read-only here and remove
>>>>> CAP_SYS_ADMIN for all apps anyway.
>>>>>
>>>>> This mainly serves the purpose of these systems that do not receive
>>>>> updates, if I don't want to expose those kernel interfaces what should
>>>>> I do ? then if I want to unload old versions and replace them with new
>>>>> ones what operation should be allowed ? and only real root of the
>>>>> system can do it. Hence, the "Once set, this value can not be changed"
>>>>> is more of a shortcut, also the idea was put in my mind based on how
>>>>> "modules_disabled" is disabled forever, and some other interfaces. I
>>>>> would say: it is easy to handle a transition from 1) "hey this system
>>>>> is still up to date, some features should be exposed" to 2) "this
>>>>> system is not up to date anymore, only root should expose some
>>>>> features..."
>>>>>
>>>>> Hmm, I am not sure if this answers your question ? :-)
>>>>
>>>> This answers my question, but in a way that I summarize as "there's no
>>>> good reason to include this securelevel-like property".
>>>>
>>>
>>> Hmm, sorry I did forget to add in my previous comment that with such
>>> systems, CAP_SYS_MODULE can be used to reset the
>>> "modules_autoload_mode" sysctl back from mode 2 to mode 1, even if we
>>> disable it privileged tasks can be triggered to overwrite the sysctl
>>> flag and get it back unless /proc is read-only... that's one of the
>>> points, it should not be so easy to relax it.
>>
>> I'm on the fence. For modules_disabled and Yama, it was tied to
>> CAP_SYS_ADMIN, basically designed to be a at-boot setting that could
>> not later be undone by an attacker gaining that privilege, keeping
>> them out of either kernel memory or existing user process memory.
>> Here, it's CAP_SYS_MODULE... it's hard to imagine the situation where
>> a CAP_SYS_MODULE-capable process could write to this sysctl but NOT
>> issue direct modprobe requests, but it's _possible_ via crazy symlink
>> games to trick capable processes into writing to sysctls. We've seen
>> this multiple times before, and it's a way for attackers to turn a
>> single privileged write into a privileged exec.
>>
>> I might turn the question around, though: why would we want to have it
>> changeable at this setting?
>>
>> I'm fine leaving that piece off, either way.
>
> I think that having the un-resettable mode is unnecessary.  We should
> have option that disables loading modules entirely and cannot be
> unset.  (That means no explicit loads and not implicit loads.)  Maybe
> we already have this.  Otherwise, tightening caps needed for implicit
> loads should just be a normal yes/no setting IMO.

Ok, so as requested by you and Alexander in the other email, I will
remove the un-resettable property, if there is no general agreement,
better leave it for future.

I would have preferred if it is "yes/no" (in systemd the module
protection and other directives are also only 'yes/no').  But 1)
backward compatibility for unprivileged loading, 2) capabilities:
CAP_SYS_MODULE, CAP_NET_ADMIN use cases and exploits against
CAP_NET_ADMIN modules, and finally 3) separate and disable implicit
operation from the explicit one for various reasons that are noted in
the commit log like explicitly load the good module version, concluded
that we need three modes!

Thanks!

-- 
tixxdz

Re: [PATCH v4 next 3/3] modules:capabilities: add a per-task modules auto-load mode

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2548 bytes --]

Hi Djalal,

[auto build test ERROR on next-20170522]

url:    https://github.com/0day-ci/linux/commits/Djalal-Harouni/modules-automatic-module-loading-restrictions/20170523-193128
config: sparc64-allnoconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All errors (new ones prefixed by >>):

   In file included from arch/sparc/kernel/setup_64.c:32:0:
   include/linux/module.h:683:30: error: storage class specified for parameter '__module_address'
    static inline struct module *__module_address(unsigned long addr)
                                 ^~~~~~~~~~~~~~~~
   include/linux/module.h:683:30: error: parameter '__module_address' declared 'inline' [-Werror]
>> include/linux/module.h:684:1: error: 'always_inline' attribute ignored [-Werror=attributes]
    {
    ^
   include/linux/module.h:683:30: error: 'no_instrument_function' attribute applies only to functions
    static inline struct module *__module_address(unsigned long addr)
                                 ^~~~~~~~~~~~~~~~
   include/linux/module.h:684:1: error: expected ';', ',' or ')' before '{' token
    {
    ^
   cc1: all warnings being treated as errors

vim +/always_inline +684 include/linux/module.h

98702500 Djalal Harouni 2017-05-22  677  static inline int task_modules_autoload_mode(struct task_struct *task)
98702500 Djalal Harouni 2017-05-22  678  {
98702500 Djalal Harouni 2017-05-22  679  	return -ENOSYS;
98702500 Djalal Harouni 2017-05-22  680  }
98702500 Djalal Harouni 2017-05-22  681  
98702500 Djalal Harouni 2017-05-22  682  static inline bool within_module_core(unsigned long addr,
e610499e Rusty Russell  2009-03-31 @683  static inline struct module *__module_address(unsigned long addr)
e610499e Rusty Russell  2009-03-31 @684  {
e610499e Rusty Russell  2009-03-31  685  	return NULL;
e610499e Rusty Russell  2009-03-31  686  }
e610499e Rusty Russell  2009-03-31  687  

:::::: The code at line 684 was first introduced by commit
:::::: e610499e2656e61975affd0af56b26eb73964c84 module: __module_address

:::::: TO: Rusty Russell <rusty@rustcorp.com.au>
:::::: CC: Rusty Russell <rusty@rustcorp.com.au>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 5264 bytes --]

Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions

[Kees Cook]

On Tue, May 23, 2017 at 12:48 AM, Solar Designer <solar-cxoSlKxDwOJWk0Htik3J/w@public.gmane.org> wrote:
> For modules_autoload_mode=2, we already seem to have the equivalent of
> modprobe=/bin/true (or does it differ subtly, maybe in return values?),
> which I already use at startup on a GPU box like this (preloading
> modules so that the OpenCL backends wouldn't need the autoloading):
>
> nvidia-smi
> nvidia-modprobe -u -c=0
> #modprobe nvidia_uvm
> #modprobe fglrx
>
> sysctl -w kernel.modprobe=/bin/true
> sysctl -w kernel.hotplug=/bin/true
>
> but it's good to also have this supported more explicitly and more
> consistently through modules_autoload_mode=2 while we're at it.  So I
> support having this mode as well.  I just question the need to have it
> non-resettable.

I agree it's useful to have the explicit =2 state just to avoid
confusion when more systems start implementing
CONFIG_STATIC_USERMODEHELPER and kernel.modprobe becomes read-only
(though the userspace implementation may allow for some way to disable
it, etc). I just like avoiding the upcall to modprobe at all.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument

[Kees Cook]

On Tue, May 23, 2017 at 3:29 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
> On Tue, May 23, 2017 at 12:20 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
>>> This is a preparation patch for the module auto-load restriction feature.
>>>
>>> In order to restrict module auto-load operations we need to check if the
>>> caller has CAP_SYS_MODULE capability. This allows to align security
>>> checks of automatic module loading with the checks of the explicit operations.
>>>
>>> However for "netdev-%s" modules, they are allowed to be loaded if
>>> CAP_NET_ADMIN is set. Therefore, in order to not break this assumption,
>>> and allow userspace to only load "netdev-%s" modules with CAP_NET_ADMIN
>>> capability which is considered a privileged operation, we have two
>>> choices: 1) parse "netdev-%s" alias and check the capability or 2) hand
>>> the capability form request_module() to security_kernel_module_request()
>>> hook and let the capability subsystem decide.
>>>
>>> After a discussion with Rusty Russell [1], the suggestion was to pass
>>> the capability from request_module() to security_kernel_module_request()
>>> for 'netdev-%s' modules that need CAP_NET_ADMIN.
>>>
>>> The patch does not update request_module(), it updates the internal
>>> __request_module() that will take an extra "allow_cap" argument. If
>>> positive, then automatic module load operation can be allowed.
>>
>> I find this refactor slightly confusing. I would expect to collapse
>> the existing caps checks in net/core/dev_ioctl.c and
>> net/ipv4/tcp_cong.c, and make this a "required cap" argument, and to
>> add a new non-__ function instead of requiring callers use
>> __request_module.
>>
>> request_module_capable(int cap_required, fmt, args);
>>
>> adjust __request_module() for the new arg, and when cap_required !=
>> -1, perform a cap check.
>>
>> Then make request_module pass -1 to __request_module(), and change
>> dev_ioctl.c (and tcp_cong.c) from:
>>
>>         if (no_module && capable(CAP_NET_ADMIN))
>>                 no_module = request_module("netdev-%s", name);
>>         if (no_module && capable(CAP_SYS_MODULE))
>>                 request_module("%s", name);
>>
>> to:
>>
>>         if (no_module)
>>                 no_module = request_module_capable(CAP_NET_ADMIN,
>> "netdev-%s", name);
>>         if (no_module)
>>                 no_module = request_module_capable(CAP_SYS_MODULE, "%s", name);
>>
>> that'll make the code cleaner, too.
>
> The refactoring in the patch is more for backward compatibility with
> CAP_NET_ADMIN,
> as discussed here: https://lkml.org/lkml/2017/4/26/147

I think Rusty and I are saying the same thing here, and I must be not
understanding something you're trying to explain. Apologies for being
dense.

> I think if there is an interface request_module_capable() , then code
> will use it. The DCCP code path did not check capabilities at all and
> called request_module(), other code does the same.
>
> A new interface can be abused, the result of this: we may break
> "modules_autoload_mode" in mode 0 and 1. In the long term code will
> want to change may_autoload_module() to also allow mode 1 to load a
> module with CAP_NET_ADMIN or other caps in its own userns, resulting
> in "modules_autoload_mode == 0 == 1". Without userns in the game we
> may just see request_module_capable(CAP_SYS_ADMIN, ...)  . There is
> already some code maybe phonet sockets ? that require CAP_SYS_ADMIN to
> get the appropriate protocol.... and no one will be able to review all
> this code or track new patches with request_module_capable() callers.

I'm having some trouble following what you're saying here, but if I
understand, you're worried about getting the kernel into a state where
autoload state 0 == 1. Autoload 0 is "business as usual", and autoload
1 is "CAP_SYS_MODULE required to be able to trigger a module auto-load
operation, or CAP_NET_ADMIN for modules with a 'netdev-%s' alias."

In the v4 patch, under autoload==1, CAP_NET_ADMIN is needed to load
netdev- modules:

        if (no_module && capable(CAP_NET_ADMIN))
               no_module = __request_module(true, CAP_NET_ADMIN,
                                            "netdev-%s", name);

and in the LSM hook, CAP_NET_ADMIN is passed as an allowable "alias"
for the CAP_SYS_MODULE requirement:

       else if (modules_autoload_mode == MODULES_AUTOLOAD_PRIVILEGED) {
               /* Check CAP_SYS_MODULE then allow_cap if valid */
               if (capable(CAP_SYS_MODULE) ||
                   (allow_cap > 0 && capable(allow_cap)))
                      return 0;
       }

What I see is some needless double-checking. Since you're making
changes to the request_module() API, it would be possible to have
request_module_cap(), which could be checked instead of open-coding
it:

     if (no_module)
        no_module = request_module_cap(CAP_NET_ADMIN, "netdev-%s", name);

If I'm understanding your objection correctly, it's that you want to
ONLY ever provide this one-time alias for CAP_SYS_MODULE with the
netdev-%s things, and you don't want to risk having other module
loading start using request_module_cap() which would lead to
CAP_SYS_MODULE aliases in other places?

If the goal is to make sure that only privileged processes are
autoloading, I don't think adding a well defined interface for
cap-checks (request_module_cap()) would lead to a slippery slope. The
worst case scenario (which would never happen) would be all
request_module() users would convert to request_module_cap(). This
would mean that all module loading would require specific privileges.
That seems in line with autoload==1. They would not be tied to
CAP_SYS_MODULE, though, which is, I suspect, what you're concerned
about.

Even in the existing code, there is a sense about CAP_NET_ADMIN and
CAP_SYS_MODULE having different privilege levels, in that
CAP_NET_ADMIN can only load netdev-%s modules, but CAP_SYS_MODULE can
load any module. What about refining request_module_cap() to _require_
an explicit string prefix instead of an arbitrary format string? e.g.
request_module_cap(CAP_NET_ADMIN, "netdev", "%s", name) which would
make requests for ("netdev-%s", name)

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

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

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.

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

What do you think?

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions

[Andy Lutomirski]

On Tue, May 23, 2017 at 11:36 AM, Kees Cook <keescook@google.com> wrote:
> On Tue, May 23, 2017 at 12:48 AM, Solar Designer <solar@openwall.com> wrote:
>> For modules_autoload_mode=2, we already seem to have the equivalent of
>> modprobe=/bin/true (or does it differ subtly, maybe in return values?),
>> which I already use at startup on a GPU box like this (preloading
>> modules so that the OpenCL backends wouldn't need the autoloading):
>>
>> nvidia-smi
>> nvidia-modprobe -u -c=0
>> #modprobe nvidia_uvm
>> #modprobe fglrx
>>
>> sysctl -w kernel.modprobe=/bin/true
>> sysctl -w kernel.hotplug=/bin/true
>>
>> but it's good to also have this supported more explicitly and more
>> consistently through modules_autoload_mode=2 while we're at it.  So I
>> support having this mode as well.  I just question the need to have it
>> non-resettable.
>
> I agree it's useful to have the explicit =2 state just to avoid
> confusion when more systems start implementing
> CONFIG_STATIC_USERMODEHELPER and kernel.modprobe becomes read-only
> (though the userspace implementation may allow for some way to disable
> it, etc). I just like avoiding the upcall to modprobe at all.

I fully support =2 to mean "no automatic loading at all".  I dislike
making it non-resettable.  If you can write to sysctls, then, most
likely you can either call init_module() directly or the system has
module loading disabled entirely.

Re: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument

[Djalal Harouni]

On Tue, May 23, 2017 at 9:19 PM, Kees Cook <keescook@google.com> wrote:
> On Tue, May 23, 2017 at 3:29 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
[...]

>> I think if there is an interface request_module_capable() , then code
>> will use it. The DCCP code path did not check capabilities at all and
>> called request_module(), other code does the same.
>>
>> A new interface can be abused, the result of this: we may break
>> "modules_autoload_mode" in mode 0 and 1. In the long term code will
>> want to change may_autoload_module() to also allow mode 1 to load a
>> module with CAP_NET_ADMIN or other caps in its own userns, resulting
>> in "modules_autoload_mode == 0 == 1". Without userns in the game we
>> may just see request_module_capable(CAP_SYS_ADMIN, ...)  . There is
>> already some code maybe phonet sockets ? that require CAP_SYS_ADMIN to
>> get the appropriate protocol.... and no one will be able to review all
>> this code or track new patches with request_module_capable() callers.
>
> I'm having some trouble following what you're saying here, but if I
> understand, you're worried about getting the kernel into a state where
> autoload state 0 == 1. Autoload 0 is "business as usual", and autoload
> 1 is "CAP_SYS_MODULE required to be able to trigger a module auto-load
> operation, or CAP_NET_ADMIN for modules with a 'netdev-%s' alias."

Indeed.


> In the v4 patch, under autoload==1, CAP_NET_ADMIN is needed to load
> netdev- modules:
>
>         if (no_module && capable(CAP_NET_ADMIN))
>                no_module = __request_module(true, CAP_NET_ADMIN,
>                                             "netdev-%s", name);
>
> and in the LSM hook, CAP_NET_ADMIN is passed as an allowable "alias"
> for the CAP_SYS_MODULE requirement:
>
>        else if (modules_autoload_mode == MODULES_AUTOLOAD_PRIVILEGED) {
>                /* Check CAP_SYS_MODULE then allow_cap if valid */
>                if (capable(CAP_SYS_MODULE) ||
>                    (allow_cap > 0 && capable(allow_cap)))
>                       return 0;
>        }
>
> What I see is some needless double-checking. Since you're making
> changes to the request_module() API, it would be possible to have

That check is *not* a double check and it is *really* needed in v4
since how may_autoload_module() was implemented. It first checks if
'autoload' == 0 == ALLOWED, if so then it allows the operation
regardless of the capability. That's why I didn't want to touch
current network logic and assumed that net code knows what it should
do.


> request_module_cap(), which could be checked instead of open-coding
> it:
>
>      if (no_module)
>         no_module = request_module_cap(CAP_NET_ADMIN, "netdev-%s", name);
>
> If I'm understanding your objection correctly, it's that you want to
> ONLY ever provide this one-time alias for CAP_SYS_MODULE with the
> netdev-%s things, and you don't want to risk having other module
> loading start using request_module_cap() which would lead to
> CAP_SYS_MODULE aliases in other places?

Yes. We can't really track capabilities usage or new code.


> If the goal is to make sure that only privileged processes are
> autoloading, I don't think adding a well defined interface for
> cap-checks (request_module_cap()) would lead to a slippery slope. The
> worst case scenario (which would never happen) would be all
> request_module() users would convert to request_module_cap(). This

I am also concerned a bit with new code. In the documentation we
explicitly say CAP_SYS_MODULE, and new code should not break that
assumption.


> would mean that all module loading would require specific privileges.
> That seems in line with autoload==1. They would not be tied to
> CAP_SYS_MODULE, though, which is, I suspect, what you're concerned
> about.

Indeed, it is just easy to say hey it needs CAP_SYS_MODULE. The
capability usage in the module subsystem more precisely with explicit
loading is clean. CAP_SYS_MODULE is not overloaded, it has clear
focus. As you say it, we should be concerned if we blindly trust
callers and end up *aliasing* CAP_SYS_MODULE with some other cap...


> Even in the existing code, there is a sense about CAP_NET_ADMIN and
> CAP_SYS_MODULE having different privilege levels, in that
> CAP_NET_ADMIN can only load netdev-%s modules, but CAP_SYS_MODULE can
> load any module. What about refining request_module_cap() to _require_
> an explicit string prefix instead of an arbitrary format string? e.g.
> request_module_cap(CAP_NET_ADMIN, "netdev", "%s", name) which would
> make requests for ("netdev-%s", name)
>
> 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. :-)


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


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


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


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

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


+int may_autoload_module(struct task_struct *task, char *kmod_name,
+                       int require_cap, char *prefix)
+{
+       unsigned int autoload;
+       int module_require_cap = 0;
+
+       if (require_cap > 0) {
+               if (prefix == NULL || *prefix == '\0')
+                       return -EPERM;
+
+               /*
+                * 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;
+       }
+
+       /* 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;
+
+       if (autoload == MODULES_AUTOLOAD_ALLOWED)
+               return 0;
+       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;
+       }
+
+       return -EPERM;
+}
+

Re: [PATCH v4 next 0/3] modules: automatic module loading restrictions

[Djalal Harouni]

On Tue, May 23, 2017 at 9:48 AM, Solar Designer <solar@openwall.com> wrote:
>> >>> On Mon, May 22, 2017 at 2:08 PM, Solar Designer <solar@openwall.com> wrote:
>> >>> > On Mon, May 22, 2017 at 01:57:03PM +0200, Djalal Harouni wrote:
>> >>> >> *) When modules_autoload_mode is set to (2), automatic module loading is
>> >>> >> disabled for all. Once set, this value can not be changed.
>> >>> >
>> >>> > What purpose does this securelevel-like property ("Once set, this value
>> >>> > can not be changed.") serve here?  I think this mode 2 is needed, but
>> >>> > without this extra property, which is bypassable by e.g. explicitly
>> >>> > loaded kernel modules anyway (and that's OK).
>
> On Mon, May 22, 2017 at 04:07:56PM -0700, Kees Cook wrote:
>> I'm on the fence. For modules_disabled and Yama, it was tied to
>> CAP_SYS_ADMIN, basically designed to be a at-boot setting that could
>> not later be undone by an attacker gaining that privilege, keeping
>> them out of either kernel memory or existing user process memory.
>> Here, it's CAP_SYS_MODULE... it's hard to imagine the situation where
>> a CAP_SYS_MODULE-capable process could write to this sysctl but NOT
>> issue direct modprobe requests, but it's _possible_ via crazy symlink
>> games to trick capable processes into writing to sysctls. We've seen
>> this multiple times before, and it's a way for attackers to turn a
>> single privileged write into a privileged exec.
>
> OK, tricking a process via crazy symlink games is finally a potentially
> valid reason.  The question then becomes: are there perhaps so many
> other important sysctl's, disk files, etc. (which the vulnerable capable
> process could similarly be tricked into writing) so that specifically
> resetting modules_autoload_mode isn't particularly lucrative?  I think
> that the answer to that is usually yes.  Another related question: do we
> really want to inconsistently single out a handful of sysctl's for this
> kind of extra protection?  I think not.
>
> I agree there are some other settings where being unable to reset them
> makes sense, but I think this isn't one of those.
>

Alright, I already replied to Andy, since it was requested I will drop
it. I definitely prefer that we have something merged and usable ;-)


>> I might turn the question around, though: why would we want to have it
>> changeable at this setting?
>
> Convenience for the sysadmin - being able to correct one's error (e.g.,
> wrong order of shell commands), respond to new findings (thought module
> autoloading was unneeded after some point, then found out some software
> relies on it), change one's mind, reuse a system differently than
> originally intended without a forced reboot.
>
>> I'm fine leaving that piece off, either way.
>
> I'm also fine with either decision.  I just thought I'd point out what
> looked weird to me.
>
> I think this is an important patch that should get in, but primarily
> for modules_autoload_mode=1, which many distros could make the default
> (and maybe the kernel eventually should?)

I do not think that desktop, or interactive systems will set it.
Ubuntu has snaps for their app store, and they can use the per-task
flag and other vendors too Flatpak, etc.


> For modules_autoload_mode=2, we already seem to have the equivalent of
> modprobe=/bin/true (or does it differ subtly, maybe in return values?),
> which I already use at startup on a GPU box like this (preloading
> modules so that the OpenCL backends wouldn't need the autoloading):
>
> nvidia-smi
> nvidia-modprobe -u -c=0
> #modprobe nvidia_uvm
> #modprobe fglrx
>
> sysctl -w kernel.modprobe=/bin/true
> sysctl -w kernel.hotplug=/bin/true
>
> but it's good to also have this supported more explicitly and more
> consistently through modules_autoload_mode=2 while we're at it.  So I
> support having this mode as well.  I just question the need to have it
> non-resettable.
>

Ok, yes with mode=2 it is clear interface, it logs what was denied, it
is consistent with the per-task flag that we want for sandboxes and
desktop, it will work with CONFIG_STATIC_USERMODEHELPER, more
importantly it will avoid doing the usermod upcall, even if one day
the kernel support upcall into namespaces, I'm pretty sure that no one
will like the idea of namespaced modprobe paths, modules are global
anyway, this allows to say we are safe by default from any future
change that may make an upcall into the wrong context, we just avoid
that.


-- 
tixxdz

Re: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument

[Kees Cook]

On Wed, May 24, 2017 at 7:16 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
> On Tue, May 23, 2017 at 9:19 PM, Kees Cook <keescook@google.com> wrote:
>> On Tue, May 23, 2017 at 3:29 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
>> Even in the existing code, there is a sense about CAP_NET_ADMIN and
>> CAP_SYS_MODULE having different privilege levels, in that
>> CAP_NET_ADMIN can only load netdev-%s modules, but CAP_SYS_MODULE can
>> load any module. What about refining request_module_cap() to _require_
>> an explicit string prefix instead of an arbitrary format string? e.g.
>> request_module_cap(CAP_NET_ADMIN, "netdev", "%s", name) which would
>> make requests for ("netdev-%s", name)
>>
>> 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.

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

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

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

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

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

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

> +
> +       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')

> +
> +               /*
> +                * 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;

> +
> +       /* 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.

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

This test can be moved to above the AUTOLOAD_DISABLED test.

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

> +
> +       return -EPERM;
> +}
> +

So, I would suggest:

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;

        /*
         * 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);
}

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument

[Djalal Harouni]

On Tue, May 30, 2017 at 7:59 PM, Kees Cook <keescook@google.com> wrote:
[...]
>>> 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.
>
>>> 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.
>
>>> 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...
>
>>> 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!


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


>> 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:
>
>> +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.



>> +
>> +       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')
>
>> +
>> +               /*
>> +                * 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;
>
>> +
>> +       /* 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.
>
>> +
>> +       if (autoload == MODULES_AUTOLOAD_ALLOWED)
>> +               return 0;
>
> This test can be moved to above the AUTOLOAD_DISABLED test.
>
>> +       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.
>
>> +
>> +       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

Re: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument

[Kees Cook]

On Thu, Jun 1, 2017 at 7:56 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
>         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 :-) ?

Yes! Looks good. I was accidentally still thinking about the caps
checks being in the net code, but obviously, that wouldn't be the case
anymore. Thanks for the catch. :)

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

Yes, incremental. I would suggest first creating the API changes to
move a basic require_cap test into the LSM (which would drop the
open-coded capable() checks in the net code), and then add the
autoload logic in the following patches. That way the "infrastructure"
changes happen separately and do not change any behaviors, but moves
the caps test down where its wanted in the LSM, before then augmenting
the logic.

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

Great, thank you!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument

[Djalal Harouni]

Hi Kees,

On Thu, Jun 1, 2017 at 9:10 PM, Kees Cook <keescook@google.com> wrote:
> On Thu, Jun 1, 2017 at 7:56 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
...
>
>> 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!
>
> Yes, incremental. I would suggest first creating the API changes to
> move a basic require_cap test into the LSM (which would drop the
> open-coded capable() checks in the net code), and then add the
> autoload logic in the following patches. That way the "infrastructure"
> changes happen separately and do not change any behaviors, but moves
> the caps test down where its wanted in the LSM, before then augmenting
> the logic.
>
>> I just need a bit of free time to check again everything and will send
>> a v5 with all requested changes.
>
> Great, thank you!
>

So sorry was busy these last months, I picked it again, will send v5 after the
merge window.

Kees I am looking on a way to integrate a test for it, we should use
something like
the example here [1] or maybe something else ? and which module to use ?

I still did not sort this out, if anyone has some suggestions, thank
you in advance!


[1] http://openwall.com/lists/kernel-hardening/2017/05/22/7

-- 
tixxdz