Re: [PATCH] libsepol: support policy modules when roletrans rules not supported

[Joshua Brindle <method@manicmethod.com>]

Eric Paris wrote:
> So I'm questioning the correctness of the range_transition and
> role_transition rules in libsepol.  My main problem is that libsepol
> defines SECCLASS_* at all.  Right now if the policy reads in one of
> these rules types without a tclass it will set SECCLASS_PROCESS in the
> tclasses bitmap.  But we never had any that would declare what the
> means.  At link time when we have to map "process" in a module to
> "process" in the base policy.  But if the module didn't require
> "process" it won't have the mapping.  So the fact that we set the
> SECCLASS_PROCESS bit could cause it to get mapped to random crap (or
> nothing at all)  Right?
>
> I know it's ugly but I think we need to do a couple of things.  #1 on
> that list is get SECCLASS_* out of libsepol altogether.  Those are
> COMPLETELY a construct of policy.  Not libsepol.  After we remove all
> of those we need to change the logic of everything that uses them to
> instead make sure that the "process" class exists in it's definitions
> and if not declare it.  Then set the bitmap for that new object.
>
> Am I not understanding something about how using SECCLASS_PROCESS
> could ever be a good idea?

It isn't the first time we've had hardcoded symbols that have to be mapped in 
the code. See OBJECT_R_VAL as an example.

It is a bit wonky though, suppose a type_transition rule were in a XenSE policy, 
I don't think they have a process object class.

