[BUG] Segfault on duplicate require of sensitivity

[BUG] Segfault on duplicate require of sensitivity

[B Topscher]

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

When I have sensitivity required in two different locations I get
segmentation faults when I try and load the module.  For example, because s0
and s15 are already declared on other files if I require them in the TE file
I get a segfault.  I looked in the module.tmp file that was created on build
and saw that s0 and s15 are declared somewhere.  However, if I comment out
my require in the TE file it loads the module fine.


if in the TE I have:

require {
      sensitivity s0;
}

function( domain_t )

and the IF I have

interface(`function',`
              gen_require(`
                          sensitivity s0;
                          ')
              .......
')

When I build and then semodule -i module.pp,  I get a segfault when
committing changes (according to semodule -v).

Thank you
Bryan

[-- Attachment #2: Type: text/html, Size: 1374 bytes --]

Re: [BUG] Segfault on duplicate require of sensitivity

[Caleb Case]

It turns out that level_datum_t is not defined as an actual datum:

/* Sensitivity attributes */
typedef struct level_datum {
        mls_level_t *level;     /* sensitivity and associated categories
*/
        unsigned char isalias;  /* is this sensitivity an alias for
another? */
        unsigned char defined;
} level_datum_t;

All *_datum_t should have the form:

typedef struct *_datum {
        symtab_datum_t s;
        *
} *_datum_t;

This assumption causes problems in module_compiler.c:require_symbol when
the symbol is a duplicate (retval == 1 means duplicate symbol):

int require_symbol(uint32_t symbol_type,
                   hashtab_key_t key, hashtab_datum_t datum,
                   uint32_t * dest_value, uint32_t * datum_value)
{

<snip>

        if (retval == 1) {
                symtab_datum_t *s =
                    (symtab_datum_t *) hashtab_search(policydbp->

symtab[symbol_type].table,
                                                      key);
                assert(s != NULL);
                *dest_value = s->value;
        } else if (retval == -2) {

<snip>

}

Which results in *dest_value being the address of level_datum->level and
not the value of the sens (which would be level_datum->level->sens.  See
module_compiler.c:require_sens for more context.

The options I see here are not good.  One option: the level_datum_t
should be changed into a conforming *_datum_t and the fallout of this
change handled in the rest of the code which expects to see a
level_datum_t->level.  Second option: level_datum_t is treated specially
in require_symbol (using the symbol_type as the switch).

On Thu, 2007-04-19 at 11:29 -0400, B Topscher wrote:
> When I have sensitivity required in two different locations I get
> segmentation faults when I try and load the module.  For example,
> because s0 and s15 are already declared on other files if I require
> them in the TE file I get a segfault.  I looked in the module.tmp file
> that was created on build and saw that s0 and s15 are declared
> somewhere.  However, if I comment out my require in the TE file it
> loads the module fine.  
> 
> 
> if in the TE I have:
> 
> require { 
>       sensitivity s0;
> }
> 
> function( domain_t )
> 
> and the IF I have 
> 
> interface(`function',`
>               gen_require(`
>                           sensitivity s0;
>                           ') 
>               .......
> ')
> 
> When I build and then semodule -i module.pp,  I get a segfault when
> committing changes (according to semodule -v).  
> 
> Thank you
> Bryan 
-- 
Caleb Case
Tresys Technology
410-290-1411 x144

--
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: [BUG] Segfault on duplicate require of sensitivity

[Karl MacMillan]

On Tue, 2007-05-15 at 10:16 -0400, Caleb Case wrote:
> It turns out that level_datum_t is not defined as an actual datum:
> 

[...]

> 
> The options I see here are not good.  One option: the level_datum_t
> should be changed into a conforming *_datum_t and the fallout of this
> change handled in the rest of the code which expects to see a
> level_datum_t->level.  Second option: level_datum_t is treated specially
> in require_symbol (using the symbol_type as the switch).
> 

Making it a _datum_t seems to be the right choice - what is your concern
about following that path?

Karl

> On Thu, 2007-04-19 at 11:29 -0400, B Topscher wrote:
> > When I have sensitivity required in two different locations I get
> > segmentation faults when I try and load the module.  For example,
> > because s0 and s15 are already declared on other files if I require
> > them in the TE file I get a segfault.  I looked in the module.tmp file
> > that was created on build and saw that s0 and s15 are declared
> > somewhere.  However, if I comment out my require in the TE file it
> > loads the module fine.  
> > 
> > 
> > if in the TE I have:
> > 
> > require { 
> >       sensitivity s0;
> > }
> > 
> > function( domain_t )
> > 
> > and the IF I have 
> > 
> > interface(`function',`
> >               gen_require(`
> >                           sensitivity s0;
> >                           ') 
> >               .......
> > ')
> > 
> > When I build and then semodule -i module.pp,  I get a segfault when
> > committing changes (according to semodule -v).  
> > 
> > Thank you
> > Bryan 


--
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: [BUG] Segfault on duplicate require of sensitivity

[Caleb Case]

On Tue, 2007-05-15 at 10:39 -0400, Karl MacMillan wrote:
> On Tue, 2007-05-15 at 10:16 -0400, Caleb Case wrote:
> > It turns out that level_datum_t is not defined as an actual datum:
> > 
> 
> [...]
> 
> > 
> > The options I see here are not good.  One option: the level_datum_t
> > should be changed into a conforming *_datum_t and the fallout of this
> > change handled in the rest of the code which expects to see a
> > level_datum_t->level.  Second option: level_datum_t is treated specially
> > in require_symbol (using the symbol_type as the switch).
> > 
> 
> Making it a _datum_t seems to be the right choice - what is your concern
> about following that path?
> 
> Karl

Mainly I am concerned because level_datum_t is exported in libsepol's
protected headers and will require changes to anything that statically
links to libsepol.

> 
> > On Thu, 2007-04-19 at 11:29 -0400, B Topscher wrote:
> > > When I have sensitivity required in two different locations I get
> > > segmentation faults when I try and load the module.  For example,
> > > because s0 and s15 are already declared on other files if I require
> > > them in the TE file I get a segfault.  I looked in the module.tmp file
> > > that was created on build and saw that s0 and s15 are declared
> > > somewhere.  However, if I comment out my require in the TE file it
> > > loads the module fine.  
> > > 
> > > 
> > > if in the TE I have:
> > > 
> > > require { 
> > >       sensitivity s0;
> > > }
> > > 
> > > function( domain_t )
> > > 
> > > and the IF I have 
> > > 
> > > interface(`function',`
> > >               gen_require(`
> > >                           sensitivity s0;
> > >                           ') 
> > >               .......
> > > ')
> > > 
> > > When I build and then semodule -i module.pp,  I get a segfault when
> > > committing changes (according to semodule -v).  
> > > 
> > > Thank you
> > > Bryan 
> 
-- 
Caleb Case
Tresys Technology
410-290-1411 x144

--
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: [BUG] Segfault on duplicate require of sensitivity

[Joshua Brindle]

Caleb Case wrote:
> On Tue, 2007-05-15 at 10:39 -0400, Karl MacMillan wrote:
>   
>> On Tue, 2007-05-15 at 10:16 -0400, Caleb Case wrote:
>>     
>>> It turns out that level_datum_t is not defined as an actual datum:
>>>
>>>       
>> [...]
>>
>>     
>>> The options I see here are not good.  One option: the level_datum_t
>>> should be changed into a conforming *_datum_t and the fallout of this
>>> change handled in the rest of the code which expects to see a
>>> level_datum_t->level.  Second option: level_datum_t is treated specially
>>> in require_symbol (using the symbol_type as the switch).
>>>
>>>       
>> Making it a _datum_t seems to be the right choice - what is your concern
>> about following that path?
>>
>> Karl
>>     
>
> Mainly I am concerned because level_datum_t is exported in libsepol's
> protected headers and will require changes to anything that statically
> links to libsepol.
>
>   
Err, I don't think this is the main issue. The level datum references 
the sens_datum, which exists independantly of the level_datum. I think 
it would cause all sorts of problems to try to change that in the 
current code base.

Another option is to just punt on this and it should be handled 
naturally in the policyrep branch.

--
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: [BUG] Segfault on duplicate require of sensitivity

[Karl MacMillan]

On Tue, 2007-05-15 at 13:18 -0400, Joshua Brindle wrote:
> Caleb Case wrote:
> > On Tue, 2007-05-15 at 10:39 -0400, Karl MacMillan wrote:
> >   
> >> On Tue, 2007-05-15 at 10:16 -0400, Caleb Case wrote:
> >>     
> >>> It turns out that level_datum_t is not defined as an actual datum:
> >>>
> >>>       
> >> [...]
> >>
> >>     
> >>> The options I see here are not good.  One option: the level_datum_t
> >>> should be changed into a conforming *_datum_t and the fallout of this
> >>> change handled in the rest of the code which expects to see a
> >>> level_datum_t->level.  Second option: level_datum_t is treated specially
> >>> in require_symbol (using the symbol_type as the switch).
> >>>
> >>>       
> >> Making it a _datum_t seems to be the right choice - what is your concern
> >> about following that path?
> >>
> >> Karl
> >>     
> >
> > Mainly I am concerned because level_datum_t is exported in libsepol's
> > protected headers and will require changes to anything that statically
> > links to libsepol.
> >
> >   
> Err, I don't think this is the main issue. The level datum references 
> the sens_datum, which exists independantly of the level_datum. I think 
> it would cause all sorts of problems to try to change that in the 
> current code base.
> 

What kind of problems?

> Another option is to just punt on this and it should be handled 
> naturally in the policyrep branch.

We can't punt on a reproducible segfault - it needs to be fixed in
stable.

Karl


--
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: [BUG] Segfault on duplicate require of sensitivity

[Joshua Brindle]

Karl MacMillan wrote:
> On Tue, 2007-05-15 at 13:18 -0400, Joshua Brindle wrote:
>   
>> Caleb Case wrote:
>>     
>>> On Tue, 2007-05-15 at 10:39 -0400, Karl MacMillan wrote:
>>>   
>>>       
>>>> On Tue, 2007-05-15 at 10:16 -0400, Caleb Case wrote:
>>>>     
>>>>         
>>>>> It turns out that level_datum_t is not defined as an actual datum:
>>>>>
>>>>>       
>>>>>           
>>>> [...]
>>>>
>>>>     
>>>>         
>>>>> The options I see here are not good.  One option: the level_datum_t
>>>>> should be changed into a conforming *_datum_t and the fallout of this
>>>>> change handled in the rest of the code which expects to see a
>>>>> level_datum_t->level.  Second option: level_datum_t is treated specially
>>>>> in require_symbol (using the symbol_type as the switch).
>>>>>
>>>>>       
>>>>>           
>>>> Making it a _datum_t seems to be the right choice - what is your concern
>>>> about following that path?
>>>>
>>>> Karl
>>>>     
>>>>         
>>> Mainly I am concerned because level_datum_t is exported in libsepol's
>>> protected headers and will require changes to anything that statically
>>> links to libsepol.
>>>
>>>   
>>>       
>> Err, I don't think this is the main issue. The level datum references 
>> the sens_datum, which exists independantly of the level_datum. I think 
>> it would cause all sorts of problems to try to change that in the 
>> current code base.
>>
>>     
>
> What kind of problems?
>
>   
What do we put in the symtab_datum? Do we reproduce what is in the 
sens_datum? They aren't the same data, I'm not totally sure why they are 
separated (Darrell, want to chime in on this?). The really crappy thing 
is that we decided to support mls in require statements by just using 
the whole string (eg., s0:c0.c126-s15:c0.c128) so that whole string gets 
a level datum (iirc). I guess we can try adding a symtab_datum and see 
what the fallout is but I think there are alot of assumptions about how 
level_datum and sens_datum relate.

>> Another option is to just punt on this and it should be handled 
>> naturally in the policyrep branch.
>>     
>
> We can't punt on a reproducible segfault - it needs to be fixed in
> stable.
>   
I know, I was half joking...

--
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: [BUG] Segfault on duplicate require of sensitivity

[Caleb Case]

On Tue, 2007-05-15 at 13:18 -0400, Joshua Brindle wrote:
> Caleb Case wrote:
> > On Tue, 2007-05-15 at 10:39 -0400, Karl MacMillan wrote:
> >   
> >> On Tue, 2007-05-15 at 10:16 -0400, Caleb Case wrote:
> >>     
> >>> It turns out that level_datum_t is not defined as an actual datum:
> >>>
> >>>       
> >> [...]
> >>
> >>     
> >>> The options I see here are not good.  One option: the level_datum_t
> >>> should be changed into a conforming *_datum_t and the fallout of this
> >>> change handled in the rest of the code which expects to see a
> >>> level_datum_t->level.  Second option: level_datum_t is treated specially
> >>> in require_symbol (using the symbol_type as the switch).
> >>>
> >>>       
> >> Making it a _datum_t seems to be the right choice - what is your concern
> >> about following that path?
> >>
> >> Karl
> >>     
> >
> > Mainly I am concerned because level_datum_t is exported in libsepol's
> > protected headers and will require changes to anything that statically
> > links to libsepol.
> >
> >   
> Err, I don't think this is the main issue. The level datum references 
> the sens_datum, which exists independantly of the level_datum. I think 
> it would cause all sorts of problems to try to change that in the 
> current code base.
> 
> Another option is to just punt on this and it should be handled 
> naturally in the policyrep branch.

This is option 2: special case the level_datum_t handling.

Index: checkpolicy/module_compiler.c
===================================================================
--- checkpolicy/module_compiler.c	(revision 2421)
+++ checkpolicy/module_compiler.c	(working copy)
@@ -142,7 +142,12 @@
 						      symtab[symbol_type].table,
 						      key);
 		assert(s != NULL);
-		*dest_value = s->value;
+		
+		if (symbol_type == SYM_LEVELS) {
+			*dest_value = ((level_datum_t *)s)->level->sens;
+		} else {
+			*dest_value = s->value;
+		}
 	} else if (retval == -2) {
 		return -2;
 	} else if (retval < 0) {
@@ -496,7 +501,12 @@
 						      symtab[symbol_type].table,
 						      key);
 		assert(s != NULL);
-		*dest_value = s->value;
+		
+		if (symbol_type == SYM_LEVELS) {
+			*dest_value = ((level_datum_t *)s)->level->sens;
+		} else {
+			*dest_value = s->value;
+		}
 	} else if (retval == -2) {
 		/* ignore require statements if that symbol was
 		 * previously declared and is in current scope */

-- 
Caleb Case
Tresys Technology
410-290-1411 x144

--
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: [BUG] Segfault on duplicate require of sensitivity

[Stephen Smalley]

On Fri, 2007-05-25 at 13:26 -0400, Caleb Case wrote:
> On Tue, 2007-05-15 at 13:18 -0400, Joshua Brindle wrote:
> > Caleb Case wrote:
> > > On Tue, 2007-05-15 at 10:39 -0400, Karl MacMillan wrote:
> > >   
> > >> On Tue, 2007-05-15 at 10:16 -0400, Caleb Case wrote:
> > >>     
> > >>> It turns out that level_datum_t is not defined as an actual datum:
> > >>>
> > >>>       
> > >> [...]
> > >>
> > >>     
> > >>> The options I see here are not good.  One option: the level_datum_t
> > >>> should be changed into a conforming *_datum_t and the fallout of this
> > >>> change handled in the rest of the code which expects to see a
> > >>> level_datum_t->level.  Second option: level_datum_t is treated specially
> > >>> in require_symbol (using the symbol_type as the switch).
> > >>>
> > >>>       
> > >> Making it a _datum_t seems to be the right choice - what is your concern
> > >> about following that path?
> > >>
> > >> Karl
> > >>     
> > >
> > > Mainly I am concerned because level_datum_t is exported in libsepol's
> > > protected headers and will require changes to anything that statically
> > > links to libsepol.
> > >
> > >   
> > Err, I don't think this is the main issue. The level datum references 
> > the sens_datum, which exists independantly of the level_datum. I think 
> > it would cause all sorts of problems to try to change that in the 
> > current code base.
> > 
> > Another option is to just punt on this and it should be handled 
> > naturally in the policyrep branch.
> 
> This is option 2: special case the level_datum_t handling.
> 
> Index: checkpolicy/module_compiler.c
> ===================================================================
> --- checkpolicy/module_compiler.c	(revision 2421)
> +++ checkpolicy/module_compiler.c	(working copy)
> @@ -142,7 +142,12 @@
>  						      symtab[symbol_type].table,
>  						      key);
>  		assert(s != NULL);
> -		*dest_value = s->value;
> +		
> +		if (symbol_type == SYM_LEVELS) {
> +			*dest_value = ((level_datum_t *)s)->level->sens;
> +		} else {
> +			*dest_value = s->value;
> +		}
>  	} else if (retval == -2) {
>  		return -2;
>  	} else if (retval < 0) {
> @@ -496,7 +501,12 @@
>  						      symtab[symbol_type].table,
>  						      key);
>  		assert(s != NULL);
> -		*dest_value = s->value;
> +		
> +		if (symbol_type == SYM_LEVELS) {
> +			*dest_value = ((level_datum_t *)s)->level->sens;
> +		} else {
> +			*dest_value = s->value;
> +		}
>  	} else if (retval == -2) {
>  		/* ignore require statements if that symbol was
>  		 * previously declared and is in current scope */

I'm merging this patch, but it occurs to me that this doesn't fully
solve the bug - it ensures that checkmodule will generate a well-formed
module, but it is still the case that an ill-formed module can cause a
segfault in libsepol at module link time, right?  We should be properly
checking those values during module link in libsepol too.

-- 
Stephen Smalley
National Security Agency


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