v0 Only call role_fix_callback for base.p_roles during expansion

v0 Only call role_fix_callback for base.p_roles during expansion

[Harry Ciao]

For analysis and tests, please refer to my reply to Chris' original email
when he discovers this problem in the first place.

Thanks,
Harry

--
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.

[v0 PATCH 1/1] Only call role_fix_callback for base.p_roles during expansion.

[Harry Ciao]

expand_role_attributes() would merge the sub role attribute's roles
ebitmap into that of the parent, then clear it off from the parent's
roles ebitmap. This supports the assertion in role_fix_callback() that
any role attribute's roles ebitmap contains just regular roles.

expand_role_attribute() works on base.p_roles table but not any
block/decl's p_roles table, so the above assertion in role_fix_callback
could fail when it is called for block/decl and some role attribute is
added into another.

Since the effect of get_local_role() would have been complemented by
the populate_roleattributes() at the end of the link phase, there is
no needs(and wrong) to call role_fix_callback() for block/decl in the
expand phase.

Signed-off-by: Harry Ciao <qingtao.cao@windriver.com>
---
 libsepol/src/expand.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index b42acbe..96ed473 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -2832,9 +2832,6 @@ int expand_module(sepol_handle_t * handle,
 		if (hashtab_map
 		    (decl->p_roles.table, role_copy_callback, &state))
 			goto cleanup;
-		if (hashtab_map
-		    (decl->p_roles.table, role_fix_callback, &state))
-			goto cleanup;
 
 		/* copy users */
 		if (hashtab_map
-- 
1.7.0.4


--
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.

Re: [v0 PATCH 1/1] Only call role_fix_callback for base.p_roles during expansion.

[Steve Lawrence]

On 08/02/2011 06:03 AM, Harry Ciao wrote:
> expand_role_attributes() would merge the sub role attribute's roles
> ebitmap into that of the parent, then clear it off from the parent's
> roles ebitmap. This supports the assertion in role_fix_callback() that
> any role attribute's roles ebitmap contains just regular roles.
> 
> expand_role_attribute() works on base.p_roles table but not any
> block/decl's p_roles table, so the above assertion in role_fix_callback
> could fail when it is called for block/decl and some role attribute is
> added into another.
> 
> Since the effect of get_local_role() would have been complemented by
> the populate_roleattributes() at the end of the link phase, there is
> no needs(and wrong) to call role_fix_callback() for block/decl in the
> expand phase.
> 
> Signed-off-by: Harry Ciao <qingtao.cao@windriver.com>
> ---
>  libsepol/src/expand.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index b42acbe..96ed473 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -2832,9 +2832,6 @@ int expand_module(sepol_handle_t * handle,
>  		if (hashtab_map
>  		    (decl->p_roles.table, role_copy_callback, &state))
>  			goto cleanup;
> -		if (hashtab_map
> -		    (decl->p_roles.table, role_fix_callback, &state))
> -			goto cleanup;
>  
>  		/* copy users */
>  		if (hashtab_map

This looks good, and I'm fine with committing this.

However, I did find what appears to be an unrelated problem. It looks
like the role attributes are getting written to the policy db as if they
were roles. I don't think this will break anything (I think), but
considering that the kernel doesn't know anything about role_attributes,
it seems odd to me that they are in the binary.

Note: I found this by looking at a downgraded policy.24 in apol, so this
could potentially be a downgrade issue. But from looking at the code, I
believe role attributes are being written as if they're roles.

- Steve

--
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.

Re: [v0 PATCH 1/1] Only call role_fix_callback for base.p_roles during expansion.

[Eric Paris]

Applied in master.

On Thu, Aug 11, 2011 at 10:05 AM, Steve Lawrence <slawrence@tresys.com> wrote:
> On 08/02/2011 06:03 AM, Harry Ciao wrote:
>> expand_role_attributes() would merge the sub role attribute's roles
>> ebitmap into that of the parent, then clear it off from the parent's
>> roles ebitmap. This supports the assertion in role_fix_callback() that
>> any role attribute's roles ebitmap contains just regular roles.
>>
>> expand_role_attribute() works on base.p_roles table but not any
>> block/decl's p_roles table, so the above assertion in role_fix_callback
>> could fail when it is called for block/decl and some role attribute is
>> added into another.
>>
>> Since the effect of get_local_role() would have been complemented by
>> the populate_roleattributes() at the end of the link phase, there is
>> no needs(and wrong) to call role_fix_callback() for block/decl in the
>> expand phase.
>>
>> Signed-off-by: Harry Ciao <qingtao.cao@windriver.com>
>> ---
>>  libsepol/src/expand.c |    3 ---
>>  1 files changed, 0 insertions(+), 3 deletions(-)
>>
>> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
>> index b42acbe..96ed473 100644
>> --- a/libsepol/src/expand.c
>> +++ b/libsepol/src/expand.c
>> @@ -2832,9 +2832,6 @@ int expand_module(sepol_handle_t * handle,
>>               if (hashtab_map
>>                   (decl->p_roles.table, role_copy_callback, &state))
>>                       goto cleanup;
>> -             if (hashtab_map
>> -                 (decl->p_roles.table, role_fix_callback, &state))
>> -                     goto cleanup;
>>
>>               /* copy users */
>>               if (hashtab_map
>
> This looks good, and I'm fine with committing this.
>
> However, I did find what appears to be an unrelated problem. It looks
> like the role attributes are getting written to the policy db as if they
> were roles. I don't think this will break anything (I think), but
> considering that the kernel doesn't know anything about role_attributes,
> it seems odd to me that they are in the binary.
>
> Note: I found this by looking at a downgraded policy.24 in apol, so this
> could potentially be a downgrade issue. But from looking at the code, I
> believe role attributes are being written as if they're roles.
>
> - Steve
>
> --
> 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.

Re: [v0 PATCH 1/1] Only call role_fix_callback for base.p_roles during expansion.

[Harry Ciao]

Hi Steve,

Steve Lawrence 写道:
> However, I did find what appears to be an unrelated problem. It looks
> like the role attributes are getting written to the policy db as if they
> were roles. I don't think this will break anything (I think), but
> considering that the kernel doesn't know anything about role_attributes,
> it seems odd to me that they are in the binary.
>
> Note: I found this by looking at a downgraded policy.24 in apol, so this
> could potentially be a downgrade issue. But from looking at the code, I
> believe role attributes are being written as if they're roles.
>
> - Steve
>
>   
>
You are right!

The role attribute's destination would have been fulfilled at the expand
stage when its types.types ebitmap populated to all its sub regular
roles, thus there is no need to write role attribute's role_datum_t to
policy.X at all. This won't cause any harm, but redundant.

We could bail out from role_write() when finding out the current datum
is a role attribute while writing to policy.X. I would send out a patch
later today.

BTW, I'd also noticed role attribute by apol but I didn't realize what
you have realized, so it's always beneficial to have others review your
patches :-)

Thanks!

Cheers,
Harry

--
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.

Re: [v0 PATCH 1/1] Only call role_fix_callback for base.p_roles during expansion.

[Joshua Brindle]

Harry Ciao wrote:
> Hi Steve,
>
> Steve Lawrence 写道:
>> However, I did find what appears to be an unrelated problem. It looks
>> like the role attributes are getting written to the policy db as if they
>> were roles. I don't think this will break anything (I think), but
>> considering that the kernel doesn't know anything about role_attributes,
>> it seems odd to me that they are in the binary.
>>
>> Note: I found this by looking at a downgraded policy.24 in apol, so this
>> could potentially be a downgrade issue. But from looking at the code, I
>> believe role attributes are being written as if they're roles.
>>
>> - Steve
>>
>>
>>
> You are right!
>
> The role attribute's destination would have been fulfilled at the expand
> stage when its types.types ebitmap populated to all its sub regular
> roles, thus there is no need to write role attribute's role_datum_t to
> policy.X at all. This won't cause any harm, but redundant.
>
> We could bail out from role_write() when finding out the current datum
> is a role attribute while writing to policy.X. I would send out a patch
> later today.
>
> BTW, I'd also noticed role attribute by apol but I didn't realize what
> you have realized, so it's always beneficial to have others review your
> patches :-)
>

When downgrading a policy I believe the downgraded policy should be 
identical (e.g., binary diffable or very close if not possible) to the 
older toolchain. In this case I don't see a reason why the downgraded 
policy should have the role_attributes in the role symtab. There should 
be a patch to correctly discard them when downgrading IMO.

--
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.

Re: [v0 PATCH 1/1] Only call role_fix_callback for base.p_roles during expansion.

[Harry Ciao]

Hi Joshua,

Joshua Brindle 写道:
> Harry Ciao wrote:
>> Hi Steve,
>>
>> Steve Lawrence 写道:
>>> However, I did find what appears to be an unrelated problem. It looks
>>> like the role attributes are getting written to the policy db as if
>>> they
>>> were roles. I don't think this will break anything (I think), but
>>> considering that the kernel doesn't know anything about
>>> role_attributes,
>>> it seems odd to me that they are in the binary.
>>>
>>> Note: I found this by looking at a downgraded policy.24 in apol, so
>>> this
>>> could potentially be a downgrade issue. But from looking at the code, I
>>> believe role attributes are being written as if they're roles.
>>>
>>> - Steve
>>>
>>>
>>>
>> You are right!
>>
>> The role attribute's destination would have been fulfilled at the expand
>> stage when its types.types ebitmap populated to all its sub regular
>> roles, thus there is no need to write role attribute's role_datum_t to
>> policy.X at all. This won't cause any harm, but redundant.
>>
>> We could bail out from role_write() when finding out the current datum
>> is a role attribute while writing to policy.X. I would send out a patch
>> later today.
>>
>> BTW, I'd also noticed role attribute by apol but I didn't realize what
>> you have realized, so it's always beneficial to have others review your
>> patches :-)
>>
>
> When downgrading a policy I believe the downgraded policy should be
> identical (e.g., binary diffable or very close if not possible) to the
> older toolchain. In this case I don't see a reason why the downgraded
> policy should have the role_attributes in the role symtab. There
> should be a patch to correctly discard them when downgrading IMO.
>
I am creating a patch to skip role attributes from writing to policy.X,
so there won't be any role attributes contained in policy.X, no matter
what X is. Even if a policy downgrade happens while loading policy.X, we
don't have to worry about discarding role attributes at all, since it
won't exist in policy.X anyway:-)

Thanks,
Harry

--
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.