>
> -Eric
>
> On Wed, Apr 13, 2011 at 2:07 PM, Eric Paris<eparis@redhat.com>  wrote:
>> On Wed, 2011-04-13 at 13:35 -0400, Steve Lawrence wrote:
>>> On 04/13/2011 11:23 AM, Steve Lawrence wrote:
>>>> Thanks for the patch. This solves the problem, but I'm still seeing
>>>> issues that seems related to support of older version of policy when
>>>> using role_transition with non-process classes. I'm now seeing an mls
>>>> range overflow error:
>>>>
>>>> libsepol.role_trans_write: Discarding role_transition rules for security
>>>> class other than "process"
>>>> libsepol.mls_read_range_helper: range overflow
>>>> libsepol.context_read_and_validate: error reading MLS range of context
>>>> libsepol.policydb_to_image: new policy image is invalid
>>>>
>>>> I'm still looking into it, but I'm not too familiar this part of the code.
>>>>
>>>>
>>>> On 04/12/2011 05:11 PM, Eric Paris wrote:
>>>>> Although the role trans code had support to handle the kernel policy
>>>>> when the version was less that roletrans such support was not in the
>>>>> module read/write code.  This patch adds proper support for role trans
>>>>> in modules.
>>>>>
>>>>> Signed-off-by: Eric Paris<eparis@redhat.com>
>>>>> ---
>>>>>   libsepol/src/policydb.c |   14 ++++++++++----
>>>>>   libsepol/src/write.c    |   37 ++++++++++++++++++++++++++++++++-----
>>>>>   2 files changed, 42 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
>>>>> index 2ecb636..6d8ff91 100644
>>>>> --- a/libsepol/src/policydb.c
>>>>> +++ b/libsepol/src/policydb.c
>>>>> @@ -3033,7 +3033,8 @@ int avrule_read_list(policydb_t * p, avrule_t ** avrules,
>>>>>     return 0;
>>>>>   }
>>>>>
>>>>> -static int role_trans_rule_read(role_trans_rule_t ** r, struct policy_file *fp)
>>>>> +static int role_trans_rule_read(policydb_t *p, role_trans_rule_t ** r,
>>>>> +                          struct policy_file *fp)
>>>>>   {
>>>>>     uint32_t buf[1], nel;
>>>>>     unsigned int i;
>>>>> @@ -3064,8 +3065,13 @@ static int role_trans_rule_read(role_trans_rule_t ** r, struct policy_file *fp)
>>>>>             if (type_set_read(&tr->types, fp))
>>>>>                     return -1;
>>>>>
>>>>> -          if (ebitmap_read(&tr->classes, fp))
>>>>> -                  return -1;
>>>>> +          if (p->policyvers>= MOD_POLICYDB_VERSION_ROLETRANS) {
>>>>> +                  if (ebitmap_read(&tr->classes, fp))
>>>>> +                          return -1;
>>>>> +          } else {
>>>>> +                  if (ebitmap_set_bit(&tr->classes, SECCLASS_PROCESS - 1, 1))
>>>>> +                          return -1;
>>>>> +          }
>>>>>
>>>>>             rc = next_entry(buf, fp, sizeof(uint32_t));
>>>>>             if (rc<  0)
>>>>> @@ -3258,7 +3264,7 @@ static int avrule_decl_read(policydb_t * p, avrule_decl_t * decl,
>>>>>     decl->enabled = le32_to_cpu(buf[1]);
>>>>>     if (cond_read_list(p,&decl->cond_list, fp) == -1 ||
>>>>>         avrule_read_list(p,&decl->avrules, fp) == -1 ||
>>>>> -      role_trans_rule_read(&decl->role_tr_rules, fp) == -1 ||
>>>>> +      role_trans_rule_read(p,&decl->role_tr_rules, fp) == -1 ||
>>>>>         role_allow_rule_read(&decl->role_allow_rules, fp) == -1) {
>>>>>             return -1;
>>>>>     }
>>>>> diff --git a/libsepol/src/write.c b/libsepol/src/write.c
>>>>> index c4f5035..78b3aa6 100644
>>>>> --- a/libsepol/src/write.c
>>>>> +++ b/libsepol/src/write.c
>>>>> @@ -1482,26 +1482,53 @@ static int avrule_write_list(avrule_t * avrules, struct policy_file *fp)
>>>>>     return POLICYDB_SUCCESS;
>>>>>   }
>>>>>
>>>>> -static int role_trans_rule_write(role_trans_rule_t * t, struct policy_file *fp)
>>>>> +static int only_process(ebitmap_t *in)
>>>>> +{
>>>>> +  unsigned int i;
>>>>> +  ebitmap_node_t *node;
>>>>> +
>>>>> +  ebitmap_for_each_bit(in, node, i) {
>>>>> +          if (ebitmap_node_get_bit(node, i)&&
>>>>> +              i != SECCLASS_PROCESS - 1)
>>>>> +                  return 0;
>>>>> +  }
>>>>> +  return 1;
>>>>> +}
>>>>> +
>>>>> +static int role_trans_rule_write(policydb_t *p, role_trans_rule_t * t,
>>>>> +                           struct policy_file *fp)
>>>>>   {
>>>>>     int nel = 0;
>>>>>     size_t items;
>>>>>     uint32_t buf[1];
>>>>>     role_trans_rule_t *tr;
>>>>> +  int warned = 0;
>>>>> +  int new_role = p->policyvers>= MOD_POLICYDB_VERSION_ROLETRANS;
>>>>>
>>>>>     for (tr = t; tr; tr = tr->next)
>>>>> -          nel++;
>>>>> +          if (new_role || only_process(&tr->classes))
>>>>> +                  nel++;
>>>>> +
>>>>>     buf[0] = cpu_to_le32(nel);
>>>>>     items = put_entry(buf, sizeof(uint32_t), 1, fp);
>>>>>     if (items != 1)
>>>>>             return POLICYDB_ERROR;
>>>>>     for (tr = t; tr; tr = tr->next) {
>>>>> +          if (!new_role&&  !only_process(&tr->classes)) {
>>>>> +                  if (!warned)
>>>>> +                          WARN(fp->handle, "Discarding role_transition "
>>>>> +                                  "rules for security classes other than "
>>>>> +                                  "\"process\"");
>>>>> +                  warned = 1;
>>>>> +                  continue;
>>>>> +          }
>>>>>             if (role_set_write(&tr->roles, fp))
>>>>>                     return POLICYDB_ERROR;
>>>>>             if (type_set_write(&tr->types, fp))
>>>>>                     return POLICYDB_ERROR;
>>>>> -          if (ebitmap_write(&tr->classes, fp))
>>>>> -                  return POLICYDB_ERROR;
>>>>> +          if (new_role)
>>>>> +                  if (ebitmap_write(&tr->classes, fp))
>>>>> +                          return POLICYDB_ERROR;
>>>>>             buf[0] = cpu_to_le32(tr->new_role);
>>>>>             items = put_entry(buf, sizeof(uint32_t), 1, fp);
>>>>>             if (items != 1)
>>>>> @@ -1636,7 +1663,7 @@ static int avrule_decl_write(avrule_decl_t * decl, int num_scope_syms,
>>>>>     }
>>>>>     if (cond_write_list(p, decl->cond_list, fp) == -1 ||
>>>>>         avrule_write_list(decl->avrules, fp) == -1 ||
>>>>> -      role_trans_rule_write(decl->role_tr_rules, fp) == -1 ||
>>>>> +      role_trans_rule_write(p, decl->role_tr_rules, fp) == -1 ||
>>>>>         role_allow_rule_write(decl->role_allow_rules, fp) == -1) {
>>>>>             return POLICYDB_ERROR;
>>>>>     }
>>>>
>>>> --
>>>> This message was distributed to subscribers of the selinux mailing list.
>>>> If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
>>>> the words "unsubscribe selinux" without quotes as the message.
>>> I think the below patch fixes the problem I was seeing. Some rules were
>>> getting discarded in role_trans_write during a policy downgrade, but the
>>> count was not being decreased. This fix is similar to your fix in
>>> role_trans_rule_write.
>> Looks as appropriate to me as when range trans does it.
>>
>> -Eric
>>
>>> ---
>>> diff --git a/libsepol/src/write.c b/libsepol/src/write.c
>>> index 78b3aa6..3a9c35a 100644
>>> --- a/libsepol/src/write.c
>>> +++ b/libsepol/src/write.c
>>> @@ -474,7 +474,9 @@ static int role_trans_write(policydb_t *p, struct
>>> policy_file *fp)
>>>
>>>      nel = 0;
>>>      for (tr = r; tr; tr = tr->next)
>>> -       nel++;
>>> +       if(new_roletr || tr->class == SECCLASS_PROCESS)
>>> +           nel++;
>>> +
>>>      buf[0] = cpu_to_le32(nel);
>>>      items = put_entry(buf, sizeof(uint32_t), 1, fp);
>>>      if (items != 1)
>>
>>
>> --
>> This message was distributed to subscribers of the selinux mailing list.
>> If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
>> the words "unsubscribe selinux" without quotes as the message.
>>
>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.