* Re: [RFC PATCH v2] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Casey Schaufler @ 2019-07-18 16:39 UTC (permalink / raw)
To: Aaron Goidel, paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel, casey
In-Reply-To: <20190718143042.11059-1-acgoide@tycho.nsa.gov>
On 7/18/2019 7:30 AM, Aaron Goidel wrote:
> As of now, setting watches on filesystem objects has, at most, applied a
> check for read access to the inode, and in the case of fanotify, requires
> CAP_SYS_ADMIN. No specific security hook or permission check has been
> provided to control the setting of watches. Using any of inotify, dnotify,
> or fanotify, it is possible to observe, not only write-like operations, but
> even read access to a file. Modeling the watch as being merely a read from
> the file is insufficient for the needs of SELinux. This is due to the fact
> that read access should not necessarily imply access to information about
> when another process reads from a file. Furthermore, fanotify watches grant
> more power to an application in the form of permission events. While
> notification events are solely, unidirectional (i.e. they only pass
> information to the receiving application), permission events are blocking.
> Permission events make a request to the receiving application which will
> then reply with a decision as to whether or not that action may be
> completed. This causes the issue of the watching application having the
> ability to exercise control over the triggering process. Without drawing a
> distinction within the permission check, the ability to read would imply
> the greater ability to control an application. Additionally, mount and
> superblock watches apply to all files within the same mount or superblock.
> Read access to one file should not necessarily imply the ability to watch
> all files accessed within a given mount or superblock.
>
> In order to solve these issues, a new LSM hook is implemented and has been
> placed within the system calls for marking filesystem objects with inotify,
> fanotify, and dnotify watches. These calls to the hook are placed at the
> point at which the target inode has been resolved and are provided with the
> inode, the mask of requested notification events, and the type of object on
> which the mark is being set (inode, superblock, or mount). The mask and
> mark_type have already been translated into common FS_* values shared by
> the entirety of the fs notification infrastructure.
>
> This only provides a hook at the point of setting a watch, and presumes
> that permission to set a particular watch implies the ability to receive
> all notification about that object which match the mask. This is all that
> is required for SELinux. If other security modules require additional hooks
> or infrastructure to control delivery of notification, these can be added
> by them. It does not make sense for us to propose hooks for which we have
> no implementation. The understanding that all notifications received by the
> requesting application are all strictly of a type for which the application
> has been granted permission shows that this implementation is sufficient in
> its coverage.
>
> Security modules wishing to provide complete control over fanotify must
> also implement a security_file_open hook that validates that the access
> requested by the watching application is authorized. Fanotify has the issue
> that it returns a file descriptor with the file mode specified during
> fanotify_init() to the watching process on event. This is already covered
> by the LSM security_file_open hook if the security module implements
> checking of the requested file mode there. Otherwise, a watching process
> can obtain escalated access to a file for which it has not been authorized.
>
> The selinux_inode_notify hook implementation works by adding five new file
> permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm
> (descriptions about which will follow), and one new filesystem permission:
> watch (which is applied to superblock checks). The hook then decides which
> subset of these permissions must be held by the requesting application
> based on the contents of the provided mask and the mark_type. The
> selinux_file_open hook already checks the requested file mode and therefore
> ensures that a watching process cannot escalate its access through
> fanotify.
>
> The watch, watch_mount, and watch_sb permissions are the baseline
> permissions for setting a watch on an object and each are a requirement for
> any watch to be set on a file, mount, or superblock respectively. It should
> be noted that having either of the other two permissions (watch_reads and
> watch_with_perm) does not imply the watch, watch_mount, or watch_sb
> permission. Superblock watches further require the filesystem watch
> permission to the superblock. As there is no labeled object in view for
> mounts, there is no specific check for mount watches beyond watch_mount to
> the inode. Such a check could be added in the future, if a suitable labeled
> object existed representing the mount.
>
> The watch_reads permission is required to receive notifications from
> read-exclusive events on filesystem objects. These events include accessing
> a file for the purpose of reading and closing a file which has been opened
> read-only. This distinction has been drawn in order to provide a direct
> indication in the policy for this otherwise not obvious capability. Read
> access to a file should not necessarily imply the ability to observe read
> events on a file.
>
> Finally, watch_with_perm only applies to fanotify masks since it is the
> only way to set a mask which allows for the blocking, permission event.
> This permission is needed for any watch which is of this type. Though
> fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
> trust to root, which we do not do, and does not support least privilege.
It looks as if there may be overlap between this work and
[RFC PATCH] security, capability: pass object information to security_capable
https://www.spinics.net/lists/selinux/msg28312.html
I say this because your patch looks an awful lot like what I suggested as
the alternative to passing object information to security_capable().
It's possible that I've muddled the discussions in my brain, and that there
isn't any way to use either to do both jobs. But it would be worth a look,
and a single new hook or change to existing hook would be a Good Thing.
>
> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
> ---
> v2:
> - Adds support for mark_type
> - Adds watch_sb and watch_mount file permissions
> - Adds watch as new filesystem permission
> - LSM hook now recieves mark_type argument
> - Changed LSM hook logic to implement checks for corresponding mark_types
> - Adds missing hook description comment
> - Fixes extrainous whitespace
> - Updates patch description based on feedback
>
> fs/notify/dnotify/dnotify.c | 14 +++++++--
> fs/notify/fanotify/fanotify_user.c | 27 +++++++++++++++--
> fs/notify/inotify/inotify_user.c | 13 ++++++--
> include/linux/lsm_hooks.h | 6 ++++
> include/linux/security.h | 9 +++++-
> security/security.c | 5 +++
> security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++
> security/selinux/include/classmap.h | 5 +--
> 8 files changed, 116 insertions(+), 10 deletions(-)
>
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index 250369d6901d..4690d8a66035 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -22,6 +22,7 @@
> #include <linux/sched/signal.h>
> #include <linux/dnotify.h>
> #include <linux/init.h>
> +#include <linux/security.h>
> #include <linux/spinlock.h>
> #include <linux/slab.h>
> #include <linux/fdtable.h>
> @@ -288,6 +289,16 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out_err;
> }
>
> + /*
> + * convert the userspace DN_* "arg" to the internal FS_*
> + * defined in fsnotify
> + */
> + mask = convert_arg(arg);
> +
> + error = security_inode_notify(inode, mask, FSNOTIFY_OBJ_TYPE_INODE);
> + if (error)
> + goto out_err;
> +
> /* expect most fcntl to add new rather than augment old */
> dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
> if (!dn) {
> @@ -302,9 +313,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out_err;
> }
>
> - /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
> - mask = convert_arg(arg);
> -
> /* set up the new_fsn_mark and new_dn_mark */
> new_fsn_mark = &new_dn_mark->fsn_mark;
> fsnotify_init_mark(new_fsn_mark, dnotify_group);
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index a90bb19dcfa2..9e3137badb6b 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = {
> };
>
> static int fanotify_find_path(int dfd, const char __user *filename,
> - struct path *path, unsigned int flags)
> + struct path *path, unsigned int flags, __u64 mask)
> {
> int ret;
> + unsigned int mark_type;
>
> pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__,
> dfd, filename, flags);
> @@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename,
>
> /* you can only watch an inode if you have read permissions on it */
> ret = inode_permission(path->dentry->d_inode, MAY_READ);
> + if (ret) {
> + path_put(path);
> + goto out;
> + }
> +
> + switch (flags & FANOTIFY_MARK_TYPE_BITS) {
> + case FAN_MARK_MOUNT:
> + mark_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> + break;
> + case FAN_MARK_FILESYSTEM:
> + mark_type = FSNOTIFY_OBJ_TYPE_SB;
> + break;
> + case FAN_MARK_INODE:
> + mark_type = FSNOTIFY_OBJ_TYPE_INODE;
> + break;
> + default:
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = security_inode_notify(path->dentry->d_inode, mask, mark_type);
> if (ret)
> path_put(path);
> +
> out:
> return ret;
> }
> @@ -1014,7 +1037,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
> goto fput_and_out;
> }
>
> - ret = fanotify_find_path(dfd, pathname, &path, flags);
> + ret = fanotify_find_path(dfd, pathname, &path, flags, mask);
> if (ret)
> goto fput_and_out;
>
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index 7b53598c8804..73b321a30bbc 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -39,6 +39,7 @@
> #include <linux/poll.h>
> #include <linux/wait.h>
> #include <linux/memcontrol.h>
> +#include <linux/security.h>
>
> #include "inotify.h"
> #include "../fdinfo.h"
> @@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
> /*
> * find_inode - resolve a user-given path to a specific inode
> */
> -static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
> +static int inotify_find_inode(const char __user *dirname, struct path *path,
> + unsigned int flags, __u64 mask)
> {
> int error;
>
> @@ -351,8 +353,15 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
> return error;
> /* you can only watch an inode if you have read permissions on it */
> error = inode_permission(path->dentry->d_inode, MAY_READ);
> + if (error) {
> + path_put(path);
> + return error;
> + }
> + error = security_inode_notify(path->dentry->d_inode, mask,
> + FSNOTIFY_OBJ_TYPE_INODE);
> if (error)
> path_put(path);
> +
> return error;
> }
>
> @@ -744,7 +753,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
> if (mask & IN_ONLYDIR)
> flags |= LOOKUP_DIRECTORY;
>
> - ret = inotify_find_inode(pathname, &path, flags);
> + ret = inotify_find_inode(pathname, &path, flags, mask);
> if (ret)
> goto fput_and_out;
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..9b3f5a5f3246 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -394,6 +394,9 @@
> * Check permission before removing the extended attribute
> * identified by @name for @dentry.
> * Return 0 if permission is granted.
> + * @inode_notify:
> + * Check permissions before setting a watch on events as defined by @mask,
> + * on an object @inode, whose type is defined by @mark_type.
> * @inode_getsecurity:
> * Retrieve a copy of the extended attribute representation of the
> * security label associated with @name for @inode via @buffer. Note that
> @@ -1571,6 +1574,8 @@ union security_list_options {
> int (*inode_getxattr)(struct dentry *dentry, const char *name);
> int (*inode_listxattr)(struct dentry *dentry);
> int (*inode_removexattr)(struct dentry *dentry, const char *name);
> + int (*inode_notify)(struct inode *inode, u64 mask,
> + unsigned int mark_type);
> int (*inode_need_killpriv)(struct dentry *dentry);
> int (*inode_killpriv)(struct dentry *dentry);
> int (*inode_getsecurity)(struct inode *inode, const char *name,
> @@ -1881,6 +1886,7 @@ struct security_hook_heads {
> struct hlist_head inode_getxattr;
> struct hlist_head inode_listxattr;
> struct hlist_head inode_removexattr;
> + struct hlist_head inode_notify;
> struct hlist_head inode_need_killpriv;
> struct hlist_head inode_killpriv;
> struct hlist_head inode_getsecurity;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 659071c2e57c..b12666513138 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -301,6 +301,8 @@ int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer
> void security_inode_getsecid(struct inode *inode, u32 *secid);
> int security_inode_copy_up(struct dentry *src, struct cred **new);
> int security_inode_copy_up_xattr(const char *name);
> +int security_inode_notify(struct inode *inode, u64 mask,
> + unsigned int mark_type);
> int security_kernfs_init_security(struct kernfs_node *kn_dir,
> struct kernfs_node *kn);
> int security_file_permission(struct file *file, int mask);
> @@ -387,7 +389,6 @@ int security_ismaclabel(const char *name);
> int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
> void security_release_secctx(char *secdata, u32 seclen);
> -
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
> @@ -776,6 +777,12 @@ static inline int security_inode_removexattr(struct dentry *dentry,
> return cap_inode_removexattr(dentry, name);
> }
>
> +static inline int security_inode_notify(struct inode *inode, u64 mask,
> + unsigned int mark_type)
> +{
> + return 0;
> +}
> +
> static inline int security_inode_need_killpriv(struct dentry *dentry)
> {
> return cap_inode_need_killpriv(dentry);
> diff --git a/security/security.c b/security/security.c
> index 613a5c00e602..bc30e201c137 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1251,6 +1251,11 @@ int security_inode_removexattr(struct dentry *dentry, const char *name)
> return evm_inode_removexattr(dentry, name);
> }
>
> +int security_inode_notify(struct inode *inode, u64 mask, unsigned int mark_type)
> +{
> + return call_int_hook(inode_notify, 0, inode, mask, mark_type);
> +}
> +
> int security_inode_need_killpriv(struct dentry *dentry)
> {
> return call_int_hook(inode_need_killpriv, 0, dentry);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c61787b15f27..c967e46a34ea 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -92,6 +92,8 @@
> #include <linux/kernfs.h>
> #include <linux/stringhash.h> /* for hashlen_string() */
> #include <uapi/linux/mount.h>
> +#include <linux/fsnotify.h>
> +#include <linux/fanotify.h>
>
> #include "avc.h"
> #include "objsec.h"
> @@ -3261,6 +3263,50 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
> return -EACCES;
> }
>
> +static int selinux_inode_notify(struct inode *inode, u64 mask,
> + unsigned int mark_type)
> +{
> + int ret;
> + u32 perm;
> +
> + struct common_audit_data ad;
> +
> + ad.type = LSM_AUDIT_DATA_INODE;
> + ad.u.inode = inode;
> +
> + /*
> + * Set permission needed based on the type of mark being set.
> + * Performs an additional check for sb watches.
> + */
> + switch (mark_type) {
> + case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
> + perm = FILE__WATCH_MOUNT;
> + break;
> + case FSNOTIFY_OBJ_TYPE_SB:
> + perm = FILE__WATCH_SB;
> + ret = superblock_has_perm(current_cred(), inode->i_sb,
> + FILESYSTEM__WATCH, &ad);
> + if (ret)
> + return ret;
> + break;
> + case FSNOTIFY_OBJ_TYPE_INODE:
> + perm = FILE__WATCH;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + // check if the mask is requesting ability to set a blocking watch
> + if (mask & (ALL_FSNOTIFY_PERM_EVENTS))
> + perm |= FILE__WATCH_WITH_PERM; // if so, check that permission
> +
> + // is the mask asking to watch file reads?
> + if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
> + perm |= FILE__WATCH_READS; // check that permission as well
> +
> + return inode_has_perm(current_cred(), inode, perm, &ad);
> +}
> +
> /*
> * Copy the inode security context value to the user.
> *
> @@ -6797,6 +6843,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
> LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
> LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
> + LSM_HOOK_INIT(inode_notify, selinux_inode_notify),
>
> LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security),
>
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 201f7e588a29..32e9b03be3dd 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -7,7 +7,8 @@
>
> #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
> "rename", "execute", "quotaon", "mounton", "audit_access", \
> - "open", "execmod"
> + "open", "execmod", "watch", "watch_mount", "watch_sb", \
> + "watch_with_perm", "watch_reads"
>
> #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
> "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
> @@ -60,7 +61,7 @@ struct security_class_mapping secclass_map[] = {
> { "filesystem",
> { "mount", "remount", "unmount", "getattr",
> "relabelfrom", "relabelto", "associate", "quotamod",
> - "quotaget", NULL } },
> + "quotaget", "watch", NULL } },
> { "file",
> { COMMON_FILE_PERMS,
> "execute_no_trans", "entrypoint", NULL } },
^ permalink raw reply
* Re: [RFC PATCH v2] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Amir Goldstein @ 2019-07-18 16:16 UTC (permalink / raw)
To: Aaron Goidel
Cc: Paul Moore, selinux, LSM List, linux-fsdevel, David Howells,
Jan Kara, James Morris, Stephen Smalley, linux-kernel
In-Reply-To: <20190718143042.11059-1-acgoide@tycho.nsa.gov>
On Thu, Jul 18, 2019 at 5:31 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>
> As of now, setting watches on filesystem objects has, at most, applied a
> check for read access to the inode, and in the case of fanotify, requires
> CAP_SYS_ADMIN. No specific security hook or permission check has been
> provided to control the setting of watches. Using any of inotify, dnotify,
> or fanotify, it is possible to observe, not only write-like operations, but
> even read access to a file. Modeling the watch as being merely a read from
> the file is insufficient for the needs of SELinux. This is due to the fact
> that read access should not necessarily imply access to information about
> when another process reads from a file. Furthermore, fanotify watches grant
> more power to an application in the form of permission events. While
> notification events are solely, unidirectional (i.e. they only pass
> information to the receiving application), permission events are blocking.
> Permission events make a request to the receiving application which will
> then reply with a decision as to whether or not that action may be
> completed. This causes the issue of the watching application having the
> ability to exercise control over the triggering process. Without drawing a
> distinction within the permission check, the ability to read would imply
> the greater ability to control an application. Additionally, mount and
> superblock watches apply to all files within the same mount or superblock.
> Read access to one file should not necessarily imply the ability to watch
> all files accessed within a given mount or superblock.
>
> In order to solve these issues, a new LSM hook is implemented and has been
> placed within the system calls for marking filesystem objects with inotify,
> fanotify, and dnotify watches. These calls to the hook are placed at the
> point at which the target inode has been resolved and are provided with the
> inode, the mask of requested notification events, and the type of object on
> which the mark is being set (inode, superblock, or mount). The mask and
> mark_type have already been translated into common FS_* values shared by
> the entirety of the fs notification infrastructure.
>
> This only provides a hook at the point of setting a watch, and presumes
> that permission to set a particular watch implies the ability to receive
> all notification about that object which match the mask. This is all that
> is required for SELinux. If other security modules require additional hooks
> or infrastructure to control delivery of notification, these can be added
> by them. It does not make sense for us to propose hooks for which we have
> no implementation. The understanding that all notifications received by the
> requesting application are all strictly of a type for which the application
> has been granted permission shows that this implementation is sufficient in
> its coverage.
>
> Security modules wishing to provide complete control over fanotify must
> also implement a security_file_open hook that validates that the access
> requested by the watching application is authorized. Fanotify has the issue
> that it returns a file descriptor with the file mode specified during
> fanotify_init() to the watching process on event. This is already covered
> by the LSM security_file_open hook if the security module implements
> checking of the requested file mode there. Otherwise, a watching process
> can obtain escalated access to a file for which it has not been authorized.
>
> The selinux_inode_notify hook implementation works by adding five new file
> permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm
> (descriptions about which will follow), and one new filesystem permission:
> watch (which is applied to superblock checks). The hook then decides which
> subset of these permissions must be held by the requesting application
> based on the contents of the provided mask and the mark_type. The
> selinux_file_open hook already checks the requested file mode and therefore
> ensures that a watching process cannot escalate its access through
> fanotify.
>
> The watch, watch_mount, and watch_sb permissions are the baseline
> permissions for setting a watch on an object and each are a requirement for
> any watch to be set on a file, mount, or superblock respectively. It should
> be noted that having either of the other two permissions (watch_reads and
> watch_with_perm) does not imply the watch, watch_mount, or watch_sb
> permission. Superblock watches further require the filesystem watch
> permission to the superblock. As there is no labeled object in view for
> mounts, there is no specific check for mount watches beyond watch_mount to
> the inode. Such a check could be added in the future, if a suitable labeled
> object existed representing the mount.
>
> The watch_reads permission is required to receive notifications from
> read-exclusive events on filesystem objects. These events include accessing
> a file for the purpose of reading and closing a file which has been opened
> read-only. This distinction has been drawn in order to provide a direct
> indication in the policy for this otherwise not obvious capability. Read
> access to a file should not necessarily imply the ability to observe read
> events on a file.
>
> Finally, watch_with_perm only applies to fanotify masks since it is the
> only way to set a mask which allows for the blocking, permission event.
> This permission is needed for any watch which is of this type. Though
> fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
> trust to root, which we do not do, and does not support least privilege.
>
> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
> ---
> v2:
> - Adds support for mark_type
> - Adds watch_sb and watch_mount file permissions
> - Adds watch as new filesystem permission
> - LSM hook now recieves mark_type argument
> - Changed LSM hook logic to implement checks for corresponding mark_types
> - Adds missing hook description comment
> - Fixes extrainous whitespace
> - Updates patch description based on feedback
>
> fs/notify/dnotify/dnotify.c | 14 +++++++--
> fs/notify/fanotify/fanotify_user.c | 27 +++++++++++++++--
> fs/notify/inotify/inotify_user.c | 13 ++++++--
> include/linux/lsm_hooks.h | 6 ++++
> include/linux/security.h | 9 +++++-
> security/security.c | 5 +++
> security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++
> security/selinux/include/classmap.h | 5 +--
> 8 files changed, 116 insertions(+), 10 deletions(-)
>
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index 250369d6901d..4690d8a66035 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -22,6 +22,7 @@
> #include <linux/sched/signal.h>
> #include <linux/dnotify.h>
> #include <linux/init.h>
> +#include <linux/security.h>
> #include <linux/spinlock.h>
> #include <linux/slab.h>
> #include <linux/fdtable.h>
> @@ -288,6 +289,16 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out_err;
> }
>
> + /*
> + * convert the userspace DN_* "arg" to the internal FS_*
> + * defined in fsnotify
> + */
> + mask = convert_arg(arg);
> +
> + error = security_inode_notify(inode, mask, FSNOTIFY_OBJ_TYPE_INODE);
> + if (error)
> + goto out_err;
> +
> /* expect most fcntl to add new rather than augment old */
> dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
> if (!dn) {
> @@ -302,9 +313,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> goto out_err;
> }
>
> - /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
> - mask = convert_arg(arg);
> -
> /* set up the new_fsn_mark and new_dn_mark */
> new_fsn_mark = &new_dn_mark->fsn_mark;
> fsnotify_init_mark(new_fsn_mark, dnotify_group);
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index a90bb19dcfa2..9e3137badb6b 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = {
> };
>
> static int fanotify_find_path(int dfd, const char __user *filename,
> - struct path *path, unsigned int flags)
> + struct path *path, unsigned int flags, __u64 mask)
> {
> int ret;
> + unsigned int mark_type;
>
> pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__,
> dfd, filename, flags);
> @@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename,
>
> /* you can only watch an inode if you have read permissions on it */
> ret = inode_permission(path->dentry->d_inode, MAY_READ);
> + if (ret) {
> + path_put(path);
> + goto out;
> + }
> +
> + switch (flags & FANOTIFY_MARK_TYPE_BITS) {
> + case FAN_MARK_MOUNT:
> + mark_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> + break;
> + case FAN_MARK_FILESYSTEM:
> + mark_type = FSNOTIFY_OBJ_TYPE_SB;
> + break;
> + case FAN_MARK_INODE:
> + mark_type = FSNOTIFY_OBJ_TYPE_INODE;
> + break;
> + default:
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = security_inode_notify(path->dentry->d_inode, mask, mark_type);
If you prefer 3 hooks security_{inode,mount,sb}_notify()
please place them in fanotify_add_{inode,mount,sb}_mark().
If you prefer single hook with path argument, please pass path
down to fanotify_add_mark() and call security_path_notify() from there,
where you already have the object type argument.
> if (ret)
> path_put(path);
> +
> out:
> return ret;
> }
> @@ -1014,7 +1037,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
> goto fput_and_out;
> }
>
> - ret = fanotify_find_path(dfd, pathname, &path, flags);
> + ret = fanotify_find_path(dfd, pathname, &path, flags, mask);
> if (ret)
> goto fput_and_out;
>
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index 7b53598c8804..73b321a30bbc 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -39,6 +39,7 @@
> #include <linux/poll.h>
> #include <linux/wait.h>
> #include <linux/memcontrol.h>
> +#include <linux/security.h>
>
> #include "inotify.h"
> #include "../fdinfo.h"
> @@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
> /*
> * find_inode - resolve a user-given path to a specific inode
> */
> -static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
> +static int inotify_find_inode(const char __user *dirname, struct path *path,
> + unsigned int flags, __u64 mask)
> {
> int error;
>
> @@ -351,8 +353,15 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
> return error;
> /* you can only watch an inode if you have read permissions on it */
> error = inode_permission(path->dentry->d_inode, MAY_READ);
> + if (error) {
> + path_put(path);
> + return error;
> + }
> + error = security_inode_notify(path->dentry->d_inode, mask,
> + FSNOTIFY_OBJ_TYPE_INODE);
> if (error)
> path_put(path);
> +
> return error;
> }
>
> @@ -744,7 +753,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
> if (mask & IN_ONLYDIR)
> flags |= LOOKUP_DIRECTORY;
>
> - ret = inotify_find_inode(pathname, &path, flags);
> + ret = inotify_find_inode(pathname, &path, flags, mask);
> if (ret)
> goto fput_and_out;
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..9b3f5a5f3246 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -394,6 +394,9 @@
> * Check permission before removing the extended attribute
> * identified by @name for @dentry.
> * Return 0 if permission is granted.
> + * @inode_notify:
> + * Check permissions before setting a watch on events as defined by @mask,
> + * on an object @inode, whose type is defined by @mark_type.
This is misleading IMO.
First of all "mark_type" is already use to describe the FAN_MARK_XXX flag
value, so please choose another name. object_type if you wish.
Secondly, it does not make sense to pass an inode object when enforcing
a mount mark, because there is no reference from inode to mount.
You should either pass in @path argument to the hook or have different
hooks for different object types.
What about adding watches by kernel/audit_fsnotify.c?
Do LSMs police other LSMs???
Thanks,
Amir.
^ permalink raw reply
* Re: Preferred subj= with multiple LSMs
From: Casey Schaufler @ 2019-07-18 16:13 UTC (permalink / raw)
To: Simon McVittie
Cc: Paul Moore, Steve Grubb, Richard Guy Briggs,
linux-audit@redhat.com, Linux Security Module list, casey
In-Reply-To: <20190718131034.GA12581@horizon>
On 7/18/2019 6:10 AM, Simon McVittie wrote:
> On Wed, 17 Jul 2019 at 16:02:16 -0700, Casey Schaufler wrote:
>> We've never had to think about having general rules on
>> what security modules do before, because with only one
>> active each could do whatever it wanted without fear of
>> conflict. If there is already a character that none of
>> the existing modules use, how would it be wrong to
>> reserve it?
>>
>> Smack disallows the four characters '"/\ because quoting
>> is too important to ignore and the likelyhood that someone
>> would confuse labels with paths seemed great. I sniffed
>> around a little, but couldn't find the sets for SELinux or
>> AppArmor.
> It seems we've been here before, when I added LinuxSecurityLabel to
> https://dbus.freedesktop.org/doc/dbus-specification.html#bus-messages-get-connection-credentials
> in D-Bus.
>
> Recapping the context for those who might have missed it: in D-Bus,
> processes communicate in a hub-and-spoke topology via a central message
> bus process, which forwards messages between the other processes. Some
> other IPC systems would call this a broker. As a result of this
> indirection, the message bus is the only process in the overall system
> that is in a position to ask the kernel for the identity of the other
> processes (credentials(7) and related topics like LSM labels) using
> unforgeable kernel-guaranteed socket options like SO_PEERCRED, SO_PEERSEC
> and SO_PEERGROUPS. This means that if two processes communicate via D-Bus
> and want to know each other's identities, they have to ask the message
> bus; so the message bus needs a representation for that information. For
> LSM labels, that representation is LinuxSecurityLabel, which is defined
> in terms of SO_PEERSEC.
>
> At the time that I defined LinuxSecurityLabel, nobody was willing to
> say for sure that the label was guaranteed to be ASCII or UTF-8 (which
> is part of the specification for the D-Bus STRING ('s') type), so I
> had to encode it as an arbitrary ARRAY of BYTE ('ay') rather than as
> a STRING. I was at least told that the label wouldn't contain embedded
> '\0', and that if there is a trailing '\0', I can safely canonicalize
> the string by removing it.
>
> Also, at the time that I did that, nobody was willing to say for sure
> that there was any particular correspondence between the security
> label obtained by reading /proc/self/attr/current and the security
> label obtained by getting the SO_PEERSEC socket option: in AppArmor,
> /proc/self/attr/current is something like "unconfined\n" whereas
> SO_PEERSEC is either "unconfined" or "unconfined\0" (I forget which),
> but the consensus seemed to be that there is no guarantee that the
> presence or absence of a trailing newline wouldn't be significant to
> some non-AppArmor LSM.
>
> If LSM stacking is going to lead to syntactic restrictions being imposed
> on security labels, please could someone add them to credentials(7)
> or some other suitable documentation so user-space developers can know
> where we stand, or tell me what the restrictions and guarantees are so
> I can propose a documentation patch?
Thank you for speaking up. It's good to hear from a concerned user-space
project.
Have you been following the discussions on setting a "display" value
to specify which LSM data is presented by /proc/self/attr/current and
SO_PEERSEC? Briefly, a process can write the name of the LSM it wants
to see data from to /proc/self/attr/display, and the aforementioned
interfaces will use that LSM. If no value has been set the first LSM
registered that uses any of these interfaces gets the nod.
Does this make sense to you? We have discussed what's currently being
called the "hideous" format, selinux='a:b:c:d',apparmor='x' which
in the past, and concluded that the compatibility issues would be too
great. It's a thorny problem, and your input would be most welcome.
>
> Thanks,
> smcv
^ permalink raw reply
* Re: Preferred subj= with multiple LSMs
From: William Roberts @ 2019-07-18 15:01 UTC (permalink / raw)
To: Paul Moore
Cc: Casey Schaufler, Richard Guy Briggs, Linux Security Module list,
linux-audit@redhat.com
In-Reply-To: <CAHC9VhTQLihNQ1iGjJB=LAn=C6BQokFsjsRcj8O_O9AjqQ7HBg@mail.gmail.com>
<snip>
> > >>>> the following (something between option #2 and #3):
> > >>>> subj1_lsm=smack subj1=<smack_label> subj2_lsm=selinux
> > >>>>
> > >>>> subj2=<selinux_label> ...
> > >>> If it's not a subj= field why use the indirection?
> > >>>
> > >>> subj_smack=<smack_label> subj_selinux=<selinux_label>
FWIW +1 on this approach.
<snip>
^ permalink raw reply
* [RFC PATCH v2] fanotify, inotify, dnotify, security: add security hook for fs notifications
From: Aaron Goidel @ 2019-07-18 14:30 UTC (permalink / raw)
To: paul
Cc: selinux, linux-security-module, linux-fsdevel, dhowells, jack,
amir73il, jmorris, sds, linux-kernel, Aaron Goidel
As of now, setting watches on filesystem objects has, at most, applied a
check for read access to the inode, and in the case of fanotify, requires
CAP_SYS_ADMIN. No specific security hook or permission check has been
provided to control the setting of watches. Using any of inotify, dnotify,
or fanotify, it is possible to observe, not only write-like operations, but
even read access to a file. Modeling the watch as being merely a read from
the file is insufficient for the needs of SELinux. This is due to the fact
that read access should not necessarily imply access to information about
when another process reads from a file. Furthermore, fanotify watches grant
more power to an application in the form of permission events. While
notification events are solely, unidirectional (i.e. they only pass
information to the receiving application), permission events are blocking.
Permission events make a request to the receiving application which will
then reply with a decision as to whether or not that action may be
completed. This causes the issue of the watching application having the
ability to exercise control over the triggering process. Without drawing a
distinction within the permission check, the ability to read would imply
the greater ability to control an application. Additionally, mount and
superblock watches apply to all files within the same mount or superblock.
Read access to one file should not necessarily imply the ability to watch
all files accessed within a given mount or superblock.
In order to solve these issues, a new LSM hook is implemented and has been
placed within the system calls for marking filesystem objects with inotify,
fanotify, and dnotify watches. These calls to the hook are placed at the
point at which the target inode has been resolved and are provided with the
inode, the mask of requested notification events, and the type of object on
which the mark is being set (inode, superblock, or mount). The mask and
mark_type have already been translated into common FS_* values shared by
the entirety of the fs notification infrastructure.
This only provides a hook at the point of setting a watch, and presumes
that permission to set a particular watch implies the ability to receive
all notification about that object which match the mask. This is all that
is required for SELinux. If other security modules require additional hooks
or infrastructure to control delivery of notification, these can be added
by them. It does not make sense for us to propose hooks for which we have
no implementation. The understanding that all notifications received by the
requesting application are all strictly of a type for which the application
has been granted permission shows that this implementation is sufficient in
its coverage.
Security modules wishing to provide complete control over fanotify must
also implement a security_file_open hook that validates that the access
requested by the watching application is authorized. Fanotify has the issue
that it returns a file descriptor with the file mode specified during
fanotify_init() to the watching process on event. This is already covered
by the LSM security_file_open hook if the security module implements
checking of the requested file mode there. Otherwise, a watching process
can obtain escalated access to a file for which it has not been authorized.
The selinux_inode_notify hook implementation works by adding five new file
permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm
(descriptions about which will follow), and one new filesystem permission:
watch (which is applied to superblock checks). The hook then decides which
subset of these permissions must be held by the requesting application
based on the contents of the provided mask and the mark_type. The
selinux_file_open hook already checks the requested file mode and therefore
ensures that a watching process cannot escalate its access through
fanotify.
The watch, watch_mount, and watch_sb permissions are the baseline
permissions for setting a watch on an object and each are a requirement for
any watch to be set on a file, mount, or superblock respectively. It should
be noted that having either of the other two permissions (watch_reads and
watch_with_perm) does not imply the watch, watch_mount, or watch_sb
permission. Superblock watches further require the filesystem watch
permission to the superblock. As there is no labeled object in view for
mounts, there is no specific check for mount watches beyond watch_mount to
the inode. Such a check could be added in the future, if a suitable labeled
object existed representing the mount.
The watch_reads permission is required to receive notifications from
read-exclusive events on filesystem objects. These events include accessing
a file for the purpose of reading and closing a file which has been opened
read-only. This distinction has been drawn in order to provide a direct
indication in the policy for this otherwise not obvious capability. Read
access to a file should not necessarily imply the ability to observe read
events on a file.
Finally, watch_with_perm only applies to fanotify masks since it is the
only way to set a mask which allows for the blocking, permission event.
This permission is needed for any watch which is of this type. Though
fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit
trust to root, which we do not do, and does not support least privilege.
Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
---
v2:
- Adds support for mark_type
- Adds watch_sb and watch_mount file permissions
- Adds watch as new filesystem permission
- LSM hook now recieves mark_type argument
- Changed LSM hook logic to implement checks for corresponding mark_types
- Adds missing hook description comment
- Fixes extrainous whitespace
- Updates patch description based on feedback
fs/notify/dnotify/dnotify.c | 14 +++++++--
fs/notify/fanotify/fanotify_user.c | 27 +++++++++++++++--
fs/notify/inotify/inotify_user.c | 13 ++++++--
include/linux/lsm_hooks.h | 6 ++++
include/linux/security.h | 9 +++++-
security/security.c | 5 +++
security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 5 +--
8 files changed, 116 insertions(+), 10 deletions(-)
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 250369d6901d..4690d8a66035 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -22,6 +22,7 @@
#include <linux/sched/signal.h>
#include <linux/dnotify.h>
#include <linux/init.h>
+#include <linux/security.h>
#include <linux/spinlock.h>
#include <linux/slab.h>
#include <linux/fdtable.h>
@@ -288,6 +289,16 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
goto out_err;
}
+ /*
+ * convert the userspace DN_* "arg" to the internal FS_*
+ * defined in fsnotify
+ */
+ mask = convert_arg(arg);
+
+ error = security_inode_notify(inode, mask, FSNOTIFY_OBJ_TYPE_INODE);
+ if (error)
+ goto out_err;
+
/* expect most fcntl to add new rather than augment old */
dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL);
if (!dn) {
@@ -302,9 +313,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
goto out_err;
}
- /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */
- mask = convert_arg(arg);
-
/* set up the new_fsn_mark and new_dn_mark */
new_fsn_mark = &new_dn_mark->fsn_mark;
fsnotify_init_mark(new_fsn_mark, dnotify_group);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index a90bb19dcfa2..9e3137badb6b 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = {
};
static int fanotify_find_path(int dfd, const char __user *filename,
- struct path *path, unsigned int flags)
+ struct path *path, unsigned int flags, __u64 mask)
{
int ret;
+ unsigned int mark_type;
pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__,
dfd, filename, flags);
@@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename,
/* you can only watch an inode if you have read permissions on it */
ret = inode_permission(path->dentry->d_inode, MAY_READ);
+ if (ret) {
+ path_put(path);
+ goto out;
+ }
+
+ switch (flags & FANOTIFY_MARK_TYPE_BITS) {
+ case FAN_MARK_MOUNT:
+ mark_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
+ break;
+ case FAN_MARK_FILESYSTEM:
+ mark_type = FSNOTIFY_OBJ_TYPE_SB;
+ break;
+ case FAN_MARK_INODE:
+ mark_type = FSNOTIFY_OBJ_TYPE_INODE;
+ break;
+ default:
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = security_inode_notify(path->dentry->d_inode, mask, mark_type);
if (ret)
path_put(path);
+
out:
return ret;
}
@@ -1014,7 +1037,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
goto fput_and_out;
}
- ret = fanotify_find_path(dfd, pathname, &path, flags);
+ ret = fanotify_find_path(dfd, pathname, &path, flags, mask);
if (ret)
goto fput_and_out;
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 7b53598c8804..73b321a30bbc 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -39,6 +39,7 @@
#include <linux/poll.h>
#include <linux/wait.h>
#include <linux/memcontrol.h>
+#include <linux/security.h>
#include "inotify.h"
#include "../fdinfo.h"
@@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = {
/*
* find_inode - resolve a user-given path to a specific inode
*/
-static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags)
+static int inotify_find_inode(const char __user *dirname, struct path *path,
+ unsigned int flags, __u64 mask)
{
int error;
@@ -351,8 +353,15 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
return error;
/* you can only watch an inode if you have read permissions on it */
error = inode_permission(path->dentry->d_inode, MAY_READ);
+ if (error) {
+ path_put(path);
+ return error;
+ }
+ error = security_inode_notify(path->dentry->d_inode, mask,
+ FSNOTIFY_OBJ_TYPE_INODE);
if (error)
path_put(path);
+
return error;
}
@@ -744,7 +753,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
if (mask & IN_ONLYDIR)
flags |= LOOKUP_DIRECTORY;
- ret = inotify_find_inode(pathname, &path, flags);
+ ret = inotify_find_inode(pathname, &path, flags, mask);
if (ret)
goto fput_and_out;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cfb6a19..9b3f5a5f3246 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -394,6 +394,9 @@
* Check permission before removing the extended attribute
* identified by @name for @dentry.
* Return 0 if permission is granted.
+ * @inode_notify:
+ * Check permissions before setting a watch on events as defined by @mask,
+ * on an object @inode, whose type is defined by @mark_type.
* @inode_getsecurity:
* Retrieve a copy of the extended attribute representation of the
* security label associated with @name for @inode via @buffer. Note that
@@ -1571,6 +1574,8 @@ union security_list_options {
int (*inode_getxattr)(struct dentry *dentry, const char *name);
int (*inode_listxattr)(struct dentry *dentry);
int (*inode_removexattr)(struct dentry *dentry, const char *name);
+ int (*inode_notify)(struct inode *inode, u64 mask,
+ unsigned int mark_type);
int (*inode_need_killpriv)(struct dentry *dentry);
int (*inode_killpriv)(struct dentry *dentry);
int (*inode_getsecurity)(struct inode *inode, const char *name,
@@ -1881,6 +1886,7 @@ struct security_hook_heads {
struct hlist_head inode_getxattr;
struct hlist_head inode_listxattr;
struct hlist_head inode_removexattr;
+ struct hlist_head inode_notify;
struct hlist_head inode_need_killpriv;
struct hlist_head inode_killpriv;
struct hlist_head inode_getsecurity;
diff --git a/include/linux/security.h b/include/linux/security.h
index 659071c2e57c..b12666513138 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -301,6 +301,8 @@ int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer
void security_inode_getsecid(struct inode *inode, u32 *secid);
int security_inode_copy_up(struct dentry *src, struct cred **new);
int security_inode_copy_up_xattr(const char *name);
+int security_inode_notify(struct inode *inode, u64 mask,
+ unsigned int mark_type);
int security_kernfs_init_security(struct kernfs_node *kn_dir,
struct kernfs_node *kn);
int security_file_permission(struct file *file, int mask);
@@ -387,7 +389,6 @@ int security_ismaclabel(const char *name);
int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
void security_release_secctx(char *secdata, u32 seclen);
-
void security_inode_invalidate_secctx(struct inode *inode);
int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
@@ -776,6 +777,12 @@ static inline int security_inode_removexattr(struct dentry *dentry,
return cap_inode_removexattr(dentry, name);
}
+static inline int security_inode_notify(struct inode *inode, u64 mask,
+ unsigned int mark_type)
+{
+ return 0;
+}
+
static inline int security_inode_need_killpriv(struct dentry *dentry)
{
return cap_inode_need_killpriv(dentry);
diff --git a/security/security.c b/security/security.c
index 613a5c00e602..bc30e201c137 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1251,6 +1251,11 @@ int security_inode_removexattr(struct dentry *dentry, const char *name)
return evm_inode_removexattr(dentry, name);
}
+int security_inode_notify(struct inode *inode, u64 mask, unsigned int mark_type)
+{
+ return call_int_hook(inode_notify, 0, inode, mask, mark_type);
+}
+
int security_inode_need_killpriv(struct dentry *dentry)
{
return call_int_hook(inode_need_killpriv, 0, dentry);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c61787b15f27..c967e46a34ea 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -92,6 +92,8 @@
#include <linux/kernfs.h>
#include <linux/stringhash.h> /* for hashlen_string() */
#include <uapi/linux/mount.h>
+#include <linux/fsnotify.h>
+#include <linux/fanotify.h>
#include "avc.h"
#include "objsec.h"
@@ -3261,6 +3263,50 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name)
return -EACCES;
}
+static int selinux_inode_notify(struct inode *inode, u64 mask,
+ unsigned int mark_type)
+{
+ int ret;
+ u32 perm;
+
+ struct common_audit_data ad;
+
+ ad.type = LSM_AUDIT_DATA_INODE;
+ ad.u.inode = inode;
+
+ /*
+ * Set permission needed based on the type of mark being set.
+ * Performs an additional check for sb watches.
+ */
+ switch (mark_type) {
+ case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
+ perm = FILE__WATCH_MOUNT;
+ break;
+ case FSNOTIFY_OBJ_TYPE_SB:
+ perm = FILE__WATCH_SB;
+ ret = superblock_has_perm(current_cred(), inode->i_sb,
+ FILESYSTEM__WATCH, &ad);
+ if (ret)
+ return ret;
+ break;
+ case FSNOTIFY_OBJ_TYPE_INODE:
+ perm = FILE__WATCH;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ // check if the mask is requesting ability to set a blocking watch
+ if (mask & (ALL_FSNOTIFY_PERM_EVENTS))
+ perm |= FILE__WATCH_WITH_PERM; // if so, check that permission
+
+ // is the mask asking to watch file reads?
+ if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE))
+ perm |= FILE__WATCH_READS; // check that permission as well
+
+ return inode_has_perm(current_cred(), inode, perm, &ad);
+}
+
/*
* Copy the inode security context value to the user.
*
@@ -6797,6 +6843,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid),
LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
+ LSM_HOOK_INIT(inode_notify, selinux_inode_notify),
LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 201f7e588a29..32e9b03be3dd 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -7,7 +7,8 @@
#define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
"rename", "execute", "quotaon", "mounton", "audit_access", \
- "open", "execmod"
+ "open", "execmod", "watch", "watch_mount", "watch_sb", \
+ "watch_with_perm", "watch_reads"
#define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
"listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
@@ -60,7 +61,7 @@ struct security_class_mapping secclass_map[] = {
{ "filesystem",
{ "mount", "remount", "unmount", "getattr",
"relabelfrom", "relabelto", "associate", "quotamod",
- "quotaget", NULL } },
+ "quotaget", "watch", NULL } },
{ "file",
{ COMMON_FILE_PERMS,
"execute_no_trans", "entrypoint", NULL } },
--
2.21.0
^ permalink raw reply related
* Re: Preferred subj= with multiple LSMs
From: Simon McVittie @ 2019-07-18 13:10 UTC (permalink / raw)
To: Casey Schaufler
Cc: Paul Moore, Steve Grubb, Richard Guy Briggs,
linux-audit@redhat.com, Linux Security Module list
In-Reply-To: <f375c23c-29e6-dc98-d71c-328db91117bc@schaufler-ca.com>
On Wed, 17 Jul 2019 at 16:02:16 -0700, Casey Schaufler wrote:
> We've never had to think about having general rules on
> what security modules do before, because with only one
> active each could do whatever it wanted without fear of
> conflict. If there is already a character that none of
> the existing modules use, how would it be wrong to
> reserve it?
>
> Smack disallows the four characters '"/\ because quoting
> is too important to ignore and the likelyhood that someone
> would confuse labels with paths seemed great. I sniffed
> around a little, but couldn't find the sets for SELinux or
> AppArmor.
It seems we've been here before, when I added LinuxSecurityLabel to
https://dbus.freedesktop.org/doc/dbus-specification.html#bus-messages-get-connection-credentials
in D-Bus.
Recapping the context for those who might have missed it: in D-Bus,
processes communicate in a hub-and-spoke topology via a central message
bus process, which forwards messages between the other processes. Some
other IPC systems would call this a broker. As a result of this
indirection, the message bus is the only process in the overall system
that is in a position to ask the kernel for the identity of the other
processes (credentials(7) and related topics like LSM labels) using
unforgeable kernel-guaranteed socket options like SO_PEERCRED, SO_PEERSEC
and SO_PEERGROUPS. This means that if two processes communicate via D-Bus
and want to know each other's identities, they have to ask the message
bus; so the message bus needs a representation for that information. For
LSM labels, that representation is LinuxSecurityLabel, which is defined
in terms of SO_PEERSEC.
At the time that I defined LinuxSecurityLabel, nobody was willing to
say for sure that the label was guaranteed to be ASCII or UTF-8 (which
is part of the specification for the D-Bus STRING ('s') type), so I
had to encode it as an arbitrary ARRAY of BYTE ('ay') rather than as
a STRING. I was at least told that the label wouldn't contain embedded
'\0', and that if there is a trailing '\0', I can safely canonicalize
the string by removing it.
Also, at the time that I did that, nobody was willing to say for sure
that there was any particular correspondence between the security
label obtained by reading /proc/self/attr/current and the security
label obtained by getting the SO_PEERSEC socket option: in AppArmor,
/proc/self/attr/current is something like "unconfined\n" whereas
SO_PEERSEC is either "unconfined" or "unconfined\0" (I forget which),
but the consensus seemed to be that there is no guarantee that the
presence or absence of a trailing newline wouldn't be significant to
some non-AppArmor LSM.
If LSM stacking is going to lead to syntactic restrictions being imposed
on security labels, please could someone add them to credentials(7)
or some other suitable documentation so user-space developers can know
where we stand, or tell me what the restrictions and guarantees are so
I can propose a documentation patch?
Thanks,
smcv
^ permalink raw reply
* [RFC/RFT v2 2/2] KEYS: trusted: Add generic trusted keys framework
From: Sumit Garg @ 2019-07-18 11:24 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-crypto, linux-security-module
Cc: dhowells, herbert, davem, jejb, jarkko.sakkinen, zohar, jmorris,
serge, casey, ard.biesheuvel, daniel.thompson, linux-kernel,
tee-dev, Sumit Garg
In-Reply-To: <1563449086-13183-1-git-send-email-sumit.garg@linaro.org>
Current trusted keys framework is tightly coupled to use TPM device as
an underlying implementation which makes it difficult for implementations
like Trusted Execution Environment (TEE) etc. to provide trusked keys
support in case platform doesn't posses a TPM device.
So this patch tries to add generic trusted keys framework where underlying
implemtations like TPM, TEE etc. could be easily plugged-in.
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
include/keys/trusted-type.h | 45 ++++
include/keys/trusted_tpm.h | 15 --
security/keys/trusted-keys/Makefile | 3 +-
security/keys/trusted-keys/trusted-tpm.c | 345 ++++++-------------------------
security/keys/trusted-keys/trusted.c | 343 ++++++++++++++++++++++++++++++
5 files changed, 448 insertions(+), 303 deletions(-)
create mode 100644 security/keys/trusted-keys/trusted.c
diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
index a94c03a..5559010 100644
--- a/include/keys/trusted-type.h
+++ b/include/keys/trusted-type.h
@@ -40,6 +40,51 @@ struct trusted_key_options {
uint32_t policyhandle;
};
+struct trusted_key_ops {
+ /*
+ * flag to indicate if trusted key implementation supports migration
+ * or not.
+ */
+ unsigned char migratable;
+
+ /* trusted key init */
+ int (*init)(void);
+
+ /* seal a trusted key */
+ int (*seal)(struct trusted_key_payload *p, char *datablob);
+
+ /* unseal a trusted key */
+ int (*unseal)(struct trusted_key_payload *p, char *datablob);
+
+ /* get random trusted key */
+ int (*get_random)(unsigned char *key, size_t key_len);
+
+ /* trusted key cleanup */
+ void (*cleanup)(void);
+};
+
extern struct key_type key_type_trusted;
+#if defined(CONFIG_TCG_TPM)
+extern struct trusted_key_ops tpm_trusted_key_ops;
+#endif
+
+#define TRUSTED_DEBUG 0
+
+#if TRUSTED_DEBUG
+static inline void dump_payload(struct trusted_key_payload *p)
+{
+ pr_info("trusted_key: key_len %d\n", p->key_len);
+ print_hex_dump(KERN_INFO, "key ", DUMP_PREFIX_NONE,
+ 16, 1, p->key, p->key_len, 0);
+ pr_info("trusted_key: bloblen %d\n", p->blob_len);
+ print_hex_dump(KERN_INFO, "blob ", DUMP_PREFIX_NONE,
+ 16, 1, p->blob, p->blob_len, 0);
+ pr_info("trusted_key: migratable %d\n", p->migratable);
+}
+#else
+static inline void dump_payload(struct trusted_key_payload *p)
+{
+}
+#endif
#endif /* _KEYS_TRUSTED_TYPE_H */
diff --git a/include/keys/trusted_tpm.h b/include/keys/trusted_tpm.h
index 7d7b108..dbbfe87 100644
--- a/include/keys/trusted_tpm.h
+++ b/include/keys/trusted_tpm.h
@@ -61,17 +61,6 @@ static inline void dump_options(struct trusted_key_options *o)
16, 1, o->pcrinfo, o->pcrinfo_len, 0);
}
-static inline void dump_payload(struct trusted_key_payload *p)
-{
- pr_info("trusted_key: key_len %d\n", p->key_len);
- print_hex_dump(KERN_INFO, "key ", DUMP_PREFIX_NONE,
- 16, 1, p->key, p->key_len, 0);
- pr_info("trusted_key: bloblen %d\n", p->blob_len);
- print_hex_dump(KERN_INFO, "blob ", DUMP_PREFIX_NONE,
- 16, 1, p->blob, p->blob_len, 0);
- pr_info("trusted_key: migratable %d\n", p->migratable);
-}
-
static inline void dump_sess(struct osapsess *s)
{
print_hex_dump(KERN_INFO, "trusted-key: handle ", DUMP_PREFIX_NONE,
@@ -97,10 +86,6 @@ static inline void dump_options(struct trusted_key_options *o)
{
}
-static inline void dump_payload(struct trusted_key_payload *p)
-{
-}
-
static inline void dump_sess(struct osapsess *s)
{
}
diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
index ad34d17..6ecadfb 100644
--- a/security/keys/trusted-keys/Makefile
+++ b/security/keys/trusted-keys/Makefile
@@ -3,4 +3,5 @@
# Makefile for trusted keys
#
-obj-$(CONFIG_TRUSTED_KEYS) += trusted-tpm.o
+obj-$(CONFIG_TRUSTED_KEYS) += trusted.o \
+ trusted-tpm.o
diff --git a/security/keys/trusted-keys/trusted-tpm.c b/security/keys/trusted-keys/trusted-tpm.c
index b7e53a3..bc15357 100644
--- a/security/keys/trusted-keys/trusted-tpm.c
+++ b/security/keys/trusted-keys/trusted-tpm.c
@@ -1,29 +1,26 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
* Copyright (C) 2010 IBM Corporation
+ * Copyright (c) 2019, Linaro Limited
*
* Author:
* David Safford <safford@us.ibm.com>
+ * Switch to generic trusted key framework: Sumit Garg <sumit.garg@linaro.org>
*
* See Documentation/security/keys/trusted-encrypted.rst
*/
#include <crypto/hash_info.h>
-#include <linux/uaccess.h>
-#include <linux/module.h>
#include <linux/init.h>
#include <linux/slab.h>
#include <linux/parser.h>
#include <linux/string.h>
#include <linux/err.h>
-#include <keys/user-type.h>
#include <keys/trusted-type.h>
#include <linux/key-type.h>
-#include <linux/rcupdate.h>
#include <linux/crypto.h>
#include <crypto/hash.h>
#include <crypto/sha.h>
-#include <linux/capability.h>
#include <linux/tpm.h>
#include <linux/tpm_command.h>
@@ -717,7 +714,6 @@ static int key_unseal(struct trusted_key_payload *p,
enum {
Opt_err,
- Opt_new, Opt_load, Opt_update,
Opt_keyhandle, Opt_keyauth, Opt_blobauth,
Opt_pcrinfo, Opt_pcrlock, Opt_migratable,
Opt_hash,
@@ -726,9 +722,6 @@ enum {
};
static const match_table_t key_tokens = {
- {Opt_new, "new"},
- {Opt_load, "load"},
- {Opt_update, "update"},
{Opt_keyhandle, "keyhandle=%s"},
{Opt_keyauth, "keyauth=%s"},
{Opt_blobauth, "blobauth=%s"},
@@ -855,71 +848,6 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
return 0;
}
-/*
- * datablob_parse - parse the keyctl data and fill in the
- * payload and options structures
- *
- * On success returns 0, otherwise -EINVAL.
- */
-static int datablob_parse(char *datablob, struct trusted_key_payload *p,
- struct trusted_key_options *o)
-{
- substring_t args[MAX_OPT_ARGS];
- long keylen;
- int ret = -EINVAL;
- int key_cmd;
- char *c;
-
- /* main command */
- c = strsep(&datablob, " \t");
- if (!c)
- return -EINVAL;
- key_cmd = match_token(c, key_tokens, args);
- switch (key_cmd) {
- case Opt_new:
- /* first argument is key size */
- c = strsep(&datablob, " \t");
- if (!c)
- return -EINVAL;
- ret = kstrtol(c, 10, &keylen);
- if (ret < 0 || keylen < MIN_KEY_SIZE || keylen > MAX_KEY_SIZE)
- return -EINVAL;
- p->key_len = keylen;
- ret = getoptions(datablob, p, o);
- if (ret < 0)
- return ret;
- ret = Opt_new;
- break;
- case Opt_load:
- /* first argument is sealed blob */
- c = strsep(&datablob, " \t");
- if (!c)
- return -EINVAL;
- p->blob_len = strlen(c) / 2;
- if (p->blob_len > MAX_BLOB_SIZE)
- return -EINVAL;
- ret = hex2bin(p->blob, c, p->blob_len);
- if (ret < 0)
- return -EINVAL;
- ret = getoptions(datablob, p, o);
- if (ret < 0)
- return ret;
- ret = Opt_load;
- break;
- case Opt_update:
- /* all arguments are options */
- ret = getoptions(datablob, p, o);
- if (ret < 0)
- return ret;
- ret = Opt_update;
- break;
- case Opt_err:
- return -EINVAL;
- break;
- }
- return ret;
-}
-
static struct trusted_key_options *trusted_options_alloc(void)
{
struct trusted_key_options *options;
@@ -940,258 +868,99 @@ static struct trusted_key_options *trusted_options_alloc(void)
return options;
}
-static struct trusted_key_payload *trusted_payload_alloc(struct key *key)
+static int tpm_tk_seal(struct trusted_key_payload *p, char *datablob)
{
- struct trusted_key_payload *p = NULL;
- int ret;
-
- ret = key_payload_reserve(key, sizeof *p);
- if (ret < 0)
- return p;
- p = kzalloc(sizeof *p, GFP_KERNEL);
- if (p)
- p->migratable = 1; /* migratable by default */
- return p;
-}
-
-/*
- * trusted_instantiate - create a new trusted key
- *
- * Unseal an existing trusted blob or, for a new key, get a
- * random key, then seal and create a trusted key-type key,
- * adding it to the specified keyring.
- *
- * On success, return 0. Otherwise return errno.
- */
-static int trusted_instantiate(struct key *key,
- struct key_preparsed_payload *prep)
-{
- struct trusted_key_payload *payload = NULL;
struct trusted_key_options *options = NULL;
- size_t datalen = prep->datalen;
- char *datablob;
int ret = 0;
- int key_cmd;
- size_t key_len;
int tpm2;
tpm2 = tpm_is_tpm2(chip);
if (tpm2 < 0)
return tpm2;
- if (datalen <= 0 || datalen > 32767 || !prep->data)
- return -EINVAL;
-
- datablob = kmalloc(datalen + 1, GFP_KERNEL);
- if (!datablob)
- return -ENOMEM;
- memcpy(datablob, prep->data, datalen);
- datablob[datalen] = '\0';
-
options = trusted_options_alloc();
- if (!options) {
- ret = -ENOMEM;
- goto out;
- }
- payload = trusted_payload_alloc(key);
- if (!payload) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!options)
+ return -ENOMEM;
- key_cmd = datablob_parse(datablob, payload, options);
- if (key_cmd < 0) {
- ret = key_cmd;
+ ret = getoptions(datablob, p, options);
+ if (ret < 0)
goto out;
- }
+ dump_options(options);
if (!options->keyhandle) {
ret = -EINVAL;
goto out;
}
- dump_payload(payload);
- dump_options(options);
+ if (tpm2)
+ ret = tpm_seal_trusted(chip, p, options);
+ else
+ ret = key_seal(p, options);
+ if (ret < 0) {
+ pr_info("tpm_trusted_key: key_seal failed (%d)\n", ret);
+ goto out;
+ }
- switch (key_cmd) {
- case Opt_load:
- if (tpm2)
- ret = tpm_unseal_trusted(chip, payload, options);
- else
- ret = key_unseal(payload, options);
- dump_payload(payload);
- dump_options(options);
- if (ret < 0)
- pr_info("trusted_key: key_unseal failed (%d)\n", ret);
- break;
- case Opt_new:
- key_len = payload->key_len;
- ret = tpm_get_random(chip, payload->key, key_len);
- if (ret != key_len) {
- pr_info("trusted_key: key_create failed (%d)\n", ret);
+ if (options->pcrlock) {
+ ret = pcrlock(options->pcrlock);
+ if (ret < 0) {
+ pr_info("tpm_trusted_key: pcrlock failed (%d)\n", ret);
goto out;
}
- if (tpm2)
- ret = tpm_seal_trusted(chip, payload, options);
- else
- ret = key_seal(payload, options);
- if (ret < 0)
- pr_info("trusted_key: key_seal failed (%d)\n", ret);
- break;
- default:
- ret = -EINVAL;
- goto out;
}
- if (!ret && options->pcrlock)
- ret = pcrlock(options->pcrlock);
out:
- kzfree(datablob);
kzfree(options);
- if (!ret)
- rcu_assign_keypointer(key, payload);
- else
- kzfree(payload);
return ret;
}
-static void trusted_rcu_free(struct rcu_head *rcu)
-{
- struct trusted_key_payload *p;
-
- p = container_of(rcu, struct trusted_key_payload, rcu);
- kzfree(p);
-}
-
-/*
- * trusted_update - reseal an existing key with new PCR values
- */
-static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
+static int tpm_tk_unseal(struct trusted_key_payload *p, char *datablob)
{
- struct trusted_key_payload *p;
- struct trusted_key_payload *new_p;
- struct trusted_key_options *new_o;
- size_t datalen = prep->datalen;
- char *datablob;
+ struct trusted_key_options *options = NULL;
int ret = 0;
+ int tpm2;
- if (key_is_negative(key))
- return -ENOKEY;
- p = key->payload.data[0];
- if (!p->migratable)
- return -EPERM;
- if (datalen <= 0 || datalen > 32767 || !prep->data)
- return -EINVAL;
+ tpm2 = tpm_is_tpm2(chip);
+ if (tpm2 < 0)
+ return tpm2;
- datablob = kmalloc(datalen + 1, GFP_KERNEL);
- if (!datablob)
+ options = trusted_options_alloc();
+ if (!options)
return -ENOMEM;
- new_o = trusted_options_alloc();
- if (!new_o) {
- ret = -ENOMEM;
- goto out;
- }
- new_p = trusted_payload_alloc(key);
- if (!new_p) {
- ret = -ENOMEM;
- goto out;
- }
- memcpy(datablob, prep->data, datalen);
- datablob[datalen] = '\0';
- ret = datablob_parse(datablob, new_p, new_o);
- if (ret != Opt_update) {
- ret = -EINVAL;
- kzfree(new_p);
+ ret = getoptions(datablob, p, options);
+ if (ret < 0)
goto out;
- }
+ dump_options(options);
- if (!new_o->keyhandle) {
+ if (!options->keyhandle) {
ret = -EINVAL;
- kzfree(new_p);
goto out;
}
- /* copy old key values, and reseal with new pcrs */
- new_p->migratable = p->migratable;
- new_p->key_len = p->key_len;
- memcpy(new_p->key, p->key, p->key_len);
- dump_payload(p);
- dump_payload(new_p);
+ if (tpm2)
+ ret = tpm_unseal_trusted(chip, p, options);
+ else
+ ret = key_unseal(p, options);
+ if (ret < 0)
+ pr_info("tpm_trusted_key: key_unseal failed (%d)\n", ret);
- ret = key_seal(new_p, new_o);
- if (ret < 0) {
- pr_info("trusted_key: key_seal failed (%d)\n", ret);
- kzfree(new_p);
- goto out;
- }
- if (new_o->pcrlock) {
- ret = pcrlock(new_o->pcrlock);
+ if (options->pcrlock) {
+ ret = pcrlock(options->pcrlock);
if (ret < 0) {
- pr_info("trusted_key: pcrlock failed (%d)\n", ret);
- kzfree(new_p);
+ pr_info("tpm_trusted_key: pcrlock failed (%d)\n", ret);
goto out;
}
}
- rcu_assign_keypointer(key, new_p);
- call_rcu(&p->rcu, trusted_rcu_free);
out:
- kzfree(datablob);
- kzfree(new_o);
+ kzfree(options);
return ret;
}
-/*
- * trusted_read - copy the sealed blob data to userspace in hex.
- * On success, return to userspace the trusted key datablob size.
- */
-static long trusted_read(const struct key *key, char __user *buffer,
- size_t buflen)
-{
- const struct trusted_key_payload *p;
- char *ascii_buf;
- char *bufp;
- int i;
-
- p = dereference_key_locked(key);
- if (!p)
- return -EINVAL;
-
- if (buffer && buflen >= 2 * p->blob_len) {
- ascii_buf = kmalloc_array(2, p->blob_len, GFP_KERNEL);
- if (!ascii_buf)
- return -ENOMEM;
-
- bufp = ascii_buf;
- for (i = 0; i < p->blob_len; i++)
- bufp = hex_byte_pack(bufp, p->blob[i]);
- if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) {
- kzfree(ascii_buf);
- return -EFAULT;
- }
- kzfree(ascii_buf);
- }
- return 2 * p->blob_len;
-}
-
-/*
- * trusted_destroy - clear and free the key's payload
- */
-static void trusted_destroy(struct key *key)
+int tpm_tk_get_random(unsigned char *key, size_t key_len)
{
- kzfree(key->payload.data[0]);
+ return tpm_get_random(chip, key, key_len);
}
-struct key_type key_type_trusted = {
- .name = "trusted",
- .instantiate = trusted_instantiate,
- .update = trusted_update,
- .destroy = trusted_destroy,
- .describe = user_describe,
- .read = trusted_read,
-};
-
-EXPORT_SYMBOL_GPL(key_type_trusted);
-
static void trusted_shash_release(void)
{
if (hashalg)
@@ -1206,14 +975,14 @@ static int __init trusted_shash_alloc(void)
hmacalg = crypto_alloc_shash(hmac_alg, 0, 0);
if (IS_ERR(hmacalg)) {
- pr_info("trusted_key: could not allocate crypto %s\n",
+ pr_info("tpm_trusted_key: could not allocate crypto %s\n",
hmac_alg);
return PTR_ERR(hmacalg);
}
hashalg = crypto_alloc_shash(hash_alg, 0, 0);
if (IS_ERR(hashalg)) {
- pr_info("trusted_key: could not allocate crypto %s\n",
+ pr_info("tpm_trusted_key: could not allocate crypto %s\n",
hash_alg);
ret = PTR_ERR(hashalg);
goto hashalg_fail;
@@ -1249,16 +1018,13 @@ static int __init init_digests(void)
return 0;
}
-static int __init init_trusted(void)
+static int __init init_tpm_trusted(void)
{
int ret;
- /* encrypted_keys.ko depends on successful load of this module even if
- * TPM is not used.
- */
chip = tpm_default_chip();
if (!chip)
- return 0;
+ return -ENODEV;
ret = init_digests();
if (ret < 0)
@@ -1279,7 +1045,7 @@ static int __init init_trusted(void)
return ret;
}
-static void __exit cleanup_trusted(void)
+static void __exit cleanup_tpm_trusted(void)
{
if (chip) {
put_device(&chip->dev);
@@ -1289,7 +1055,12 @@ static void __exit cleanup_trusted(void)
}
}
-late_initcall(init_trusted);
-module_exit(cleanup_trusted);
-
-MODULE_LICENSE("GPL");
+struct trusted_key_ops tpm_trusted_key_ops = {
+ .migratable = 1, /* migratable by default */
+ .init = init_tpm_trusted,
+ .seal = tpm_tk_seal,
+ .unseal = tpm_tk_unseal,
+ .get_random = tpm_tk_get_random,
+ .cleanup = cleanup_tpm_trusted,
+};
+EXPORT_SYMBOL_GPL(tpm_trusted_key_ops);
diff --git a/security/keys/trusted-keys/trusted.c b/security/keys/trusted-keys/trusted.c
new file mode 100644
index 0000000..8f00fde
--- /dev/null
+++ b/security/keys/trusted-keys/trusted.c
@@ -0,0 +1,343 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2010 IBM Corporation
+ * Copyright (c) 2019, Linaro Limited
+ *
+ * Author:
+ * David Safford <safford@us.ibm.com>
+ * Added generic trusted key framework: Sumit Garg <sumit.garg@linaro.org>
+ *
+ * See Documentation/security/keys/trusted-encrypted.rst
+ */
+
+#include <keys/user-type.h>
+#include <keys/trusted-type.h>
+#include <linux/capability.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/key-type.h>
+#include <linux/module.h>
+#include <linux/parser.h>
+#include <linux/rcupdate.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/uaccess.h>
+
+static struct trusted_key_ops *available_tk_ops[] = {
+#if defined(CONFIG_TCG_TPM)
+ &tpm_trusted_key_ops,
+#endif
+};
+static struct trusted_key_ops *tk_ops;
+
+enum {
+ Opt_err,
+ Opt_new, Opt_load, Opt_update,
+};
+
+static const match_table_t key_tokens = {
+ {Opt_new, "new"},
+ {Opt_load, "load"},
+ {Opt_update, "update"},
+ {Opt_err, NULL}
+};
+
+/*
+ * datablob_parse - parse the keyctl data and fill in the
+ * payload structure
+ *
+ * On success returns 0, otherwise -EINVAL.
+ */
+static int datablob_parse(char *datablob, struct trusted_key_payload *p)
+{
+ substring_t args[MAX_OPT_ARGS];
+ long keylen;
+ int ret = -EINVAL;
+ int key_cmd;
+ char *c;
+
+ /* main command */
+ c = strsep(&datablob, " \t");
+ if (!c)
+ return -EINVAL;
+ key_cmd = match_token(c, key_tokens, args);
+ switch (key_cmd) {
+ case Opt_new:
+ /* first argument is key size */
+ c = strsep(&datablob, " \t");
+ if (!c)
+ return -EINVAL;
+ ret = kstrtol(c, 10, &keylen);
+ if (ret < 0 || keylen < MIN_KEY_SIZE || keylen > MAX_KEY_SIZE)
+ return -EINVAL;
+ p->key_len = keylen;
+ ret = Opt_new;
+ break;
+ case Opt_load:
+ /* first argument is sealed blob */
+ c = strsep(&datablob, " \t");
+ if (!c)
+ return -EINVAL;
+ p->blob_len = strlen(c) / 2;
+ if (p->blob_len > MAX_BLOB_SIZE)
+ return -EINVAL;
+ ret = hex2bin(p->blob, c, p->blob_len);
+ if (ret < 0)
+ return -EINVAL;
+ ret = Opt_load;
+ break;
+ case Opt_update:
+ ret = Opt_update;
+ break;
+ case Opt_err:
+ return -EINVAL;
+ }
+ return ret;
+}
+
+static struct trusted_key_payload *trusted_payload_alloc(struct key *key)
+{
+ struct trusted_key_payload *p = NULL;
+ int ret;
+
+ ret = key_payload_reserve(key, sizeof(*p));
+ if (ret < 0)
+ return p;
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
+
+ p->migratable = tk_ops->migratable;
+
+ return p;
+}
+
+/*
+ * trusted_instantiate - create a new trusted key
+ *
+ * Unseal an existing trusted blob or, for a new key, get a
+ * random key, then seal and create a trusted key-type key,
+ * adding it to the specified keyring.
+ *
+ * On success, return 0. Otherwise return errno.
+ */
+static int trusted_instantiate(struct key *key,
+ struct key_preparsed_payload *prep)
+{
+ struct trusted_key_payload *payload = NULL;
+ size_t datalen = prep->datalen;
+ char *datablob;
+ int ret = 0;
+ int key_cmd;
+ size_t key_len;
+
+ if (datalen <= 0 || datalen > 32767 || !prep->data)
+ return -EINVAL;
+
+ datablob = kmalloc(datalen + 1, GFP_KERNEL);
+ if (!datablob)
+ return -ENOMEM;
+ memcpy(datablob, prep->data, datalen);
+ datablob[datalen] = '\0';
+
+ payload = trusted_payload_alloc(key);
+ if (!payload) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ key_cmd = datablob_parse(datablob, payload);
+ if (key_cmd < 0) {
+ ret = key_cmd;
+ goto out;
+ }
+
+ dump_payload(payload);
+
+ switch (key_cmd) {
+ case Opt_load:
+ ret = tk_ops->unseal(payload, datablob);
+ dump_payload(payload);
+ if (ret < 0)
+ pr_info("trusted_key: key_unseal failed (%d)\n", ret);
+ break;
+ case Opt_new:
+ key_len = payload->key_len;
+ ret = tk_ops->get_random(payload->key, key_len);
+ if (ret != key_len) {
+ pr_info("trusted_key: key_create failed (%d)\n", ret);
+ goto out;
+ }
+
+ ret = tk_ops->seal(payload, datablob);
+ if (ret < 0)
+ pr_info("trusted_key: key_seal failed (%d)\n", ret);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+out:
+ kzfree(datablob);
+ if (!ret)
+ rcu_assign_keypointer(key, payload);
+ else
+ kzfree(payload);
+ return ret;
+}
+
+static void trusted_rcu_free(struct rcu_head *rcu)
+{
+ struct trusted_key_payload *p;
+
+ p = container_of(rcu, struct trusted_key_payload, rcu);
+ kzfree(p);
+}
+
+/*
+ * trusted_update - reseal an existing key with new PCR values
+ */
+static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
+{
+ struct trusted_key_payload *p;
+ struct trusted_key_payload *new_p;
+ size_t datalen = prep->datalen;
+ char *datablob;
+ int ret = 0;
+
+ if (key_is_negative(key))
+ return -ENOKEY;
+ p = key->payload.data[0];
+ if (!p->migratable)
+ return -EPERM;
+ if (datalen <= 0 || datalen > 32767 || !prep->data)
+ return -EINVAL;
+
+ datablob = kmalloc(datalen + 1, GFP_KERNEL);
+ if (!datablob)
+ return -ENOMEM;
+
+ new_p = trusted_payload_alloc(key);
+ if (!new_p) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ memcpy(datablob, prep->data, datalen);
+ datablob[datalen] = '\0';
+ ret = datablob_parse(datablob, new_p);
+ if (ret != Opt_update) {
+ ret = -EINVAL;
+ kzfree(new_p);
+ goto out;
+ }
+
+ /* copy old key values, and reseal with new pcrs */
+ new_p->migratable = p->migratable;
+ new_p->key_len = p->key_len;
+ memcpy(new_p->key, p->key, p->key_len);
+ dump_payload(p);
+ dump_payload(new_p);
+
+ ret = tk_ops->seal(new_p, datablob);
+ if (ret < 0) {
+ pr_info("trusted_key: key_seal failed (%d)\n", ret);
+ kzfree(new_p);
+ goto out;
+ }
+
+ rcu_assign_keypointer(key, new_p);
+ call_rcu(&p->rcu, trusted_rcu_free);
+out:
+ kzfree(datablob);
+ return ret;
+}
+
+/*
+ * trusted_read - copy the sealed blob data to userspace in hex.
+ * On success, return to userspace the trusted key datablob size.
+ */
+static long trusted_read(const struct key *key, char __user *buffer,
+ size_t buflen)
+{
+ const struct trusted_key_payload *p;
+ char *ascii_buf;
+ char *bufp;
+ int i;
+
+ p = dereference_key_locked(key);
+ if (!p)
+ return -EINVAL;
+
+ if (buffer && buflen >= 2 * p->blob_len) {
+ ascii_buf = kmalloc_array(2, p->blob_len, GFP_KERNEL);
+ if (!ascii_buf)
+ return -ENOMEM;
+
+ bufp = ascii_buf;
+ for (i = 0; i < p->blob_len; i++)
+ bufp = hex_byte_pack(bufp, p->blob[i]);
+ if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) {
+ kzfree(ascii_buf);
+ return -EFAULT;
+ }
+ kzfree(ascii_buf);
+ }
+ return 2 * p->blob_len;
+}
+
+/*
+ * trusted_destroy - clear and free the key's payload
+ */
+static void trusted_destroy(struct key *key)
+{
+ kzfree(key->payload.data[0]);
+}
+
+struct key_type key_type_trusted = {
+ .name = "trusted",
+ .instantiate = trusted_instantiate,
+ .update = trusted_update,
+ .destroy = trusted_destroy,
+ .describe = user_describe,
+ .read = trusted_read,
+};
+EXPORT_SYMBOL_GPL(key_type_trusted);
+
+static int __init init_trusted(void)
+{
+ int i, ret = 0;
+
+ for (i = 0; i < sizeof(available_tk_ops); i++) {
+ tk_ops = available_tk_ops[i];
+
+ if (!(tk_ops && tk_ops->init && tk_ops->seal &&
+ tk_ops->unseal && tk_ops->get_random))
+ continue;
+
+ ret = tk_ops->init();
+ if (ret) {
+ if (tk_ops->cleanup)
+ tk_ops->cleanup();
+ } else {
+ break;
+ }
+ }
+
+ /*
+ * encrypted_keys.ko depends on successful load of this module even if
+ * trusted key implementation is not found.
+ */
+ if (ret == -ENODEV)
+ return 0;
+
+ return ret;
+}
+
+static void __exit cleanup_trusted(void)
+{
+ if (tk_ops->cleanup)
+ tk_ops->cleanup();
+}
+
+late_initcall(init_trusted);
+module_exit(cleanup_trusted);
+
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related
* [RFC/RFT v2 1/2] KEYS: trusted: create trusted keys subsystem
From: Sumit Garg @ 2019-07-18 11:24 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-crypto, linux-security-module
Cc: dhowells, herbert, davem, jejb, jarkko.sakkinen, zohar, jmorris,
serge, casey, ard.biesheuvel, daniel.thompson, linux-kernel,
tee-dev, Sumit Garg
In-Reply-To: <1563449086-13183-1-git-send-email-sumit.garg@linaro.org>
Move existing code to trusted keys subsystem. Also, rename files with
"tpm" as suffix which provides the underlying implementation.
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
crypto/asymmetric_keys/asym_tpm.c | 2 +-
include/keys/{trusted.h => trusted_tpm.h} | 4 ++--
security/keys/Makefile | 2 +-
security/keys/trusted-keys/Makefile | 6 ++++++
security/keys/{trusted.c => trusted-keys/trusted-tpm.c} | 2 +-
5 files changed, 11 insertions(+), 5 deletions(-)
rename include/keys/{trusted.h => trusted_tpm.h} (98%)
create mode 100644 security/keys/trusted-keys/Makefile
rename security/keys/{trusted.c => trusted-keys/trusted-tpm.c} (99%)
diff --git a/crypto/asymmetric_keys/asym_tpm.c b/crypto/asymmetric_keys/asym_tpm.c
index 76d2ce3..ec3f309 100644
--- a/crypto/asymmetric_keys/asym_tpm.c
+++ b/crypto/asymmetric_keys/asym_tpm.c
@@ -13,7 +13,7 @@
#include <crypto/sha.h>
#include <asm/unaligned.h>
#include <keys/asymmetric-subtype.h>
-#include <keys/trusted.h>
+#include <keys/trusted_tpm.h>
#include <crypto/asym_tpm_subtype.h>
#include <crypto/public_key.h>
diff --git a/include/keys/trusted.h b/include/keys/trusted_tpm.h
similarity index 98%
rename from include/keys/trusted.h
rename to include/keys/trusted_tpm.h
index 0071298..7d7b108 100644
--- a/include/keys/trusted.h
+++ b/include/keys/trusted_tpm.h
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __TRUSTED_KEY_H
-#define __TRUSTED_KEY_H
+#ifndef __TRUSTED_TPM_H
+#define __TRUSTED_TPM_H
/* implementation specific TPM constants */
#define MAX_BUF_SIZE 1024
diff --git a/security/keys/Makefile b/security/keys/Makefile
index 9cef540..074f275 100644
--- a/security/keys/Makefile
+++ b/security/keys/Makefile
@@ -28,5 +28,5 @@ obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += keyctl_pkey.o
# Key types
#
obj-$(CONFIG_BIG_KEYS) += big_key.o
-obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
+obj-$(CONFIG_TRUSTED_KEYS) += trusted-keys/
obj-$(CONFIG_ENCRYPTED_KEYS) += encrypted-keys/
diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
new file mode 100644
index 0000000..ad34d17
--- /dev/null
+++ b/security/keys/trusted-keys/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for trusted keys
+#
+
+obj-$(CONFIG_TRUSTED_KEYS) += trusted-tpm.o
diff --git a/security/keys/trusted.c b/security/keys/trusted-keys/trusted-tpm.c
similarity index 99%
rename from security/keys/trusted.c
rename to security/keys/trusted-keys/trusted-tpm.c
index 9a94672..b7e53a3 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted-keys/trusted-tpm.c
@@ -27,7 +27,7 @@
#include <linux/tpm.h>
#include <linux/tpm_command.h>
-#include <keys/trusted.h>
+#include <keys/trusted_tpm.h>
static const char hmac_alg[] = "hmac(sha1)";
static const char hash_alg[] = "sha1";
--
2.7.4
^ permalink raw reply related
* [RFC/RFT v2 0/2] KEYS: trusted: Add generic trusted keys framework
From: Sumit Garg @ 2019-07-18 11:24 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-crypto, linux-security-module
Cc: dhowells, herbert, davem, jejb, jarkko.sakkinen, zohar, jmorris,
serge, casey, ard.biesheuvel, daniel.thompson, linux-kernel,
tee-dev, Sumit Garg
This patch-set is an outcome of discussion here [1].
I have tested this framework with trusted keys support provided via TEE
but I wasn't able to test it with a TPM device as I don't possess one. It
would be really helpful if others could test this patch-set using a TPM
device.
[1] https://www.mail-archive.com/linux-doc@vger.kernel.org/msg30591.html
Changes in v2:
Split trusted keys abstraction patch for ease of review.
Sumit Garg (2):
KEYS: trusted: create trusted keys subsystem
KEYS: trusted: Add generic trusted keys framework
crypto/asymmetric_keys/asym_tpm.c | 2 +-
include/keys/trusted-type.h | 45 +++
include/keys/{trusted.h => trusted_tpm.h} | 19 +-
security/keys/Makefile | 2 +-
security/keys/trusted-keys/Makefile | 7 +
.../keys/{trusted.c => trusted-keys/trusted-tpm.c} | 347 ++++-----------------
security/keys/trusted-keys/trusted.c | 343 ++++++++++++++++++++
7 files changed, 458 insertions(+), 307 deletions(-)
rename include/keys/{trusted.h => trusted_tpm.h} (85%)
create mode 100644 security/keys/trusted-keys/Makefile
rename security/keys/{trusted.c => trusted-keys/trusted-tpm.c} (77%)
create mode 100644 security/keys/trusted-keys/trusted.c
--
2.7.4
^ permalink raw reply
* Re: [RFC PATCH v7 1/1] Add dm verity root hash pkcs7 sig validation.
From: Randy Dunlap @ 2019-07-18 1:04 UTC (permalink / raw)
To: Jaskaran Khurana, gmazyland
Cc: linux-security-module, linux-kernel, linux-integrity,
linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh, mdsakib,
mpatocka, ebiggers
In-Reply-To: <20190718004615.16818-2-jaskarankhurana@linux.microsoft.com>
Hi,
Just a couple of minor nits:
On 7/17/19 5:46 PM, Jaskaran Khurana wrote:
> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
> index 3834332f4963..c2b04d226c90 100644
> --- a/drivers/md/Kconfig
> +++ b/drivers/md/Kconfig
> @@ -490,6 +490,18 @@ config DM_VERITY
>
> If unsure, say N.
>
> +config DM_VERITY_VERIFY_ROOTHASH_SIG
> + def_bool n
It already defaults to n, so we usually try to omit that (don't repeat it).
> + bool "Verity data device root hash signature verification support"
> + depends on DM_VERITY
> + select SYSTEM_DATA_VERIFICATION
> + help
"help" should only be indented by one tab (and not the extra 2 spaces).
> + The device mapper target created by DM-VERITY can be validated if the
> + pre-generated tree of cryptographic checksums passed has a pkcs#7
> + signature file that can validate the roothash of the tree.
> +
> + If unsure, say N.
> +
> config DM_VERITY_FEC
> bool "Verity forward error correction support"
> depends on DM_VERITY
thanks.
--
~Randy
^ permalink raw reply
* [RFC PATCH v7 1/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Khurana @ 2019-07-18 0:46 UTC (permalink / raw)
To: gmazyland
Cc: linux-security-module, linux-kernel, linux-integrity,
linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh, mdsakib,
mpatocka, ebiggers
In-Reply-To: <20190718004615.16818-1-jaskarankhurana@linux.microsoft.com>
The verification is to support cases where the roothash is not secured by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after boot,
the root hash provided during the creation of the dm-verity volume has to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.
The signature being provided for verification must verify the root hash and
must be trusted by the builtin keyring for verification to succeed.
The hash is added as a key of type "user" and the description is passed to the
kernel so it can look it up and use it for verification.
Adds CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG which can be turned on if root hash
verification is needed.
Kernel commandline parameter require_signatures will indicate whether to force
(for all dm verity volumes) roothash signature verification.
Signed-off-by: Jaskaran Khurana <jaskarankhurana@linux.microsoft.com>
Tested-and-Reviewed-by: Milan Broz <gmazyland@gmail.com>
---
Documentation/admin-guide/device-mapper/verity.rst | 7 ++
drivers/md/Kconfig | 12 ++
drivers/md/Makefile | 4 +
drivers/md/dm-verity-target.c | 43 ++++++-
drivers/md/dm-verity-verify-sig.c | 133 +++++++++++++++++++++
drivers/md/dm-verity-verify-sig.h | 60 ++++++++++
drivers/md/dm-verity.h | 2 +
7 files changed, 256 insertions(+), 5 deletions(-)
create mode 100644 drivers/md/dm-verity-verify-sig.c
create mode 100644 drivers/md/dm-verity-verify-sig.h
diff --git a/Documentation/admin-guide/device-mapper/verity.rst b/Documentation/admin-guide/device-mapper/verity.rst
index a4d1c1476d72..bb02caa45289 100644
--- a/Documentation/admin-guide/device-mapper/verity.rst
+++ b/Documentation/admin-guide/device-mapper/verity.rst
@@ -125,6 +125,13 @@ check_at_most_once
blocks, and a hash block will not be verified any more after all the data
blocks it covers have been verified anyway.
+root_hash_sig_key_desc <key_description>
+ This is the description of the USER_KEY that the kernel will lookup to get
+ the pkcs7 signature of the roothash. The pkcs7 signature is used to validate
+ the root hash during the creation of the device mapper block device.
+ Verification of roothash depends on the config DM_VERITY_VERIFY_ROOTHASH_SIG
+ being set in the kernel.
+
Theory of operation
===================
diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index 3834332f4963..c2b04d226c90 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -490,6 +490,18 @@ config DM_VERITY
If unsure, say N.
+config DM_VERITY_VERIFY_ROOTHASH_SIG
+ def_bool n
+ bool "Verity data device root hash signature verification support"
+ depends on DM_VERITY
+ select SYSTEM_DATA_VERIFICATION
+ help
+ The device mapper target created by DM-VERITY can be validated if the
+ pre-generated tree of cryptographic checksums passed has a pkcs#7
+ signature file that can validate the roothash of the tree.
+
+ If unsure, say N.
+
config DM_VERITY_FEC
bool "Verity forward error correction support"
depends on DM_VERITY
diff --git a/drivers/md/Makefile b/drivers/md/Makefile
index be7a6eb92abc..0d922335335c 100644
--- a/drivers/md/Makefile
+++ b/drivers/md/Makefile
@@ -81,3 +81,7 @@ endif
ifeq ($(CONFIG_DM_VERITY_FEC),y)
dm-verity-objs += dm-verity-fec.o
endif
+
+ifeq ($(CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG),y)
+dm-verity-objs += dm-verity-verify-sig.o
+endif
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index ea24ff0612e3..4fb33e7562c5 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -15,7 +15,7 @@
#include "dm-verity.h"
#include "dm-verity-fec.h"
-
+#include "dm-verity-verify-sig.h"
#include <linux/module.h>
#include <linux/reboot.h>
@@ -33,7 +33,8 @@
#define DM_VERITY_OPT_IGN_ZEROES "ignore_zero_blocks"
#define DM_VERITY_OPT_AT_MOST_ONCE "check_at_most_once"
-#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC)
+#define DM_VERITY_OPTS_MAX (2 + DM_VERITY_OPTS_FEC + \
+ DM_VERITY_ROOT_HASH_VERIFICATION_OPTS)
static unsigned dm_verity_prefetch_cluster = DM_VERITY_DEFAULT_PREFETCH_SIZE;
@@ -713,6 +714,8 @@ static void verity_status(struct dm_target *ti, status_type_t type,
args++;
if (v->validated_blocks)
args++;
+ if (v->signature_key_desc)
+ args += DM_VERITY_ROOT_HASH_VERIFICATION_OPTS;
if (!args)
return;
DMEMIT(" %u", args);
@@ -734,6 +737,9 @@ static void verity_status(struct dm_target *ti, status_type_t type,
if (v->validated_blocks)
DMEMIT(" " DM_VERITY_OPT_AT_MOST_ONCE);
sz = verity_fec_status_table(v, sz, result, maxlen);
+ if (v->signature_key_desc)
+ DMEMIT(" " DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY
+ " %s", v->signature_key_desc);
break;
}
}
@@ -799,6 +805,8 @@ static void verity_dtr(struct dm_target *ti)
verity_fec_dtr(v);
+ kfree(v->signature_key_desc);
+
kfree(v);
}
@@ -854,7 +862,8 @@ out:
return r;
}
-static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
+static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+ struct dm_verity_sig_opts *verify_args)
{
int r;
unsigned argc;
@@ -903,6 +912,14 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
if (r)
return r;
continue;
+ } else if (verity_verify_is_sig_opt_arg(arg_name)) {
+ r = verity_verify_sig_parse_opt_args(as, v,
+ verify_args,
+ &argc, arg_name);
+ if (r)
+ return r;
+ continue;
+
}
ti->error = "Unrecognized verity feature request";
@@ -929,6 +946,7 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
{
struct dm_verity *v;
+ struct dm_verity_sig_opts verify_args = {0};
struct dm_arg_set as;
unsigned int num;
unsigned long long num_ll;
@@ -936,6 +954,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
int i;
sector_t hash_position;
char dummy;
+ char *root_hash_digest_to_validate;
v = kzalloc(sizeof(struct dm_verity), GFP_KERNEL);
if (!v) {
@@ -1069,6 +1088,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
r = -EINVAL;
goto bad;
}
+ root_hash_digest_to_validate = argv[8];
if (strcmp(argv[9], "-")) {
v->salt_size = strlen(argv[9]) / 2;
@@ -1094,11 +1114,20 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
as.argc = argc;
as.argv = argv;
- r = verity_parse_opt_args(&as, v);
+ r = verity_parse_opt_args(&as, v, &verify_args);
if (r < 0)
goto bad;
}
+ /* Root hash signature is a optional parameter*/
+ r = verity_verify_root_hash(root_hash_digest_to_validate,
+ strlen(root_hash_digest_to_validate),
+ verify_args.sig,
+ verify_args.sig_size);
+ if (r < 0) {
+ ti->error = "Root hash verification failed";
+ goto bad;
+ }
v->hash_per_block_bits =
__fls((1 << v->hash_dev_block_bits) / v->digest_size);
@@ -1164,9 +1193,13 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
ti->per_io_data_size = roundup(ti->per_io_data_size,
__alignof__(struct dm_verity_io));
+ verity_verify_sig_opts_cleanup(&verify_args);
+
return 0;
bad:
+
+ verity_verify_sig_opts_cleanup(&verify_args);
verity_dtr(ti);
return r;
@@ -1174,7 +1207,7 @@ bad:
static struct target_type verity_target = {
.name = "verity",
- .version = {1, 4, 0},
+ .version = {1, 5, 0},
.module = THIS_MODULE,
.ctr = verity_ctr,
.dtr = verity_dtr,
diff --git a/drivers/md/dm-verity-verify-sig.c b/drivers/md/dm-verity-verify-sig.c
new file mode 100644
index 000000000000..614e43db93aa
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author: Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#include <linux/device-mapper.h>
+#include <linux/verification.h>
+#include <keys/user-type.h>
+#include <linux/module.h>
+#include "dm-verity.h"
+#include "dm-verity-verify-sig.h"
+
+#define DM_VERITY_VERIFY_ERR(s) DM_VERITY_ROOT_HASH_VERIFICATION " " s
+
+static bool require_signatures;
+module_param(require_signatures, bool, false);
+MODULE_PARM_DESC(require_signatures,
+ "Verify the roothash of dm-verity hash tree");
+
+#define DM_VERITY_IS_SIG_FORCE_ENABLED() \
+ (require_signatures != false)
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name)
+{
+ return (!strcasecmp(arg_name,
+ DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY));
+}
+
+static int verity_verify_get_sig_from_key(const char *key_desc,
+ struct dm_verity_sig_opts *sig_opts)
+{
+ struct key *key;
+ const struct user_key_payload *ukp;
+ int ret = 0;
+
+ key = request_key(&key_type_user,
+ key_desc, NULL);
+ if (IS_ERR(key))
+ return PTR_ERR(key);
+
+ down_read(&key->sem);
+
+ ukp = user_key_payload_locked(key);
+ if (!ukp) {
+ ret = -EKEYREVOKED;
+ goto end;
+ }
+
+ sig_opts->sig = kmalloc(ukp->datalen, GFP_KERNEL);
+ if (!sig_opts->sig) {
+ ret = -ENOMEM;
+ goto end;
+ }
+ sig_opts->sig_size = ukp->datalen;
+
+ memcpy(sig_opts->sig, ukp->data, sig_opts->sig_size);
+
+end:
+ up_read(&key->sem);
+ key_put(key);
+
+ return ret;
+}
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as,
+ struct dm_verity *v,
+ struct dm_verity_sig_opts *sig_opts,
+ unsigned int *argc,
+ const char *arg_name)
+{
+ struct dm_target *ti = v->ti;
+ int ret = 0;
+ const char *sig_key = NULL;
+
+ if (!*argc) {
+ ti->error = DM_VERITY_VERIFY_ERR("Signature key not specified");
+ return -EINVAL;
+ }
+
+ sig_key = dm_shift_arg(as);
+ (*argc)--;
+
+ ret = verity_verify_get_sig_from_key(sig_key, sig_opts);
+ if (ret < 0)
+ ti->error = DM_VERITY_VERIFY_ERR("Invalid key specified");
+
+ v->signature_key_desc = kstrdup(sig_key, GFP_KERNEL);
+ if (!v->signature_key_desc)
+ return -ENOMEM;
+
+ return ret;
+}
+
+/*
+ * verify_verify_roothash - Verify the root hash of the verity hash device
+ * using builtin trusted keys.
+ *
+ * @root_hash: For verity, the roothash/data to be verified.
+ * @root_hash_len: Size of the roothash/data to be verified.
+ * @sig_data: The trusted signature that verifies the roothash/data.
+ * @sig_len: Size of the signature.
+ *
+ */
+int verity_verify_root_hash(const void *root_hash, size_t root_hash_len,
+ const void *sig_data, size_t sig_len)
+{
+ int ret;
+
+ if (!root_hash || root_hash_len == 0)
+ return -EINVAL;
+
+ if (!sig_data || sig_len == 0) {
+ if (DM_VERITY_IS_SIG_FORCE_ENABLED())
+ return -ENOKEY;
+ else
+ return 0;
+ }
+
+ ret = verify_pkcs7_signature(root_hash, root_hash_len, sig_data,
+ sig_len, NULL, VERIFYING_UNSPECIFIED_SIGNATURE,
+ NULL, NULL);
+
+ return ret;
+}
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts)
+{
+ kfree(sig_opts->sig);
+ sig_opts->sig = NULL;
+ sig_opts->sig_size = 0;
+}
diff --git a/drivers/md/dm-verity-verify-sig.h b/drivers/md/dm-verity-verify-sig.h
new file mode 100644
index 000000000000..19b1547aa741
--- /dev/null
+++ b/drivers/md/dm-verity-verify-sig.h
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Microsoft Corporation.
+ *
+ * Author: Jaskaran Singh Khurana <jaskarankhurana@linux.microsoft.com>
+ *
+ */
+#ifndef DM_VERITY_SIG_VERIFICATION_H
+#define DM_VERITY_SIG_VERIFICATION_H
+
+#define DM_VERITY_ROOT_HASH_VERIFICATION "DM Verity Sig Verification"
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPT_SIG_KEY "root_hash_sig_key_desc"
+
+struct dm_verity_sig_opts {
+ unsigned int sig_size;
+ u8 *sig;
+};
+
+#ifdef CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG
+
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPTS 2
+
+int verity_verify_root_hash(const void *data, size_t data_len,
+ const void *sig_data, size_t sig_len);
+bool verity_verify_is_sig_opt_arg(const char *arg_name);
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+ struct dm_verity_sig_opts *sig_opts,
+ unsigned int *argc, const char *arg_name);
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts);
+
+#else
+
+#define DM_VERITY_ROOT_HASH_VERIFICATION_OPTS 0
+
+int verity_verify_root_hash(const void *data, size_t data_len,
+ const void *sig_data, size_t sig_len)
+{
+ return 0;
+}
+
+bool verity_verify_is_sig_opt_arg(const char *arg_name)
+{
+ return false;
+}
+
+int verity_verify_sig_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
+ struct dm_verity_sig_opts *sig_opts,
+ unsigned int *argc, const char *arg_name)
+{
+ return -EINVAL;
+}
+
+void verity_verify_sig_opts_cleanup(struct dm_verity_sig_opts *sig_opts)
+{
+}
+
+#endif /* CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG */
+#endif /* DM_VERITY_SIG_VERIFICATION_H */
diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
index eeaf940aef6d..641b9e3a399b 100644
--- a/drivers/md/dm-verity.h
+++ b/drivers/md/dm-verity.h
@@ -63,6 +63,8 @@ struct dm_verity {
struct dm_verity_fec *fec; /* forward error correction */
unsigned long *validated_blocks; /* bitset blocks validated */
+
+ char *signature_key_desc; /* signature keyring reference */
};
struct dm_verity_io {
--
cgit 1.2-0.3.lf.el7
^ permalink raw reply related
* [RFC PATCH v7 0/1] Add dm verity root hash pkcs7 sig validation.
From: Jaskaran Khurana @ 2019-07-18 0:46 UTC (permalink / raw)
To: gmazyland
Cc: linux-security-module, linux-kernel, linux-integrity,
linux-fsdevel, agk, snitzer, dm-devel, jmorris, scottsh, mdsakib,
mpatocka, ebiggers
This patch set adds in-kernel pkcs7 signature checking for the roothash
of
the dm-verity hash tree.
The verification is to support cases where the roothash is not secured
by
Trusted Boot, UEFI Secureboot or similar technologies.
One of the use cases for this is for dm-verity volumes mounted after
boot,
the root hash provided during the creation of the dm-verity volume has
to
be secure and thus in-kernel validation implemented here will be used
before we trust the root hash and allow the block device to be created.
Why we are doing validation in the Kernel?
------------------------------------------
The reason is to still be secure in cases where the attacker is able to
compromise the user mode application in which case the user mode
validation
could not have been trusted.
The root hash signature validation in the kernel along with existing
dm-verity implementation gives a higher level of confidence in the
executable code or the protected data. Before allowing the creation of
the device mapper block device the kernel code will check that the
detached
pkcs7 signature passed to it validates the roothash and the signature is
trusted by builtin keys set at kernel creation. The kernel should be
secured using Verified boot, UEFI Secure Boot or similar technologies so
we
can trust it.
What about attacker mounting non dm-verity volumes to run executable code?
--------------------------------------------------------------------------
This verification can be used to have a security architecture where a
LSM
can enforce this verification for all the volumes and by doing this it
can
ensure that all executable code runs from signed and trusted dm-verity
volumes.
Further patches will be posted that build on this and enforce this
verification based on policy for all the volumes on the system.
Kernel commandline parameter require_signatures will indicate whether to
force (for all dm verity volumes) roothash signature verification.
How are these changes tested?
-----------------------------
Build time steps:
----------------
CONFIG_DM_VERITY_VERIFY_ROOTHASH_SIG needs to be turned on in .config.
Add the certificate(pubic key) that will be used to check whether to trust
the signature of the roothash in a PEM-encoded file to the
CONFIG_SYSTEM_TRUSTED_KEYS (example step 1) and step 2) below).
When formatting the fs for verity we need to save and sign the roothash, the
signature would be used in verification by the kernel later when we create dm
verity block device on the test machine/kernel.
Dump the roothash returned by veritysetup format in a text file,
say roothash.txt and then sign using the openssl command (see below).
1) openssl req -x509 -newkey rsa:1024 -keyout ca_key.pem -out ca.pem -nodes
-days 365 -set_serial 01 -subj /CN=example.com
2) In .config add the certificate as CONFIG_SYSTEM_TRUSTED_KEYS="path/ca.pem"
3) veritysetup format <fs> <hashdev>, this will return the ROOT_HASH.
4) Use the roothash returned in step 3) and save it to a file for reference and
signing.
echo -n <ROOT_HASH> > roothash.txt
5) openssl smime -sign -nocerts -noattr -binary -in <roothash.txt>
-inkey ca_key.pem -signer ca.pem -outform der -out sign.txt
Kernel Commandline:
-------------------
To enforce the signatures for all dm verity volumes specify the
dm_verity.require_signatures=1 on the kernel commandline.
Steps on the test kernel/machine:
---------------------------------
After the kernel boots try to create device mapper block device by providing
the roothash and the roothash signature which will be validated by the kernel.
To pass the roothash signature to dm-verity, veritysetup part of
cryptsetup library was modified to take a optional root-hash-sig parameter.
Use the signature file from above step as a parameter to veritysetup.
The changes for veritysetup are in a topic branch for now at:
https://github.com/jaskarankhurana/veritysetup/tree/veritysetup_add_sig
veritysetup open <data_device> <name> <hash_device> <root_hash>
--root-hash-sig=<root_hash_pkcs7_detached_sig>
OR
--
We could also use a unpatched veritysetup to test this.
Steps are shown as an example below :
(The roothash and signature will be obtained from steps 1 to 5 above)
eg:
NAME=test
DEV=/dev/sdc
DEV_HASH=/dev/sdd
ROOT_HASH=778fccab393842688c9af89cfd0c5cde69377cbe21ed439109ec856f2aa8a423
SIGN_NAME=verity:$NAME
SIGN=sign.txt
# load signature to keyring
keyctl padd user $SIGN_NAME @u <$SIGN
# add device-mapper table, now with sighed root hash optional argument
dmsetup create -r $NAME --table "$TABLE 2 root_hash_sig_key_desc $SIGN_NAME"
dmsetup table $NAME
# cleanup
dmsetup remove $NAME
keyctl clear @u
Changelog:
---------
v7
- Changes to patch header to add steps that can help test this.
- Use the rebased version of the patch that was tested by Milan Broz from his
tree.
v6
- Address comments from Milan Broz and Eric Biggers on v5.
- Keep the verification code under config DM_VERITY_VERIFY_ROOTHASH_SIG.
- Change the command line parameter to requires_signatures(bool) which will
force root hash to be signed and trusted if specified.
- Fix the signature not being present in verity_status. Merged the
https://git.kernel.org/pub/scm/linux/kernel/git/mbroz/linux.git/commit/?h=dm-cryptsetup&id=a26c10806f5257e255b6a436713127e762935ad3
made by Milan Broz and tested it.
v5 (since previous):
- Code review feedback given by Milan Broz.
- Remove the Kconfig for root hash verification and instead add a
commandline parameter(dm_verity.verify_sig) that determines whether
to
check or enforce root hash signature validation.
- Fixed a small issue when dm-verity was built sepaerately as a
module.
- Added the openssl commandline that can be used to sign the roothash
in the cover letter.
v4:
- Code review feedback given by Milan Broz.
- Add documentation about the root hash signature parameter.
- Bump up the dm-verity target version.
- Provided way to sign and test with veritysetup in cover letter.
v3:
- Code review feedback given by Sasha Levin.
- Removed EXPORT_SYMBOL_GPL since this was not required.
- Removed "This file is released under the GPLv2" since we have SPDX
identifier.
- Inside verity_verify_root_hash changed EINVAL to ENOKEY when the key
descriptor is not specified but due to force option being set it is
expected.
- Moved CONFIG check to inside verity_verify_get_sig_from_key.
(Did not move the sig_opts_cleanup to inside verity_dtr as the
sig_opts do not need to be allocated for the entire duration the
block
device is active unlike the verity structure, note verity_dtr is
called only if verity_ctr fails or after the lifetime of the
block device.)
v2:
- Code review feedback to pass the signature binary blob as a key that
can be looked up in the kernel and be used to verify the roothash.
[Suggested by Milan Broz]
- Made the code related change suggested in review of v1.
[Suggested by Balbir Singh]
v1:
- Add kconfigs to control dm-verity root has signature verification
and
use the signature if specified to verify the root hash.
Jaskaran Khurana (1):
Add dm verity root hash pkcs7 sig validation.
.../admin-guide/device-mapper/verity.rst | 7 +
drivers/md/Kconfig | 12 ++
drivers/md/Makefile | 5 +
drivers/md/dm-verity-target.c | 43 +++++-
drivers/md/dm-verity-verify-sig.c | 133 ++++++++++++++++++
drivers/md/dm-verity-verify-sig.h | 60 ++++++++
drivers/md/dm-verity.h | 2 +
7 files changed, 257 insertions(+), 5 deletions(-)
create mode 100644 drivers/md/dm-verity-verify-sig.c
create mode 100644 drivers/md/dm-verity-verify-sig.h
--
2.17.1
^ permalink raw reply
* Re: Preferred subj= with multiple LSMs
From: Casey Schaufler @ 2019-07-17 23:02 UTC (permalink / raw)
To: Paul Moore
Cc: Steve Grubb, Richard Guy Briggs, linux-audit@redhat.com,
Linux Security Module list, casey
In-Reply-To: <CAHC9VhQS9We1TNqRfuR_E-kV4aZddx9euaiv5Gzd5B5AkiDAUQ@mail.gmail.com>
On 7/17/2019 9:23 AM, Paul Moore wrote:
> On Wed, Jul 17, 2019 at 11:49 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 7/17/2019 5:14 AM, Paul Moore wrote:
>>> On Tue, Jul 16, 2019 at 7:47 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 7/16/2019 4:13 PM, Paul Moore wrote:
>>>>> On Tue, Jul 16, 2019 at 6:18 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>> It sounds as if some variant of the Hideous format:
>>>>>>
>>>>>> subj=selinux='a:b:c:d',apparmor='z'
>>>>>> subj=selinux/a:b:c:d/apparmor/z
>>>>>> subj=(selinux)a:b:c:d/(apparmor)z
>>>>>>
>>>>>> would meet Steve's searchability requirements, but with significant
>>>>>> parsing performance penalties.
>>>>> I think "hideous format" sums it up nicely. Whatever we choose here
>>>>> we are likely going to be stuck with for some time and I'm near to
>>>>> 100% that multiplexing the labels onto a single field is going to be a
>>>>> disaster.
>>>> If the requirement is that subj= be searchable I don't see much of
>>>> an alternative to a Hideous format. If we can get past that, and say
>>>> that all subj_* have to be searchable we can avoid that set of issues.
>>>> Instead of:
>>>>
>>>> s = strstr(source, "subj=")
>>>> search_after_subj(s, ...);
>>> This example does a lot of hand waving in search_after_subj(...)
>>> regarding parsing the multiplexed LSM label. Unless we restrict the
>>> LSM label formats (which seems both wrong, and too late IMHO)
>> I don't think it's too late, and I think it would be healthy
>> to restrict LSM "contexts" to character sets that make command
>> line specification possible. Embedded newlines? Ewwww.
> That would imply that the delimiter you would choose for the
> multiplexed approach would be something odd (I think you suggested
> 0x02, or similar, earlier) which would likely require the multiplexed
> subj field to become a hex encoded field which would be very
> unfortunate in my opinion and would technically break with the current
> subj/obj field format spec. Picking a normal-ish delimiter, and
> restricting its use by LSMs seems wrong to me.
Just say "no" to hex encoding! BTW, keys are not hex encoded.
We've never had to think about having general rules on
what security modules do before, because with only one
active each could do whatever it wanted without fear of
conflict. If there is already a character that none of
the existing modules use, how would it be wrong to
reserve it?
Smack disallows the four characters '"/\ because quoting
is too important to ignore and the likelyhood that someone
would confuse labels with paths seemed great. I sniffed
around a little, but couldn't find the sets for SELinux or
AppArmor.
> It's also worth noting that if you were to move subj/obj to hex
> encoded fields, in addition to causing a backwards compatibility
> problem, you completely kill the ability to look at the raw log data
> and make sense of the fields ... well, unless you can do the ascii hex
> conversion in your head on the fly.
Agreed, even though there was a time when I could do
hex decoding in both ASCII and EBCDIC on the fly.
>>> we have
>>> a parsing nightmare; can you write a safe multiplexed LSM label parser
>>> without knowledge of each LSM label format? Can you do that for each
>>> LSM without knowing their loaded policy? What happens when the policy
>>> and/or label format changes? What happens in a few years when another
>>> LSM is added to the kernel?
>> I was intentionally hand-wavy because of those very issues.
> Then you should already realize why this is a terrible idea ;)
Unfortunately, I'm facing two options, one of which the
kernel maintainer thinks is a bad idea and the other the
user space maintainer thinks is a bad idea. Plus, I'm not
very happy with either, either.
>> Steve says that parsing is limited to "strstr()", so looking for
>> ":s7:" in the subject should work just as well with a Hideous
>> format as it does today, with the exception of false positives
>> where LSMs have label string overlaps.
> Today when you go to search through your audit log you know that a
> single LSM is providing subj labels, and you also know which LSM that
> happens to be, so searching on a given string, or substring, is easy
> and generally safe. In a multiplexed approach this becomes much more
> difficult, and depending on the search being done it could be
> misleading, perhaps even dangerous with complicated searches that
> exclude label substrings.
I'm aware of this issue, which is one of the reasons I'm
asking about the preferred approach.
> It's important to remember that Steve's strstr() comment only reflects
> his set of userspace tools. When you start talking about log
> aggregation and analytics, it seems very likely that there are other
> tools in use, likely with their own parsers that do much more
> complicated searches than a simple strstr() call.
Point. But long term, they'll have to be updated to accommodate
whatever we decide on. Which makes the "simple" case, where one
security module is in use all the more important.
>> Where is the need to use a module specific label parser coming
>> from? Does the audit code parse SELinux contexts now?
> If you can't pick a "safe" delimiter that isn't included in any of the
> LSM label formats, how else do you know how to parse the multiplexed
> mess?
Ah, but if we can ...
>>>> we have
>>>>
>>>> s = source
>>>> for (i = 0; i < lsm_slots ; i++) {
>>>> s = strstr(s, "subj_")
>>>> if (!s)
>>>> break;
>>>> s = search_after_subj_(s, lsm_slot_name[i], ...)
>>> The hand waving here in search_after_subj_(...) is much less;
>>> essentially you just match "subj_X" and then you can take the field
>>> value as the LSM's label without having to know the format, the policy
>>> loaded, etc. It is both safer and doesn't require knowledge of the
>>> LSMs (the LSM "name" can be specified as a parameter to the search
>>> tool).
>> You can do that with the Hideous format as well. I wouldn't
>> say which would be easier without delving into the audit user
>> space.
> No, you can't. You still need to parse the multiplexed mess, that's
> the problem.
You move the parsing problem to the record, where you have to
look for subj_selinux= instead of having the parsing problem in
the subj= field, where you look for something like selinux=
within the field. Neither looks like the work of an afternoon to
get right.
It probably looks like I'm arguing for the Hideous format option.
That would require less work and code disruption, so it is tempting
to push for it. But I would have to know the user space side a
whole lot better than I do to feel good about pushing anything that
isn't obviously a good choice. I kind of prefer Paul's "subj=?"
approach, but as it's harder, I don't want to spend too much time
on it if it gets me a big, juicy, well deserved NAK.
^ permalink raw reply
* Re: [GIT PULL] SafeSetID LSM changes for 5.3
From: Micah Morton @ 2019-07-17 19:40 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Linux Kernel Mailing List, linux-security-module
In-Reply-To: <CAHk-=whY1J-LFvTa8ihiPNRSv1dwxPk9ycPCEhdcjsk7c=tGAw@mail.gmail.com>
On Tue, Jul 16, 2019 at 12:06 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Jul 15, 2019 at 9:05 AM Micah Morton <mortonm@chromium.org> wrote:
> >
> > I'm maintaining the new SafeSetID LSM and was told to set up my own
> > tree for sending pull requests rather than sending my changes through
> > James Morris and the security subsystem tree.
>
> Yes. It would be good if you also added yourself to the MAINTAINERS
> file. Right now there's no entry for security/safesetid at all.
Yes, I have a patch for this but was told it would be better to send
the patch through my tree rather than the security tree. I can send a
pull request for that.
>
> > This is my first time doing one of these pull requests so hopefully I
> > didn't screw something up.
>
> So a couple of notes:
>
> - *please* don't rebase your work in the day before
Got it.
>
> Was this in linux-next? was this tested at all? Hard to tell, since
> it was rebased recently, so for all I know it's all completely new
This was not in linux-next, but was tested by Jann on a Chrome OS
device. There's also the selftest for this code. But I can send
non-trivial stuff to linux-next first next time.
>
> - don't use a random kernel-of-the-day as the base for development
Got it.
>
> This is related to the rebasing issue, but is true even if you
> don't rebase. There is no way that it was a good idea to pick my
> random - possibly completely broken - kernel from Sunday afternoon in
> the middle of a merge window as a base for development.
>
> If you start development, or if you have to rebase (for some *good*
> reason) you need to do so on a good stable base, not on the quick-sand
> that is "random kernel of the day during the busiest merge activity".
Makes sense. The development was not actually done on that kernel, I
just grabbed that random kernel for committing the changes on top of
(these changes were developed a little while ago, but they're all self
contained to the SafeSetID LSM), but I'll pick a stable one next time.
>
> - Please use the "git pull-request" format and then add any extra
> notes you feel are necessary
>
> Yes, your pull request is *almost* git pull-request, but you seem
> to have actively removed whitespace making it almost illegible. It's
> really hard to pick out the line that has the actual git repository
> address, because it's basically hidden inside one big blob of text.
>
> I've pulled this as-is since it's the first time, but I expect better next time.
>
> There are various resources on some cleanliness issues, and people
> fairly recently tried to combine it under
>
> Documentation/maintainer/rebasing-and-merging.rst
>
> which covers at least the basics on why not to rebase etc.
Thanks for the pointer. I had not seen that yet.
>
> And if you *do* end up rebasing, consider the end result "untested",
> so then it should have been done before the merge window even started,
> and the rebased branch should have been in linux-next. And not sent to
> me the very next day.
Yep, makes sense.
>
> Linus
Thanks!
^ permalink raw reply
* Re: Preferred subj= with multiple LSMs
From: Paul Moore @ 2019-07-17 16:23 UTC (permalink / raw)
To: Casey Schaufler
Cc: Steve Grubb, Richard Guy Briggs, linux-audit@redhat.com,
Linux Security Module list
In-Reply-To: <e9cf875a-0d0f-a56f-71dd-c22c67bdcc2d@schaufler-ca.com>
On Wed, Jul 17, 2019 at 11:49 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 7/17/2019 5:14 AM, Paul Moore wrote:
> > On Tue, Jul 16, 2019 at 7:47 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 7/16/2019 4:13 PM, Paul Moore wrote:
> >>> On Tue, Jul 16, 2019 at 6:18 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>> It sounds as if some variant of the Hideous format:
> >>>>
> >>>> subj=selinux='a:b:c:d',apparmor='z'
> >>>> subj=selinux/a:b:c:d/apparmor/z
> >>>> subj=(selinux)a:b:c:d/(apparmor)z
> >>>>
> >>>> would meet Steve's searchability requirements, but with significant
> >>>> parsing performance penalties.
> >>> I think "hideous format" sums it up nicely. Whatever we choose here
> >>> we are likely going to be stuck with for some time and I'm near to
> >>> 100% that multiplexing the labels onto a single field is going to be a
> >>> disaster.
> >> If the requirement is that subj= be searchable I don't see much of
> >> an alternative to a Hideous format. If we can get past that, and say
> >> that all subj_* have to be searchable we can avoid that set of issues.
> >> Instead of:
> >>
> >> s = strstr(source, "subj=")
> >> search_after_subj(s, ...);
> > This example does a lot of hand waving in search_after_subj(...)
> > regarding parsing the multiplexed LSM label. Unless we restrict the
> > LSM label formats (which seems both wrong, and too late IMHO)
>
> I don't think it's too late, and I think it would be healthy
> to restrict LSM "contexts" to character sets that make command
> line specification possible. Embedded newlines? Ewwww.
That would imply that the delimiter you would choose for the
multiplexed approach would be something odd (I think you suggested
0x02, or similar, earlier) which would likely require the multiplexed
subj field to become a hex encoded field which would be very
unfortunate in my opinion and would technically break with the current
subj/obj field format spec. Picking a normal-ish delimiter, and
restricting its use by LSMs seems wrong to me.
It's also worth noting that if you were to move subj/obj to hex
encoded fields, in addition to causing a backwards compatibility
problem, you completely kill the ability to look at the raw log data
and make sense of the fields ... well, unless you can do the ascii hex
conversion in your head on the fly.
> > we have
> > a parsing nightmare; can you write a safe multiplexed LSM label parser
> > without knowledge of each LSM label format? Can you do that for each
> > LSM without knowing their loaded policy? What happens when the policy
> > and/or label format changes? What happens in a few years when another
> > LSM is added to the kernel?
>
> I was intentionally hand-wavy because of those very issues.
Then you should already realize why this is a terrible idea ;)
> Steve says that parsing is limited to "strstr()", so looking for
> ":s7:" in the subject should work just as well with a Hideous
> format as it does today, with the exception of false positives
> where LSMs have label string overlaps.
Today when you go to search through your audit log you know that a
single LSM is providing subj labels, and you also know which LSM that
happens to be, so searching on a given string, or substring, is easy
and generally safe. In a multiplexed approach this becomes much more
difficult, and depending on the search being done it could be
misleading, perhaps even dangerous with complicated searches that
exclude label substrings.
It's important to remember that Steve's strstr() comment only reflects
his set of userspace tools. When you start talking about log
aggregation and analytics, it seems very likely that there are other
tools in use, likely with their own parsers that do much more
complicated searches than a simple strstr() call.
> Where is the need to use a module specific label parser coming
> from? Does the audit code parse SELinux contexts now?
If you can't pick a "safe" delimiter that isn't included in any of the
LSM label formats, how else do you know how to parse the multiplexed
mess?
> >> we have
> >>
> >> s = source
> >> for (i = 0; i < lsm_slots ; i++) {
> >> s = strstr(s, "subj_")
> >> if (!s)
> >> break;
> >> s = search_after_subj_(s, lsm_slot_name[i], ...)
> > The hand waving here in search_after_subj_(...) is much less;
> > essentially you just match "subj_X" and then you can take the field
> > value as the LSM's label without having to know the format, the policy
> > loaded, etc. It is both safer and doesn't require knowledge of the
> > LSMs (the LSM "name" can be specified as a parameter to the search
> > tool).
>
> You can do that with the Hideous format as well. I wouldn't
> say which would be easier without delving into the audit user
> space.
No, you can't. You still need to parse the multiplexed mess, that's
the problem.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: Preferred subj= with multiple LSMs
From: Casey Schaufler @ 2019-07-17 15:49 UTC (permalink / raw)
To: Paul Moore
Cc: Steve Grubb, Richard Guy Briggs, linux-audit@redhat.com,
Linux Security Module list, casey
In-Reply-To: <CAHC9VhQ9MSh5zCkhMja4r9j0RT952LwKSaG5dR-BqXzXrtEAUw@mail.gmail.com>
On 7/17/2019 5:14 AM, Paul Moore wrote:
> On Tue, Jul 16, 2019 at 7:47 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 7/16/2019 4:13 PM, Paul Moore wrote:
>>> On Tue, Jul 16, 2019 at 6:18 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> It sounds as if some variant of the Hideous format:
>>>>
>>>> subj=selinux='a:b:c:d',apparmor='z'
>>>> subj=selinux/a:b:c:d/apparmor/z
>>>> subj=(selinux)a:b:c:d/(apparmor)z
>>>>
>>>> would meet Steve's searchability requirements, but with significant
>>>> parsing performance penalties.
>>> I think "hideous format" sums it up nicely. Whatever we choose here
>>> we are likely going to be stuck with for some time and I'm near to
>>> 100% that multiplexing the labels onto a single field is going to be a
>>> disaster.
>> If the requirement is that subj= be searchable I don't see much of
>> an alternative to a Hideous format. If we can get past that, and say
>> that all subj_* have to be searchable we can avoid that set of issues.
>> Instead of:
>>
>> s = strstr(source, "subj=")
>> search_after_subj(s, ...);
> This example does a lot of hand waving in search_after_subj(...)
> regarding parsing the multiplexed LSM label. Unless we restrict the
> LSM label formats (which seems both wrong, and too late IMHO)
I don't think it's too late, and I think it would be healthy
to restrict LSM "contexts" to character sets that make command
line specification possible. Embedded newlines? Ewwww.
> we have
> a parsing nightmare; can you write a safe multiplexed LSM label parser
> without knowledge of each LSM label format? Can you do that for each
> LSM without knowing their loaded policy? What happens when the policy
> and/or label format changes? What happens in a few years when another
> LSM is added to the kernel?
I was intentionally hand-wavy because of those very issues.
Steve says that parsing is limited to "strstr()", so looking for
":s7:" in the subject should work just as well with a Hideous
format as it does today, with the exception of false positives
where LSMs have label string overlaps.
Where is the need to use a module specific label parser coming
from? Does the audit code parse SELinux contexts now?
>> we have
>>
>> s = source
>> for (i = 0; i < lsm_slots ; i++) {
>> s = strstr(s, "subj_")
>> if (!s)
>> break;
>> s = search_after_subj_(s, lsm_slot_name[i], ...)
> The hand waving here in search_after_subj_(...) is much less;
> essentially you just match "subj_X" and then you can take the field
> value as the LSM's label without having to know the format, the policy
> loaded, etc. It is both safer and doesn't require knowledge of the
> LSMs (the LSM "name" can be specified as a parameter to the search
> tool).
You can do that with the Hideous format as well. I wouldn't
say which would be easier without delving into the audit user
space.
>> There's enough ugly to go around either way.
>> And I'm not partial to either approach, but do would very
>> much like to get the code done so I can get on to the next
>> set of amazing challenges.
>>
>> Oh, and I don't want to pick on subj= as obj= has the exact same issues.
> Yes, I stopped talking about both subj and obj some time ago in this
> thread because I figure we can use the same approach for both.
>
^ permalink raw reply
* Re: [RFC PATCH v6 0/1] Add dm verity root hash pkcs7 sig validation.
From: Milan Broz @ 2019-07-17 13:08 UTC (permalink / raw)
To: Jaskaran Singh Khurana
Cc: ebiggers, linux-security-module, linux-kernel, linux-integrity,
linux-fsdevel, agk, snitzer, dm-devel, jmorris, Scott Shell,
Nazmus Sakib, mpatocka
In-Reply-To: <alpine.LRH.2.21.1907161035490.121213@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.inter>
Hi,
On 16/07/2019 20:08, Jaskaran Singh Khurana wrote:
>>> Could you please provide feedback on this v6 version.
>>
>> Hi,
>>
>> I am ok with the v6 patch; I think Mike will return to it in 5.4 reviews.
>>
>
> Thanks for the help and also for reviewing this patch. Could you please
> add Reviewed-by/Tested-by tag to the patch.
ok, you can add
Tested-and-Reviewed-by: Milan Broz <gmazyland@gmail.com>
or just use the version on my git, I already updated few lines because
of recent kernel changes, mainly the revert of keyring changes, tested patch is here
https://git.kernel.org/pub/scm/linux/kernel/git/mbroz/linux.git/commit/?h=dm-cryptsetup&id=266f7c9c74b23e4cb2e67ceb813dd707061c1641
...
> The steps and workflow is correct. I will send the cryptsetup changes for
> review.
ok, I'll probably try to use our existing userspace libcryptsetup API to avoid introducing new calls,
but that is not important for now - the kernel bits must be in the mainline kernel first.
Thanks,
Milan
^ permalink raw reply
* Re: Preferred subj= with multiple LSMs
From: Paul Moore @ 2019-07-17 12:23 UTC (permalink / raw)
To: James Morris
Cc: Steve Grubb, Casey Schaufler, Richard Guy Briggs,
linux-audit@redhat.com, Linux Security Module list
In-Reply-To: <alpine.LRH.2.21.1907171435200.2314@namei.org>
On Wed, Jul 17, 2019 at 12:36 AM James Morris <jmorris@namei.org> wrote:
> On Tue, 16 Jul 2019, Paul Moore wrote:
>
> > The subj_X approach is still backwards compatible, the difference is
> > that old versions of the tools get a "?" for the LSM creds which is a
> > rather sane way of indicating something is different.
>
> This will still break existing userspace, right? We can't do that.
Trust me, I don't want to break userspace, I wouldn't be suggesting that.
The subj_X approach would cause userspace to see a "?" for the LSM
creds when looking at logs from a stacked-LSM system. I would argue
this is actually safer than the multiplexed approach as "?" is a safe
sentinel used by the audit subsystem when the value can't be
determined; the multiplexed label in the hands of legacy userspace
tools would be confusing at best, and misleading at worst.
> > Once again, I believe that the subj_X approach is going to be faster
> > than safely parsing the multiplexed format.
>
> What about emitting one audit record for each LSM?
In many of the LSM generated audit events that is what would happen,
and should just work. What we've been discussing in all the cases
where the audit event is generated outside the context of the LSM but
the LSM credentials are still desirable bits of information. While we
are definitely going in the direction of making multiple record events
more common, duplicating the same record, with only changes to the LSM
creds, may end up confusing Steve's tools. It would also end up
bloating the audit log, which I know is something everyone wants to
avoid.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: Preferred subj= with multiple LSMs
From: Paul Moore @ 2019-07-17 12:14 UTC (permalink / raw)
To: Casey Schaufler
Cc: Steve Grubb, Richard Guy Briggs, linux-audit@redhat.com,
Linux Security Module list
In-Reply-To: <5ea2a25b-364f-3c30-79c6-cfb18515d7ba@schaufler-ca.com>
On Tue, Jul 16, 2019 at 7:47 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 7/16/2019 4:13 PM, Paul Moore wrote:
> > On Tue, Jul 16, 2019 at 6:18 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> It sounds as if some variant of the Hideous format:
> >>
> >> subj=selinux='a:b:c:d',apparmor='z'
> >> subj=selinux/a:b:c:d/apparmor/z
> >> subj=(selinux)a:b:c:d/(apparmor)z
> >>
> >> would meet Steve's searchability requirements, but with significant
> >> parsing performance penalties.
> > I think "hideous format" sums it up nicely. Whatever we choose here
> > we are likely going to be stuck with for some time and I'm near to
> > 100% that multiplexing the labels onto a single field is going to be a
> > disaster.
>
> If the requirement is that subj= be searchable I don't see much of
> an alternative to a Hideous format. If we can get past that, and say
> that all subj_* have to be searchable we can avoid that set of issues.
> Instead of:
>
> s = strstr(source, "subj=")
> search_after_subj(s, ...);
This example does a lot of hand waving in search_after_subj(...)
regarding parsing the multiplexed LSM label. Unless we restrict the
LSM label formats (which seems both wrong, and too late IMHO) we have
a parsing nightmare; can you write a safe multiplexed LSM label parser
without knowledge of each LSM label format? Can you do that for each
LSM without knowing their loaded policy? What happens when the policy
and/or label format changes? What happens in a few years when another
LSM is added to the kernel?
> we have
>
> s = source
> for (i = 0; i < lsm_slots ; i++) {
> s = strstr(s, "subj_")
> if (!s)
> break;
> s = search_after_subj_(s, lsm_slot_name[i], ...)
The hand waving here in search_after_subj_(...) is much less;
essentially you just match "subj_X" and then you can take the field
value as the LSM's label without having to know the format, the policy
loaded, etc. It is both safer and doesn't require knowledge of the
LSMs (the LSM "name" can be specified as a parameter to the search
tool).
> There's enough ugly to go around either way.
> And I'm not partial to either approach, but do would very
> much like to get the code done so I can get on to the next
> set of amazing challenges.
>
> Oh, and I don't want to pick on subj= as obj= has the exact same issues.
Yes, I stopped talking about both subj and obj some time ago in this
thread because I figure we can use the same approach for both.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* KASAN: use-after-free Write in check_noncircular
From: syzbot @ 2019-07-17 8:58 UTC (permalink / raw)
To: ast, daniel, jmorris, john.fastabend, linux-kernel,
linux-security-module, netdev, penguin-kernel, serge,
syzkaller-bugs, takedakn
Hello,
syzbot found the following crash on:
HEAD commit: 9637d517 Merge tag 'for-linus-20190715' of git://git.kerne..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=12f42e1fa00000
kernel config: https://syzkaller.appspot.com/x/.config?x=88095c4f62402bcd
dashboard link: https://syzkaller.appspot.com/bug?extid=f5ceb7c55f59455035ca
compiler: clang version 9.0.0 (/home/glider/llvm/clang
80fee25776c2fb61e74c1ecb1a523375c2500b69)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12cfbe1fa00000
The bug was bisected to:
commit e9db4ef6bf4ca9894bb324c76e01b8f1a16b2650
Author: John Fastabend <john.fastabend@gmail.com>
Date: Sat Jun 30 13:17:47 2018 +0000
bpf: sockhash fix omitted bucket lock in sock_close
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=109c3078600000
final crash: https://syzkaller.appspot.com/x/report.txt?x=129c3078600000
console output: https://syzkaller.appspot.com/x/log.txt?x=149c3078600000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+f5ceb7c55f59455035ca@syzkaller.appspotmail.com
Fixes: e9db4ef6bf4c ("bpf: sockhash fix omitted bucket lock in sock_close")
==================================================================
BUG: KASAN: use-after-free in check_noncircular+0x91/0x560
kernel/locking/lockdep.c:1722
Write of size 56 at addr ffff888089815160 by task syz-executor.4/8772
CPU: 1 PID: 8772 Comm: syz-executor.4 Not tainted 5.2.0+ #31
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
Allocated by task 8457:
save_stack mm/kasan/common.c:69 [inline]
set_track mm/kasan/common.c:77 [inline]
__kasan_kmalloc+0x11c/0x1b0 mm/kasan/common.c:487
kasan_kmalloc+0x9/0x10 mm/kasan/common.c:501
kmem_cache_alloc_trace+0x221/0x2f0 mm/slab.c:3550
kmalloc include/linux/slab.h:552 [inline]
tomoyo_print_header security/tomoyo/audit.c:156 [inline]
tomoyo_init_log+0x176/0x1f20 security/tomoyo/audit.c:255
tomoyo_supervisor+0x39c/0x13f0 security/tomoyo/common.c:2095
tomoyo_audit_path_log security/tomoyo/file.c:168 [inline]
tomoyo_path_permission security/tomoyo/file.c:587 [inline]
tomoyo_check_open_permission+0x488/0x9e0 security/tomoyo/file.c:777
tomoyo_file_open+0x141/0x190 security/tomoyo/tomoyo.c:319
security_file_open+0x65/0x2f0 security/security.c:1457
do_dentry_open+0x397/0x1060 fs/open.c:765
vfs_open+0x73/0x80 fs/open.c:887
do_last fs/namei.c:3416 [inline]
path_openat+0x136d/0x4400 fs/namei.c:3533
do_filp_open+0x1f7/0x430 fs/namei.c:3563
do_sys_open+0x343/0x620 fs/open.c:1070
__do_sys_open fs/open.c:1088 [inline]
__se_sys_open fs/open.c:1083 [inline]
__x64_sys_open+0x87/0x90 fs/open.c:1083
do_syscall_64+0xfe/0x140 arch/x86/entry/common.c:296
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Freed by task 8457:
save_stack mm/kasan/common.c:69 [inline]
set_track mm/kasan/common.c:77 [inline]
__kasan_slab_free+0x12a/0x1e0 mm/kasan/common.c:449
kasan_slab_free+0xe/0x10 mm/kasan/common.c:457
__cache_free mm/slab.c:3425 [inline]
kfree+0x115/0x200 mm/slab.c:3756
tomoyo_init_log+0x1bf7/0x1f20 security/tomoyo/audit.c:294
tomoyo_supervisor+0x39c/0x13f0 security/tomoyo/common.c:2095
tomoyo_audit_path_log security/tomoyo/file.c:168 [inline]
tomoyo_path_permission security/tomoyo/file.c:587 [inline]
tomoyo_check_open_permission+0x488/0x9e0 security/tomoyo/file.c:777
tomoyo_file_open+0x141/0x190 security/tomoyo/tomoyo.c:319
security_file_open+0x65/0x2f0 security/security.c:1457
do_dentry_open+0x397/0x1060 fs/open.c:765
vfs_open+0x73/0x80 fs/open.c:887
do_last fs/namei.c:3416 [inline]
path_openat+0x136d/0x4400 fs/namei.c:3533
do_filp_open+0x1f7/0x430 fs/namei.c:3563
do_sys_open+0x343/0x620 fs/open.c:1070
__do_sys_open fs/open.c:1088 [inline]
__se_sys_open fs/open.c:1083 [inline]
__x64_sys_open+0x87/0x90 fs/open.c:1083
do_syscall_64+0xfe/0x140 arch/x86/entry/common.c:296
entry_SYSCALL_64_after_hwframe+0x49/0xbe
The buggy address belongs to the object at ffff8880898146c0
which belongs to the cache kmalloc-4k of size 4096
The buggy address is located 2720 bytes inside of
4096-byte region [ffff8880898146c0, ffff8880898156c0)
The buggy address belongs to the page:
page:ffffea0002260500 refcount:1 mapcount:0 mapping:ffff8880aa402000
index:0x0 compound_mapcount: 0
flags: 0x1fffc0000010200(slab|head)
raw: 01fffc0000010200 ffffea0002263b08 ffffea0002916d08 ffff8880aa402000
raw: 0000000000000000 ffff8880898146c0 0000000100000001 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff888089815000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff888089815080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff888089815100: fb fb fb fb f1 f1 f1 f1 00 f2 f2 f2 fb fb fb fb
^
ffff888089815180: fb fb fb f3 f3 f3 f3 f3 fb fb fb fb fb fb fb fb
ffff888089815200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: Preferred subj= with multiple LSMs
From: James Morris @ 2019-07-17 4:36 UTC (permalink / raw)
To: Paul Moore
Cc: Steve Grubb, Casey Schaufler, Richard Guy Briggs,
linux-audit@redhat.com, Linux Security Module list
In-Reply-To: <CAHC9VhS-WGuKYzG5AU01BN8tk8nzp+7qxk7Sz-hbHQGL+UXOfg@mail.gmail.com>
On Tue, 16 Jul 2019, Paul Moore wrote:
> The subj_X approach is still backwards compatible, the difference is
> that old versions of the tools get a "?" for the LSM creds which is a
> rather sane way of indicating something is different.
This will still break existing userspace, right? We can't do that.
> Once again, I believe that the subj_X approach is going to be faster
> than safely parsing the multiplexed format.
What about emitting one audit record for each LSM?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: Preferred subj= with multiple LSMs
From: Casey Schaufler @ 2019-07-16 23:47 UTC (permalink / raw)
To: Paul Moore
Cc: Steve Grubb, Richard Guy Briggs, linux-audit@redhat.com,
Linux Security Module list, casey
In-Reply-To: <CAHC9VhTpcnyGg5j3b6Z7Yi0Ob01JETRiBmz1AuLqPWqP9tEAnA@mail.gmail.com>
On 7/16/2019 4:13 PM, Paul Moore wrote:
> On Tue, Jul 16, 2019 at 6:18 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> It sounds as if some variant of the Hideous format:
>>
>> subj=selinux='a:b:c:d',apparmor='z'
>> subj=selinux/a:b:c:d/apparmor/z
>> subj=(selinux)a:b:c:d/(apparmor)z
>>
>> would meet Steve's searchability requirements, but with significant
>> parsing performance penalties.
> I think "hideous format" sums it up nicely. Whatever we choose here
> we are likely going to be stuck with for some time and I'm near to
> 100% that multiplexing the labels onto a single field is going to be a
> disaster.
If the requirement is that subj= be searchable I don't see much of
an alternative to a Hideous format. If we can get past that, and say
that all subj_* have to be searchable we can avoid that set of issues.
Instead of:
s = strstr(source, "subj=")
search_after_subj(s, ...);
we have
s = source
for (i = 0; i < lsm_slots ; i++) {
s = strstr(s, "subj_")
if (!s)
break;
s = search_after_subj_(s, lsm_slot_name[i], ...)
}
There's enough ugly to go around either way.
And I'm not partial to either approach, but do would very
much like to get the code done so I can get on to the next
set of amazing challenges.
Oh, and I don't want to pick on subj= as obj= has the exact same issues.
^ permalink raw reply
* Re: [PATCH V35 27/29] tracefs: Restrict tracefs when the kernel is locked down
From: Matthew Garrett @ 2019-07-16 23:22 UTC (permalink / raw)
To: Steven Rostedt
Cc: James Morris, LSM List, Linux Kernel Mailing List, Linux API
In-Reply-To: <20190716191439.59a1ac32@gandalf.local.home>
On Tue, Jul 16, 2019 at 4:14 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> Small nit, but please add this as the first declaration, to keep the
> "upside-down x-mas tree" look. I know some of the other functions in
> this file don't follow that (which should be cleaned up some day), but
> I'd like to avoid adding more that breaks the aesthetic of the code.
ACK.
> > +
> > + if (fops)
>
> I think you meant "if (!fops)".
Blink. Whoops! Yup.
^ permalink raw reply
* Re: [PATCH V35 27/29] tracefs: Restrict tracefs when the kernel is locked down
From: Steven Rostedt @ 2019-07-16 23:14 UTC (permalink / raw)
To: Matthew Garrett
Cc: jmorris, linux-security-module, linux-kernel, linux-api,
Matthew Garrett
In-Reply-To: <20190715195946.223443-28-matthewgarrett@google.com>
On Mon, 15 Jul 2019 12:59:44 -0700
Matthew Garrett <matthewgarrett@google.com> wrote:
> Tracefs may release more information about the kernel than desirable, so
> restrict it when the kernel is locked down in confidentiality mode by
> preventing open().
>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> ---
>
> @@ -389,6 +414,7 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
> {
> struct dentry *dentry;
> struct inode *inode;
> + struct file_operations *proxy_fops;
Small nit, but please add this as the first declaration, to keep the
"upside-down x-mas tree" look. I know some of the other functions in
this file don't follow that (which should be cleaned up some day), but
I'd like to avoid adding more that breaks the aesthetic of the code.
>
> if (!(mode & S_IFMT))
> mode |= S_IFREG;
> @@ -402,8 +428,18 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
> if (unlikely(!inode))
> return failed_creating(dentry);
>
> + proxy_fops = kzalloc(sizeof(struct file_operations), GFP_KERNEL);
> + if (!proxy_fops)
> + return failed_creating(dentry);
> +
> + if (fops)
I think you meant "if (!fops)".
-- Steve
> + fops = &tracefs_file_operations;
> +
> + dentry->d_fsdata = (void *)fops;
> + memcpy(proxy_fops, fops, sizeof(*proxy_fops));
> + proxy_fops->open = default_open_file;
> inode->i_mode = mode;
> - inode->i_fop = fops ? fops : &tracefs_file_operations;
> + inode->i_fop = proxy_fops;
> inode->i_private = data;
> d_instantiate(dentry, inode);
> fsnotify_create(dentry->d_parent->d_inode, dentry);
^ permalink raw reply
* Re: Preferred subj= with multiple LSMs
From: Paul Moore @ 2019-07-16 23:13 UTC (permalink / raw)
To: Casey Schaufler
Cc: Steve Grubb, Richard Guy Briggs, linux-audit@redhat.com,
Linux Security Module list
In-Reply-To: <27e2c710-efe6-d9cd-d4f9-bc217df5ede3@schaufler-ca.com>
On Tue, Jul 16, 2019 at 6:18 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> It sounds as if some variant of the Hideous format:
>
> subj=selinux='a:b:c:d',apparmor='z'
> subj=selinux/a:b:c:d/apparmor/z
> subj=(selinux)a:b:c:d/(apparmor)z
>
> would meet Steve's searchability requirements, but with significant
> parsing performance penalties.
I think "hideous format" sums it up nicely. Whatever we choose here
we are likely going to be stuck with for some time and I'm near to
100% that multiplexing the labels onto a single field is going to be a
disaster.
--
paul moore
www.paul-moore.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox