[permalink] [raw]

Subject: Re: [PATCH 2/9] security: Add hooks to rule on setting a watch [ver #5]

On 6/28/19 11:48 AM, David Howells wrote:
> Add security hooks that will allow an LSM to rule on whether or not a watch
> may be set. More than one hook is required as the watches watch different
> types of object.
>
> Signed-off-by: David Howells <[email protected]>
> cc: Casey Schaufler <[email protected]>
> cc: Stephen Smalley <[email protected]>
> cc: [email protected]
> ---
>
> include/linux/lsm_hooks.h | 22 ++++++++++++++++++++++
> include/linux/security.h | 15 +++++++++++++++
> security/security.c | 13 +++++++++++++
> 3 files changed, 50 insertions(+)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..f9d31f6445e4 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1413,6 +1413,20 @@
> * @ctx is a pointer in which to place the allocated security context.
> * @ctxlen points to the place to put the length of @ctx.
> *
> + * Security hooks for the general notification queue:
> + *
> + * @watch_key:
> + * Check to see if a process is allowed to watch for event notifications
> + * from a key or keyring.
> + * @watch: The watch object
> + * @key: The key to watch.
> + *
> + * @watch_devices:
> + * Check to see if a process is allowed to watch for event notifications
> + * from devices (as a global set).
> + * @watch: The watch object

It is difficult to evaluate these without at least one implementation of
each hook. I am unclear as to how any security module would use the
watch argument, since it has no security field/blob and does not appear
to contain any information that would be relevant to deciding whether or
not to permit the watch to be set.

> + *
> + *
> * Security hooks for using the eBPF maps and programs functionalities through
> * eBPF syscalls.
> *
> @@ -1688,6 +1702,10 @@ union security_list_options {
> int (*inode_notifysecctx)(struct inode *inode, void *ctx, u32 ctxlen);
> int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32 ctxlen);
> int (*inode_getsecctx)(struct inode *inode, void **ctx, u32 *ctxlen);
> +#ifdef CONFIG_WATCH_QUEUE
> + int (*watch_key)(struct watch *watch, struct key *key);
> + int (*watch_devices)(struct watch *watch);
> +#endif /* CONFIG_WATCH_QUEUE */
>
> #ifdef CONFIG_SECURITY_NETWORK
> int (*unix_stream_connect)(struct sock *sock, struct sock *other,
> @@ -1964,6 +1982,10 @@ struct security_hook_heads {
> struct hlist_head inode_notifysecctx;
> struct hlist_head inode_setsecctx;
> struct hlist_head inode_getsecctx;
> +#ifdef CONFIG_WATCH_QUEUE
> + struct hlist_head watch_key;
> + struct hlist_head watch_devices;
> +#endif /* CONFIG_WATCH_QUEUE */
> #ifdef CONFIG_SECURITY_NETWORK
> struct hlist_head unix_stream_connect;
> struct hlist_head unix_may_send;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 659071c2e57c..540863678355 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -57,6 +57,7 @@ struct mm_struct;
> struct fs_context;
> struct fs_parameter;
> enum fs_value_type;
> +struct watch;
>
> /* Default (no) options for the capable function */
> #define CAP_OPT_NONE 0x0
> @@ -392,6 +393,10 @@ 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);
> int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> +#ifdef CONFIG_WATCH_QUEUE
> +int security_watch_key(struct watch *watch, struct key *key);
> +int security_watch_devices(struct watch *watch);
> +#endif /* CONFIG_WATCH_QUEUE */
> #else /* CONFIG_SECURITY */
>
> static inline int call_lsm_notifier(enum lsm_event event, void *data)
> @@ -1204,6 +1209,16 @@ static inline int security_inode_getsecctx(struct inode *inode, void **ctx, u32
> {
> return -EOPNOTSUPP;
> }
> +#ifdef CONFIG_WATCH_QUEUE
> +static inline int security_watch_key(struct watch *watch, struct key *key)
> +{
> + return 0;
> +}
> +static inline int security_watch_devices(struct watch *watch)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_WATCH_QUEUE */
> #endif /* CONFIG_SECURITY */
>
> #ifdef CONFIG_SECURITY_NETWORK
> diff --git a/security/security.c b/security/security.c
> index 613a5c00e602..2c9919226ad1 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1917,6 +1917,19 @@ int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
> }
> EXPORT_SYMBOL(security_inode_getsecctx);
>
> +#ifdef CONFIG_WATCH_QUEUE
> +int security_watch_key(struct watch *watch, struct key *key)
> +{
> + return call_int_hook(watch_key, 0, watch, key);
> +}
> +
> +int security_watch_devices(struct watch *watch)
> +{
> + return call_int_hook(watch_devices, 0, watch);
> +}
> +
> +#endif /* CONFIG_WATCH_QUEUE */
> +
> #ifdef CONFIG_SECURITY_NETWORK
>
> int security_unix_stream_connect(struct sock *sock, struct sock *other, struct sock *newsk)
>

2019-07-08 22:47:47

by Stephen Smalley

[permalink] [raw]

Subject: Re: [PATCH 3/9] security: Add a hook for the point of notification insertion [ver #5]

On 6/28/19 11:49 AM, David Howells wrote:
> Add a security hook that allows an LSM to rule on whether a notification
> message is allowed to be inserted into a particular watch queue.
>
> The hook is given the following information:
>
> (1) The credentials of the triggerer (which may be init_cred for a system
> notification, eg. a hardware error).
>
> (2) The credentials of the whoever set the watch.
>
> (3) The notification message.

As with the other proposed hooks, it is difficult to evaluate this hook
without at least one implementation of the hook. Since Casey is the
only one requesting this hook, a Smack implementation would be the
natural choice; I do not intend to implement this hook for SELinux.
However, by providing this hook, you are in effect taking a position wrt
the earlier controversy over it, i.e. that application developers must
deal with the possibility that notifications can be dropped if a
security module does not permit the triggerer to post the notification
to the watcher, without any indication to either the triggerer or the
watcher. This is a choice you are making by providing this hook. The
alternative is to require that permission to set a watch imply the
ability to receive all notifications for the watched object. Aside from
friendliness to application developers, the latter also yields stable,
sane policy and better performance.

>
> Signed-off-by: David Howells <[email protected]>
> cc: Casey Schaufler <[email protected]>
> cc: Stephen Smalley <[email protected]>
> cc: [email protected]
> ---
>
> include/linux/lsm_hooks.h | 10 ++++++++++
> include/linux/security.h | 10 ++++++++++
> security/security.c | 6 ++++++
> 3 files changed, 26 insertions(+)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index f9d31f6445e4..fd4b2b14e7d0 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1426,6 +1426,12 @@
> * from devices (as a global set).
> * @watch: The watch object
> *
> + * @post_notification:
> + * Check to see if a watch notification can be posted to a particular
> + * queue.
> + * @w_cred: The credentials of the whoever set the watch.
> + * @cred: The event-triggerer's credentials
> + * @n: The notification being posted
> *
> * Security hooks for using the eBPF maps and programs functionalities through
> * eBPF syscalls.
> @@ -1705,6 +1711,9 @@ union security_list_options {
> #ifdef CONFIG_WATCH_QUEUE
> int (*watch_key)(struct watch *watch, struct key *key);
> int (*watch_devices)(struct watch *watch);
> + int (*post_notification)(const struct cred *w_cred,
> + const struct cred *cred,
> + struct watch_notification *n);
> #endif /* CONFIG_WATCH_QUEUE */
>
> #ifdef CONFIG_SECURITY_NETWORK
> @@ -1985,6 +1994,7 @@ struct security_hook_heads {
> #ifdef CONFIG_WATCH_QUEUE
> struct hlist_head watch_key;
> struct hlist_head watch_devices;
> + struct hlist_head post_notification;
> #endif /* CONFIG_WATCH_QUEUE */
> #ifdef CONFIG_SECURITY_NETWORK
> struct hlist_head unix_stream_connect;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 540863678355..5c074bf18bea 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -58,6 +58,7 @@ struct fs_context;
> struct fs_parameter;
> enum fs_value_type;
> struct watch;
> +struct watch_notification;
>
> /* Default (no) options for the capable function */
> #define CAP_OPT_NONE 0x0
> @@ -396,6 +397,9 @@ int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> #ifdef CONFIG_WATCH_QUEUE
> int security_watch_key(struct watch *watch, struct key *key);
> int security_watch_devices(struct watch *watch);
> +int security_post_notification(const struct cred *w_cred,
> + const struct cred *cred,
> + struct watch_notification *n);
> #endif /* CONFIG_WATCH_QUEUE */
> #else /* CONFIG_SECURITY */
>
> @@ -1218,6 +1222,12 @@ static inline int security_watch_devices(struct watch *watch)
> {
> return 0;
> }
> +static inline int security_post_notification(const struct cred *w_cred,
> + const struct cred *cred,
> + struct watch_notification *n)
> +{
> + return 0;
> +}
> #endif /* CONFIG_WATCH_QUEUE */
> #endif /* CONFIG_SECURITY */
>
> diff --git a/security/security.c b/security/security.c
> index 2c9919226ad1..459e87d55ac9 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1928,6 +1928,12 @@ int security_watch_devices(struct watch *watch)
> return call_int_hook(watch_devices, 0, watch);
> }
>
> +int security_post_notification(const struct cred *w_cred,
> + const struct cred *cred,
> + struct watch_notification *n)
> +{
> + return call_int_hook(post_notification, 0, w_cred, cred, n);
> +}
> #endif /* CONFIG_WATCH_QUEUE */
>
> #ifdef CONFIG_SECURITY_NETWORK
>