The security_key_alloc() function acted as both an authorizer and
security structure allocation function. These roles should be
separated. There are two reasons for this.
First, if two modules are stacked, the first module might grant
permission and allocate security data, after which the second
module refuses permission.
Second, by adding a security_post_alloc() function after the
serial number has been assigned, security modules can append
useful info.
Note that currently there is no LSM using these hooks, so the
question of whether an LSM needs to recored the serial number
can't really be answered.
An alternative to this patch, supported by the historical approach
to LSM hooks, would be to remove all these hooks. However as
the keystore starts being used - in particular by, eg, ecryptfs -
one might expect LSMs to be more interested in key activity.
:100644 100644 aaa0a5c... 3d8602e... M include/linux/security.h
:100644 100644 fd99429... 1eff777... M security/dummy.c
:100644 100644 a057e33... 6be6269... M security/keys/key.c
Signed-off-by: Serge Hallyn <[email protected]>
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -844,10 +844,14 @@ struct swap_info_struct;
* Security hooks affecting all Key Management operations
*
* @key_alloc:
- * Permit allocation of a key and assign security data. Note that key does
- * not have a serial number assigned at this point.
+ * Check permission to allocate a key and assign security data. Note
+ * that key does not have a serial number assigned at this point.
* @key points to the key.
* Return 0 if permission is granted, -ve error otherwise.
+ * @key_post_alloc:
+ * Allocate and attach a security structure to a key structure.
+ * At this point there is a serial number attached to the key.
+ * @key points to the key.
* @key_free:
* Notification of destruction; free security data.
* @key points to the key.
@@ -1312,6 +1316,7 @@ struct security_operations {
/* key management security hooks */
#ifdef CONFIG_KEYS
int (*key_alloc)(struct key *key);
+ void (*key_post_alloc)(struct key *key);
void (*key_free)(struct key *key);
int (*key_permission)(key_ref_t key_ref,
struct task_struct *context,
@@ -3001,6 +3006,11 @@ static inline int security_key_alloc(str
return security_ops->key_alloc(key);
}
+static inline void security_key_post_alloc(struct key *key)
+{
+ security_ops->key_post_alloc(key);
+}
+
static inline void security_key_free(struct key *key)
{
security_ops->key_free(key);
@@ -3020,6 +3030,10 @@ static inline int security_key_alloc(str
return 0;
}
+static inline void security_key_post_alloc(struct key *key)
+{
+}
+
static inline void security_key_free(struct key *key)
{
}
diff --git a/security/dummy.c b/security/dummy.c
index fd99429..1eff777 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -860,6 +860,10 @@ static inline int dummy_key_alloc(struct
return 0;
}
+static inline void dummy_key_post_alloc(struct key *key)
+{
+}
+
static inline void dummy_key_free(struct key *key)
{
}
@@ -1036,6 +1040,7 @@ void security_fixup_ops (struct security
#endif /* CONFIG_SECURITY_NETWORK_XFRM */
#ifdef CONFIG_KEYS
set_to_dummy_if_null(ops, key_alloc);
+ set_to_dummy_if_null(ops, key_post_alloc);
set_to_dummy_if_null(ops, key_free);
set_to_dummy_if_null(ops, key_permission);
#endif /* CONFIG_KEYS */
diff --git a/security/keys/key.c b/security/keys/key.c
index a057e33..6be6269 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -325,6 +325,7 @@ struct key *key_alloc(struct key_type *t
/* publish the key by giving it a serial number */
atomic_inc(&user->nkeys);
key_alloc_serial(key);
+ security_key_post_alloc(key);
error:
return key;
On Tue, 2006-03-28 at 07:05 -0600, Serge E. Hallyn wrote:
> The security_key_alloc() function acted as both an authorizer and
> security structure allocation function. These roles should be
> separated. There are two reasons for this.
>
> First, if two modules are stacked, the first module might grant
> permission and allocate security data, after which the second
> module refuses permission.
>
> Second, by adding a security_post_alloc() function after the
> serial number has been assigned, security modules can append
> useful info.
Are you sure that the key cannot be accessed (looked up) by another
process as soon as it is assigned a serial number? If it can be, then
you risk having it accessed before its security structure is set up.
> Note that currently there is no LSM using these hooks, so the
> question of whether an LSM needs to recored the serial number
> can't really be answered.
>
> An alternative to this patch, supported by the historical approach
> to LSM hooks, would be to remove all these hooks. However as
> the keystore starts being used - in particular by, eg, ecryptfs -
> one might expect LSMs to be more interested in key activity.
We are interested in using these hooks for SELinux, as we need complete
mediation for MAC. But we aren't yet clear on the precise usage model,
so we don't have a well-defined set of controls just yet.
>
> :100644 100644 aaa0a5c... 3d8602e... M include/linux/security.h
> :100644 100644 fd99429... 1eff777... M security/dummy.c
> :100644 100644 a057e33... 6be6269... M security/keys/key.c
>
> Signed-off-by: Serge Hallyn <[email protected]>
>
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -844,10 +844,14 @@ struct swap_info_struct;
> * Security hooks affecting all Key Management operations
> *
> * @key_alloc:
> - * Permit allocation of a key and assign security data. Note that key does
> - * not have a serial number assigned at this point.
> + * Check permission to allocate a key and assign security data. Note
> + * that key does not have a serial number assigned at this point.
> * @key points to the key.
> * Return 0 if permission is granted, -ve error otherwise.
> + * @key_post_alloc:
> + * Allocate and attach a security structure to a key structure.
> + * At this point there is a serial number attached to the key.
> + * @key points to the key.
> * @key_free:
> * Notification of destruction; free security data.
> * @key points to the key.
> @@ -1312,6 +1316,7 @@ struct security_operations {
> /* key management security hooks */
> #ifdef CONFIG_KEYS
> int (*key_alloc)(struct key *key);
> + void (*key_post_alloc)(struct key *key);
> void (*key_free)(struct key *key);
> int (*key_permission)(key_ref_t key_ref,
> struct task_struct *context,
> @@ -3001,6 +3006,11 @@ static inline int security_key_alloc(str
> return security_ops->key_alloc(key);
> }
>
> +static inline void security_key_post_alloc(struct key *key)
> +{
> + security_ops->key_post_alloc(key);
> +}
> +
> static inline void security_key_free(struct key *key)
> {
> security_ops->key_free(key);
> @@ -3020,6 +3030,10 @@ static inline int security_key_alloc(str
> return 0;
> }
>
> +static inline void security_key_post_alloc(struct key *key)
> +{
> +}
> +
> static inline void security_key_free(struct key *key)
> {
> }
> diff --git a/security/dummy.c b/security/dummy.c
> index fd99429..1eff777 100644
> --- a/security/dummy.c
> +++ b/security/dummy.c
> @@ -860,6 +860,10 @@ static inline int dummy_key_alloc(struct
> return 0;
> }
>
> +static inline void dummy_key_post_alloc(struct key *key)
> +{
> +}
> +
> static inline void dummy_key_free(struct key *key)
> {
> }
> @@ -1036,6 +1040,7 @@ void security_fixup_ops (struct security
> #endif /* CONFIG_SECURITY_NETWORK_XFRM */
> #ifdef CONFIG_KEYS
> set_to_dummy_if_null(ops, key_alloc);
> + set_to_dummy_if_null(ops, key_post_alloc);
> set_to_dummy_if_null(ops, key_free);
> set_to_dummy_if_null(ops, key_permission);
> #endif /* CONFIG_KEYS */
> diff --git a/security/keys/key.c b/security/keys/key.c
> index a057e33..6be6269 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -325,6 +325,7 @@ struct key *key_alloc(struct key_type *t
> /* publish the key by giving it a serial number */
> atomic_inc(&user->nkeys);
> key_alloc_serial(key);
> + security_key_post_alloc(key);
>
> error:
> return key;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Stephen Smalley
National Security Agency
Serge E. Hallyn <[email protected]> wrote:
> The security_key_alloc() function acted as both an authorizer and
> security structure allocation function. These roles should be
> separated. There are two reasons for this.
Your patch is part of what I had originally, but this was mildly objected to
because SE Linux didn't care, so I dropped the post function for later
resurrection. See the email thread rooted on:
| From: David Howells <[email protected]>
| To: [email protected], [email protected]
| Date: Wed, 05 Oct 2005 17:28:34 +0100
| Cc: [email protected], [email protected]
| Subject: [Keyrings] [PATCH] Keys: Add LSM hooks for key management
in which I included key_post_alloc():
/* publish the key by giving it a serial number */
atomic_inc(&user->nkeys);
key_alloc_serial(key);
- error:
+ /* let the security module know the key has been published */
+ security_key_post_alloc(key);
+
+error:
I'm happy with this patch, though I'd like the comment you can see in the
above snippet to be added back into key.c too.
Note also that by the time the post function is called, it's a little late to
be authorising creation of the key since the key has been published by that
point.
Acked-By: David Howells <[email protected]>
Quoting Stephen Smalley ([email protected]):
> On Tue, 2006-03-28 at 07:05 -0600, Serge E. Hallyn wrote:
> > The security_key_alloc() function acted as both an authorizer and
> > security structure allocation function. These roles should be
> > separated. There are two reasons for this.
> >
> > First, if two modules are stacked, the first module might grant
> > permission and allocate security data, after which the second
> > module refuses permission.
> >
> > Second, by adding a security_post_alloc() function after the
> > serial number has been assigned, security modules can append
> > useful info.
>
> Are you sure that the key cannot be accessed (looked up) by another
> process as soon as it is assigned a serial number? If it can be, then
> you risk having it accessed before its security structure is set up.
Ah, that makes sense, and even rings a bell.
So if we were to add a post_alloc() hook, it should likely go into
key_alloc_serial() under the key_serial_lock?
Still assuming that storing the serial number is desirable...
thanks,
-serge
The security_key_alloc() function acted as both an authorizer and
security structure allocation function. These roles should be
separated. There are two reasons for this.
First, if two modules are stacked, the first module might grant
permission and allocate security data, after which the second
module refuses permission.
Second, by adding a security_post_alloc() function after the
serial number has been assigned, security modules can append
useful info.
Note that currently there is no LSM using these hooks, so the
question of whether an LSM needs to recored the serial number
can't really be answered.
An alternative to this patch, supported by the historical approach
to LSM hooks, would be to remove all these hooks. However as
the keystore starts being used - in particular by, eg, ecryptfs -
one might expect LSMs to be more interested in key activity.
Changelog:
Moved the security_key_post_alloc() hook under the
key_serial_lock to ensure that is not accessed before
an LSM has a chance to tag it.
:100644 100644 aaa0a5c... 569e874... M include/linux/security.h
:100644 100644 fd99429... 1eff777... M security/dummy.c
:100644 100644 a057e33... b72ffe2... M security/keys/key.c
Signed-off-by: Serge Hallyn <[email protected]>
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -844,10 +844,15 @@ struct swap_info_struct;
* Security hooks affecting all Key Management operations
*
* @key_alloc:
- * Permit allocation of a key and assign security data. Note that key does
- * not have a serial number assigned at this point.
+ * Check permission to allocate a key and assign security data. Note
+ * that key does not have a serial number assigned at this point.
* @key points to the key.
* Return 0 if permission is granted, -ve error otherwise.
+ * @key_post_alloc:
+ * Allocate and attach a security structure to a key structure.
+ * This is called under the key_serial_lock spinlock, after a
+ * serial number is assigned and the key is inserted into a keyring.
+ * @key points to the key.
* @key_free:
* Notification of destruction; free security data.
* @key points to the key.
@@ -1312,6 +1317,7 @@ struct security_operations {
/* key management security hooks */
#ifdef CONFIG_KEYS
int (*key_alloc)(struct key *key);
+ void (*key_post_alloc)(struct key *key);
void (*key_free)(struct key *key);
int (*key_permission)(key_ref_t key_ref,
struct task_struct *context,
@@ -3001,6 +3007,11 @@ static inline int security_key_alloc(str
return security_ops->key_alloc(key);
}
+static inline void security_key_post_alloc(struct key *key)
+{
+ security_ops->key_post_alloc(key);
+}
+
static inline void security_key_free(struct key *key)
{
security_ops->key_free(key);
@@ -3020,6 +3031,10 @@ static inline int security_key_alloc(str
return 0;
}
+static inline void security_key_post_alloc(struct key *key)
+{
+}
+
static inline void security_key_free(struct key *key)
{
}
diff --git a/security/dummy.c b/security/dummy.c
index fd99429..1eff777 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -860,6 +860,10 @@ static inline int dummy_key_alloc(struct
return 0;
}
+static inline void dummy_key_post_alloc(struct key *key)
+{
+}
+
static inline void dummy_key_free(struct key *key)
{
}
@@ -1036,6 +1040,7 @@ void security_fixup_ops (struct security
#endif /* CONFIG_SECURITY_NETWORK_XFRM */
#ifdef CONFIG_KEYS
set_to_dummy_if_null(ops, key_alloc);
+ set_to_dummy_if_null(ops, key_post_alloc);
set_to_dummy_if_null(ops, key_free);
set_to_dummy_if_null(ops, key_permission);
#endif /* CONFIG_KEYS */
diff --git a/security/keys/key.c b/security/keys/key.c
index a057e33..b72ffe2 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -231,6 +231,8 @@ static inline void key_alloc_serial(stru
insert_here:
rb_link_node(&key->serial_node, parent, p);
rb_insert_color(&key->serial_node, &key_serial_tree);
+ /* let the security module know the key has been published */
+ security_key_post_alloc(key);
spin_unlock(&key_serial_lock);
On Tue, 2006-03-28 at 07:53 -0600, Serge E. Hallyn wrote:
> Quoting Stephen Smalley ([email protected]):
> > Are you sure that the key cannot be accessed (looked up) by another
> > process as soon as it is assigned a serial number? If it can be, then
> > you risk having it accessed before its security structure is set up.
>
> Ah, that makes sense, and even rings a bell.
>
> So if we were to add a post_alloc() hook, it should likely go into
> key_alloc_serial() under the key_serial_lock?
>
> Still assuming that storing the serial number is desirable...
I'm not sure how/why SELinux would want that information, as we would
just be labeling the key based on its creator (possibly via a transition
computation to allow derived types), and then later possibly support
explicit labeling by security-aware applications as permitted by policy.
Serial number wouldn't be used for access control, and audit is being
handled separately these days (e.g. one might introduce audit hooks to
collect the serial number at the right points for later inclusion in
audit records emitted at syscall exit).
--
Stephen Smalley
National Security Agency