Stephen Smalley wrote:
> On Thu, 2008-08-14 at 16:38 +0900, KaiGai Kohei wrote:
>> The following patch is the revised one for the kernel.
>>
>> Joshua suggested it is not necessary to bound type attributes
>> based on boundary relationship, and I also agreed this opinion.
>> The related code is dropped in this patch, and several code
>> cleanup contained.
>>
>> This change makes it unnecessary to deliver text represented
>> attributes into kernelspace, but related codes are still remained
>> for ...
>>
>> | Stephen Smalley wrote:
>> | An aside: Possibly we should add the attribute flag at the same time
>> | and leave the type attributes in the type symtab in the kernel policy,
>> | as previously discussed. Then the kernel policy won't be lacking any
>> | information needed for analysis. Just need to make sure that
>> | context_to_sid doesn't accept attributes in the type field.
>>
>> Stephen, could you point out the previous discussion?
>
> Keeping the type attribute names in the types symtab in the kernel
> policy allows tools like audit2why and apol to extract the original
> attribute names and display them. I originally shed them because the
> kernel didn't need to use that information and we don't want attribute
> names to be used in security contexts, but it costs us little to save
> them and check for them, and it benefits the tools.
>
>> Thanks,
>>
>> [1/3] thread-context-kernel.4.patch
>> It allows a multithreaded process to assign an individual
>> "more bounded" security context, and it also enables to
>> handle binary policy format version 24 in kernel space.
>>
>> Signed-off-by: KaiGai Kohei <kaigai@xxxxxxxxxxxxx>
>> --
>> security/selinux/avc.c | 2 +-
>> security/selinux/hooks.c | 11 +-
>> security/selinux/include/avc.h | 4 +
>> security/selinux/include/security.h | 16 ++-
>> security/selinux/ss/policydb.c | 298 +++++++++++++++++++++++++++++++++--
>> security/selinux/ss/policydb.h | 5 +
>> security/selinux/ss/services.c | 227 +++++++++++++++++++++++----
>> 7 files changed, 515 insertions(+), 48 deletions(-)
>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index bc1c3ae..0201227 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -5180,9 +5180,14 @@ static int selinux_setprocattr(struct task_struct *p,
>>
>> if (sid == 0)
>> return -EINVAL;
>> -
>> - /* Only allow single threaded processes to change context */
>> - if (atomic_read(&p->mm->mm_users) != 1) {
>> + /*
>> + * SELinux allows to change context in the following case only.
>> + * - Single threaded processes.
>> + * - Multi threaded processes intend to change its context into
>> + * more restricted domain (defined by TYPEBOUNDS statement).
>> + */
>> + if (atomic_read(&p->mm->mm_users) != 1
>> + && security_bounded_transition(tsec->sid, sid) != 0) {
>
> Alternatively you could defer the security_bounded_transition() check
> until the if (t->mm == mm && t != p) block. The mm_users check is a
> merely a quick check to see whether we need to probe further to
> determine whether or not there are any other threads sharing the mm.
The reason why I moved it to front of the block is we can skip the checks
whether the given process is multithreaded or not, if security_bounded_transition()
returns success.
However, I have no preference here. There may be a bit performance gain, but this
code is not invoked so frequently.
So, I'll revert it on the next revision.
>> @@ -62,6 +63,17 @@ enum {
>> extern int selinux_policycap_netpeer;
>> extern int selinux_policycap_openperm;
>>
>> +/* boundary related definitions */
>> +#define POLICYDB_BOUNDS_MAXDEPTH 4
>> +#define POLICYDB_BOUNDS_ATTRIBUTE_FLAG 0xffffffff
>> +/*
>> + * NOTE: When "bounds" field equals POLICYDB_BOUNDS_ATTRIBUTE_FLAG,
>> + * it means this entry is attribute, not a normal type.
>> + * Boundary feature also requires to deliver attribute info into
>> + * kernel space, because it has to notice boundary violation related
>> + * to type/attribute relationships.
>> + */
>
> It seems a bit ugly to overload the bounds field for this purpose vs.
> encoding it into the primary field as you suggest elsewhere.
OK, I'll encode these properties of type_datum into the primary field.
>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
>> index 84f8cc7..f134322 100644
>> --- a/security/selinux/ss/policydb.c
>> +++ b/security/selinux/ss/policydb.c
> <snip>
>> +static int constraint_bounds_sanity_check(struct policydb *p,
>> + struct constraint_expr *cexpr)
>> +{
>> + struct user_datum *user;
>> + struct role_datum *role;
>> + struct type_datum *type;
>> + struct ebitmap_node *node;
>> + unsigned long bit;
>> +
>> + /* no need to check boundary constraint */
>> + if (cexpr->expr_type != CEXPR_NAMES)
>> + return 0;
>> +
>> + ebitmap_for_each_positive_bit(&cexpr->names, node, bit) {
>> + if (cexpr->attr & CEXPR_USER) {
>> + if (bit >= p->p_users.nprim)
>> + goto broken_ebitmap;
>> +
>> + user = p->user_val_to_struct[bit];
>> + if (user->bounds &&
>> + !ebitmap_get_bit(&cexpr->names, user->bounds - 1)) {
>> + printk(KERN_ERR
>> + "SELinux: boundary violated cexpr:"
>> + " CEXPR_NAMES: user:%s bounds:%s\n",
>> + p->p_user_val_to_name[user->value - 1],
>> + p->p_user_val_to_name[user->bounds - 1]);
>> + return -EINVAL;
>> + }
>
> What if cexpr->op is CEXPR_NEQ, e.g. the clause was something like
> (u1 != untrusteduser)? Or the entire clause is negated? I'm not sure
> you want to apply these bounds checks on constraints at all.
CEXPR_NEQ is dealt as simply negated ebitmap in the current implementation.
For example, (u1 != untrusteduser) requires a user is not "untrusteduser"
and its parent is not "untrusteduser" also.
Hmm, however, it may be a bit complicated/hard to understand rule.
The purpose of this check is to prevent to attach wider permissions on
bounded types, like mcssetcats attribute which means a privilege.
Here is an idea.
If the type_attribute_bounds_av() is put after constraint checks on avc
computing, we can make sure the purpose of bounds checks. It simply means
a bounded type cannot have wider permissions than its parent in TE rules
and constraints.
Thanks,
--
KaiGai Kohei <kaigai@xxxxxxxxxxxx>
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
