| Message ID | 1484772829-15960-1-git-send-email-jwcart2@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 01/18/2017 09:53 PM, James Carter wrote: > Nicolas Iooss discovered that requiring a type in an optional block > after the type has already been declared in another optional block > results in a duplicate declaration error. > from what i have been told and from experience, types cannot, reliably, be declared in optional blocks. if the above is true, then the compiler should not allow one to declare a type in an optional block in the first place > The following policy will trigger the error. > > optional { > type T1; > } > > optional { > require { type T1; } > } > > In this case, although symtab_insert() in libsepol properly identifies > that the first T1 is a declaration while the second is a require, it > will return -2 which is the return value for a duplicate declaration > and expect the calling code to properly handle the situation. The > caller is require_symbol() in checkpolicy and it checks if the previous > declaration is in the current scope. If it is, then the require can be > ignored. It also checks to see if the declaration is a type and the > require an attribute or vice versa which is an error. The problem is > that -2 is returned if the declaration is not in scope which is > interpreted as a duplicate declaration error. > > The function should return 1 instead which means that they symbol was not > added and needs to be freed later. > > Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> > --- > checkpolicy/module_compiler.c | 16 +++------------- > 1 file changed, 3 insertions(+), 13 deletions(-) > > diff --git a/checkpolicy/module_compiler.c b/checkpolicy/module_compiler.c > index 6e5483c..bb60f34 100644 > --- a/checkpolicy/module_compiler.c > +++ b/checkpolicy/module_compiler.c > @@ -647,7 +647,6 @@ int require_symbol(uint32_t symbol_type, > } else if (retval == -2) { > /* ignore require statements if that symbol was > * previously declared and is in current scope */ > - int prev_declaration_ok = 0; > if (is_id_in_scope(symbol_type, key)) { > if (symbol_type == SYM_TYPES) { > /* check that previous symbol has same > @@ -661,23 +660,14 @@ int require_symbol(uint32_t symbol_type, > table, key); > assert(old_datum != NULL); > unsigned char old_isattr = old_datum->flavor; > - prev_declaration_ok = > - (old_isattr == new_isattr ? 1 : 0); > - } else { > - prev_declaration_ok = 1; > + if (old_isattr != new_isattr) > + return -2; > } > - } > - if (prev_declaration_ok) { > /* ignore this require statement because it > * was already declared within my scope */ > stack_top->require_given = 1; > - return 1; > - } else { > - /* previous declaration was not in scope or > - * had a mismatched type/attribute, so > - * generate an error */ > - return -2; > } > + return 1; > } else if (retval < 0) { > return -3; > } else { /* fall through possible if retval is 0 or 1 */ >
On 01/18/2017 03:58 PM, Dominick Grift wrote: > On 01/18/2017 09:53 PM, James Carter wrote: >> Nicolas Iooss discovered that requiring a type in an optional block >> after the type has already been declared in another optional block >> results in a duplicate declaration error. >> > > from what i have been told and from experience, types cannot, reliably, > be declared in optional blocks. > > if the above is true, then the compiler should not allow one to declare > a type in an optional block in the first place > Well, this ordering issue reported by Nicolas would definitely cause their declaration to be unreliable. ;) I hope to make declaring types in optional blocks reliable. Jim >> The following policy will trigger the error. >> >> optional { >> type T1; >> } >> >> optional { >> require { type T1; } >> } >> >> In this case, although symtab_insert() in libsepol properly identifies >> that the first T1 is a declaration while the second is a require, it >> will return -2 which is the return value for a duplicate declaration >> and expect the calling code to properly handle the situation. The >> caller is require_symbol() in checkpolicy and it checks if the previous >> declaration is in the current scope. If it is, then the require can be >> ignored. It also checks to see if the declaration is a type and the >> require an attribute or vice versa which is an error. The problem is >> that -2 is returned if the declaration is not in scope which is >> interpreted as a duplicate declaration error. >> >> The function should return 1 instead which means that they symbol was not >> added and needs to be freed later. >> >> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> >> --- >> checkpolicy/module_compiler.c | 16 +++------------- >> 1 file changed, 3 insertions(+), 13 deletions(-) >> >> diff --git a/checkpolicy/module_compiler.c b/checkpolicy/module_compiler.c >> index 6e5483c..bb60f34 100644 >> --- a/checkpolicy/module_compiler.c >> +++ b/checkpolicy/module_compiler.c >> @@ -647,7 +647,6 @@ int require_symbol(uint32_t symbol_type, >> } else if (retval == -2) { >> /* ignore require statements if that symbol was >> * previously declared and is in current scope */ >> - int prev_declaration_ok = 0; >> if (is_id_in_scope(symbol_type, key)) { >> if (symbol_type == SYM_TYPES) { >> /* check that previous symbol has same >> @@ -661,23 +660,14 @@ int require_symbol(uint32_t symbol_type, >> table, key); >> assert(old_datum != NULL); >> unsigned char old_isattr = old_datum->flavor; >> - prev_declaration_ok = >> - (old_isattr == new_isattr ? 1 : 0); >> - } else { >> - prev_declaration_ok = 1; >> + if (old_isattr != new_isattr) >> + return -2; >> } >> - } >> - if (prev_declaration_ok) { >> /* ignore this require statement because it >> * was already declared within my scope */ >> stack_top->require_given = 1; >> - return 1; >> - } else { >> - /* previous declaration was not in scope or >> - * had a mismatched type/attribute, so >> - * generate an error */ >> - return -2; >> } >> + return 1; >> } else if (retval < 0) { >> return -3; >> } else { /* fall through possible if retval is 0 or 1 */ >> > > > > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. >
On 01/18/2017 10:09 PM, James Carter wrote: > On 01/18/2017 03:58 PM, Dominick Grift wrote: >> On 01/18/2017 09:53 PM, James Carter wrote: >>> Nicolas Iooss discovered that requiring a type in an optional block >>> after the type has already been declared in another optional block >>> results in a duplicate declaration error. >>> >> >> from what i have been told and from experience, types cannot, reliably, >> be declared in optional blocks. >> >> if the above is true, then the compiler should not allow one to declare >> a type in an optional block in the first place >> > > Well, this ordering issue reported by Nicolas would definitely cause > their declaration to be unreliable. ;) > > I hope to make declaring types in optional blocks reliable. That would be nice, but just so you know. This was not the issue i was encountering with declaring types in optionals. So even though this might have made things unreliable. It was not what made it unreliable for me. > > Jim > >>> The following policy will trigger the error. >>> >>> optional { >>> type T1; >>> } >>> >>> optional { >>> require { type T1; } >>> } >>> >>> In this case, although symtab_insert() in libsepol properly identifies >>> that the first T1 is a declaration while the second is a require, it >>> will return -2 which is the return value for a duplicate declaration >>> and expect the calling code to properly handle the situation. The >>> caller is require_symbol() in checkpolicy and it checks if the previous >>> declaration is in the current scope. If it is, then the require can be >>> ignored. It also checks to see if the declaration is a type and the >>> require an attribute or vice versa which is an error. The problem is >>> that -2 is returned if the declaration is not in scope which is >>> interpreted as a duplicate declaration error. >>> >>> The function should return 1 instead which means that they symbol was >>> not >>> added and needs to be freed later. >>> >>> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> >>> --- >>> checkpolicy/module_compiler.c | 16 +++------------- >>> 1 file changed, 3 insertions(+), 13 deletions(-) >>> >>> diff --git a/checkpolicy/module_compiler.c >>> b/checkpolicy/module_compiler.c >>> index 6e5483c..bb60f34 100644 >>> --- a/checkpolicy/module_compiler.c >>> +++ b/checkpolicy/module_compiler.c >>> @@ -647,7 +647,6 @@ int require_symbol(uint32_t symbol_type, >>> } else if (retval == -2) { >>> /* ignore require statements if that symbol was >>> * previously declared and is in current scope */ >>> - int prev_declaration_ok = 0; >>> if (is_id_in_scope(symbol_type, key)) { >>> if (symbol_type == SYM_TYPES) { >>> /* check that previous symbol has same >>> @@ -661,23 +660,14 @@ int require_symbol(uint32_t symbol_type, >>> table, key); >>> assert(old_datum != NULL); >>> unsigned char old_isattr = old_datum->flavor; >>> - prev_declaration_ok = >>> - (old_isattr == new_isattr ? 1 : 0); >>> - } else { >>> - prev_declaration_ok = 1; >>> + if (old_isattr != new_isattr) >>> + return -2; >>> } >>> - } >>> - if (prev_declaration_ok) { >>> /* ignore this require statement because it >>> * was already declared within my scope */ >>> stack_top->require_given = 1; >>> - return 1; >>> - } else { >>> - /* previous declaration was not in scope or >>> - * had a mismatched type/attribute, so >>> - * generate an error */ >>> - return -2; >>> } >>> + return 1; >>> } else if (retval < 0) { >>> return -3; >>> } else { /* fall through possible if retval is 0 or 1 */ >>> >> >> >> >> >> _______________________________________________ >> Selinux mailing list >> Selinux@tycho.nsa.gov >> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >> To get help, send an email containing "help" to >> Selinux-request@tycho.nsa.gov. >> > >
On 01/18/2017 10:15 PM, Dominick Grift wrote: > On 01/18/2017 10:09 PM, James Carter wrote: >> On 01/18/2017 03:58 PM, Dominick Grift wrote: >>> On 01/18/2017 09:53 PM, James Carter wrote: >>>> Nicolas Iooss discovered that requiring a type in an optional block >>>> after the type has already been declared in another optional block >>>> results in a duplicate declaration error. >>>> >>> >>> from what i have been told and from experience, types cannot, reliably, >>> be declared in optional blocks. >>> >>> if the above is true, then the compiler should not allow one to declare >>> a type in an optional block in the first place >>> >> >> Well, this ordering issue reported by Nicolas would definitely cause >> their declaration to be unreliable. ;) >> >> I hope to make declaring types in optional blocks reliable. > > That would be nice, but just so you know. This was not the issue i was > encountering with declaring types in optionals. So even though this > might have made things unreliable. It was not what made it unreliable > for me. > Actually, sorry. I do actually recognize the second scenario (duplicate declaration). So this might in fact be related. I can't believe that this ancient bug would finally be fixed. Ive been lead to believe that this was just a fundamental limitation of module policy all this time. My whole policy was structured on the assumption that types cannot reliably be declared in optionals.... >> >> Jim >> >>>> The following policy will trigger the error. >>>> >>>> optional { >>>> type T1; >>>> } >>>> >>>> optional { >>>> require { type T1; } >>>> } >>>> >>>> In this case, although symtab_insert() in libsepol properly identifies >>>> that the first T1 is a declaration while the second is a require, it >>>> will return -2 which is the return value for a duplicate declaration >>>> and expect the calling code to properly handle the situation. The >>>> caller is require_symbol() in checkpolicy and it checks if the previous >>>> declaration is in the current scope. If it is, then the require can be >>>> ignored. It also checks to see if the declaration is a type and the >>>> require an attribute or vice versa which is an error. The problem is >>>> that -2 is returned if the declaration is not in scope which is >>>> interpreted as a duplicate declaration error. >>>> >>>> The function should return 1 instead which means that they symbol was >>>> not >>>> added and needs to be freed later. >>>> >>>> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> >>>> --- >>>> checkpolicy/module_compiler.c | 16 +++------------- >>>> 1 file changed, 3 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/checkpolicy/module_compiler.c >>>> b/checkpolicy/module_compiler.c >>>> index 6e5483c..bb60f34 100644 >>>> --- a/checkpolicy/module_compiler.c >>>> +++ b/checkpolicy/module_compiler.c >>>> @@ -647,7 +647,6 @@ int require_symbol(uint32_t symbol_type, >>>> } else if (retval == -2) { >>>> /* ignore require statements if that symbol was >>>> * previously declared and is in current scope */ >>>> - int prev_declaration_ok = 0; >>>> if (is_id_in_scope(symbol_type, key)) { >>>> if (symbol_type == SYM_TYPES) { >>>> /* check that previous symbol has same >>>> @@ -661,23 +660,14 @@ int require_symbol(uint32_t symbol_type, >>>> table, key); >>>> assert(old_datum != NULL); >>>> unsigned char old_isattr = old_datum->flavor; >>>> - prev_declaration_ok = >>>> - (old_isattr == new_isattr ? 1 : 0); >>>> - } else { >>>> - prev_declaration_ok = 1; >>>> + if (old_isattr != new_isattr) >>>> + return -2; >>>> } >>>> - } >>>> - if (prev_declaration_ok) { >>>> /* ignore this require statement because it >>>> * was already declared within my scope */ >>>> stack_top->require_given = 1; >>>> - return 1; >>>> - } else { >>>> - /* previous declaration was not in scope or >>>> - * had a mismatched type/attribute, so >>>> - * generate an error */ >>>> - return -2; >>>> } >>>> + return 1; >>>> } else if (retval < 0) { >>>> return -3; >>>> } else { /* fall through possible if retval is 0 or 1 */ >>>> >>> >>> >>> >>> >>> _______________________________________________ >>> Selinux mailing list >>> Selinux@tycho.nsa.gov >>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >>> To get help, send an email containing "help" to >>> Selinux-request@tycho.nsa.gov. >>> >> >> > >
On Wed, 2017-01-18 at 21:58 +0100, Dominick Grift wrote: > On 01/18/2017 09:53 PM, James Carter wrote: > > > > Nicolas Iooss discovered that requiring a type in an optional block > > after the type has already been declared in another optional block > > results in a duplicate declaration error. > > > > from what i have been told and from experience, types cannot, > reliably, > be declared in optional blocks. If true, that is likely a linker issue rather than a checkpolicy issue. But that would make optionals rather useless if true. > > if the above is true, then the compiler should not allow one to > declare > a type in an optional block in the first place > > > > > The following policy will trigger the error. > > > > optional { > > type T1; > > } > > > > optional { > > require { type T1; } > > } > > > > In this case, although symtab_insert() in libsepol properly > > identifies > > that the first T1 is a declaration while the second is a require, > > it > > will return -2 which is the return value for a duplicate > > declaration > > and expect the calling code to properly handle the situation. The > > caller is require_symbol() in checkpolicy and it checks if the > > previous > > declaration is in the current scope. If it is, then the require can > > be > > ignored. It also checks to see if the declaration is a type and the > > require an attribute or vice versa which is an error. The problem > > is > > that -2 is returned if the declaration is not in scope which is > > interpreted as a duplicate declaration error. > > > > The function should return 1 instead which means that they symbol > > was not > > added and needs to be freed later. > > > > Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> > > --- > > checkpolicy/module_compiler.c | 16 +++------------- > > 1 file changed, 3 insertions(+), 13 deletions(-) > > > > diff --git a/checkpolicy/module_compiler.c > > b/checkpolicy/module_compiler.c > > index 6e5483c..bb60f34 100644 > > --- a/checkpolicy/module_compiler.c > > +++ b/checkpolicy/module_compiler.c > > @@ -647,7 +647,6 @@ int require_symbol(uint32_t symbol_type, > > } else if (retval == -2) { > > /* ignore require statements if that symbol was > > * previously declared and is in current scope */ > > - int prev_declaration_ok = 0; > > if (is_id_in_scope(symbol_type, key)) { > > if (symbol_type == SYM_TYPES) { > > /* check that previous symbol has > > same > > @@ -661,23 +660,14 @@ int require_symbol(uint32_t symbol_type, > > > > table, key); > > assert(old_datum != NULL); > > unsigned char old_isattr = > > old_datum->flavor; > > - prev_declaration_ok = > > - (old_isattr == new_isattr ? 1 > > : 0); > > - } else { > > - prev_declaration_ok = 1; > > + if (old_isattr != new_isattr) > > + return -2; > > } > > - } > > - if (prev_declaration_ok) { > > /* ignore this require statement because > > it > > * was already declared within my scope */ > > stack_top->require_given = 1; > > - return 1; > > - } else { > > - /* previous declaration was not in scope > > or > > - * had a mismatched type/attribute, so > > - * generate an error */ > > - return -2; > > } > > + return 1; > > } else if (retval < 0) { > > return -3; > > } else { /* fall through possible if retval > > is 0 or 1 */ > > > > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho > .nsa.gov.
On 01/19/2017 06:21 PM, Stephen Smalley wrote: > On Wed, 2017-01-18 at 21:58 +0100, Dominick Grift wrote: >> On 01/18/2017 09:53 PM, James Carter wrote: >>> >>> Nicolas Iooss discovered that requiring a type in an optional block >>> after the type has already been declared in another optional block >>> results in a duplicate declaration error. >>> >> >> from what i have been told and from experience, types cannot, >> reliably, >> be declared in optional blocks. > > If true, that is likely a linker issue rather than a checkpolicy issue. > But that would make optionals rather useless if true. Let me present it in a different way then: Should one be able to, reliably, "blockinherit" in optionals? This is the commit in dssp1 where i hit the dead-end with declaring types in optionals: https://github.com/DefenSec/dssp-contrib/commit/45236da4030f0a541c26753e13a91f69411d022e I asked slawrence to have a look at the policy, and he indicated to me that blocks cannot be inherited in optionals. The compiler accepts it but the policy becomes inconsistent > >> >> if the above is true, then the compiler should not allow one to >> declare >> a type in an optional block in the first place >> >>> >>> The following policy will trigger the error. >>> >>> optional { >>> type T1; >>> } >>> >>> optional { >>> require { type T1; } >>> } >>> >>> In this case, although symtab_insert() in libsepol properly >>> identifies >>> that the first T1 is a declaration while the second is a require, >>> it >>> will return -2 which is the return value for a duplicate >>> declaration >>> and expect the calling code to properly handle the situation. The >>> caller is require_symbol() in checkpolicy and it checks if the >>> previous >>> declaration is in the current scope. If it is, then the require can >>> be >>> ignored. It also checks to see if the declaration is a type and the >>> require an attribute or vice versa which is an error. The problem >>> is >>> that -2 is returned if the declaration is not in scope which is >>> interpreted as a duplicate declaration error. >>> >>> The function should return 1 instead which means that they symbol >>> was not >>> added and needs to be freed later. >>> >>> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> >>> --- >>> checkpolicy/module_compiler.c | 16 +++------------- >>> 1 file changed, 3 insertions(+), 13 deletions(-) >>> >>> diff --git a/checkpolicy/module_compiler.c >>> b/checkpolicy/module_compiler.c >>> index 6e5483c..bb60f34 100644 >>> --- a/checkpolicy/module_compiler.c >>> +++ b/checkpolicy/module_compiler.c >>> @@ -647,7 +647,6 @@ int require_symbol(uint32_t symbol_type, >>> } else if (retval == -2) { >>> /* ignore require statements if that symbol was >>> * previously declared and is in current scope */ >>> - int prev_declaration_ok = 0; >>> if (is_id_in_scope(symbol_type, key)) { >>> if (symbol_type == SYM_TYPES) { >>> /* check that previous symbol has >>> same >>> @@ -661,23 +660,14 @@ int require_symbol(uint32_t symbol_type, >>> >>> table, key); >>> assert(old_datum != NULL); >>> unsigned char old_isattr = >>> old_datum->flavor; >>> - prev_declaration_ok = >>> - (old_isattr == new_isattr ? 1 >>> : 0); >>> - } else { >>> - prev_declaration_ok = 1; >>> + if (old_isattr != new_isattr) >>> + return -2; >>> } >>> - } >>> - if (prev_declaration_ok) { >>> /* ignore this require statement because >>> it >>> * was already declared within my scope */ >>> stack_top->require_given = 1; >>> - return 1; >>> - } else { >>> - /* previous declaration was not in scope >>> or >>> - * had a mismatched type/attribute, so >>> - * generate an error */ >>> - return -2; >>> } >>> + return 1; >>> } else if (retval < 0) { >>> return -3; >>> } else { /* fall through possible if retval >>> is 0 or 1 */ >>> >> >> >> _______________________________________________ >> Selinux mailing list >> Selinux@tycho.nsa.gov >> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >> To get help, send an email containing "help" to Selinux-request@tycho >> .nsa.gov.
On 01/19/2017 04:22 PM, Dominick Grift wrote: > On 01/19/2017 06:21 PM, Stephen Smalley wrote: >> On Wed, 2017-01-18 at 21:58 +0100, Dominick Grift wrote: >>> On 01/18/2017 09:53 PM, James Carter wrote: >>>> >>>> Nicolas Iooss discovered that requiring a type in an optional block >>>> after the type has already been declared in another optional block >>>> results in a duplicate declaration error. >>>> >>> >>> from what i have been told and from experience, types cannot, >>> reliably, >>> be declared in optional blocks. >> >> If true, that is likely a linker issue rather than a checkpolicy issue. >> But that would make optionals rather useless if true. > > Let me present it in a different way then: > > Should one be able to, reliably, "blockinherit" in optionals? > > This is the commit in dssp1 where i hit the dead-end with declaring > types in optionals: > > https://github.com/DefenSec/dssp-contrib/commit/45236da4030f0a541c26753e13a91f69411d022e > > I asked slawrence to have a look at the policy, and he indicated to me > that blocks cannot be inherited in optionals. > > The compiler accepts it but the policy becomes inconsistent > By inconsistent, do you mean that an the binary policy produced is not valid, or something else? Macro definitions are not allowed in optionals, so if you have a macro definition in a block, that should cause an error. Other than that, I can't think of what would cause problems. Jim >> >>> >>> if the above is true, then the compiler should not allow one to >>> declare >>> a type in an optional block in the first place >>> >>>> >>>> The following policy will trigger the error. >>>> >>>> optional { >>>> type T1; >>>> } >>>> >>>> optional { >>>> require { type T1; } >>>> } >>>> >>>> In this case, although symtab_insert() in libsepol properly >>>> identifies >>>> that the first T1 is a declaration while the second is a require, >>>> it >>>> will return -2 which is the return value for a duplicate >>>> declaration >>>> and expect the calling code to properly handle the situation. The >>>> caller is require_symbol() in checkpolicy and it checks if the >>>> previous >>>> declaration is in the current scope. If it is, then the require can >>>> be >>>> ignored. It also checks to see if the declaration is a type and the >>>> require an attribute or vice versa which is an error. The problem >>>> is >>>> that -2 is returned if the declaration is not in scope which is >>>> interpreted as a duplicate declaration error. >>>> >>>> The function should return 1 instead which means that they symbol >>>> was not >>>> added and needs to be freed later. >>>> >>>> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> >>>> --- >>>> checkpolicy/module_compiler.c | 16 +++------------- >>>> 1 file changed, 3 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/checkpolicy/module_compiler.c >>>> b/checkpolicy/module_compiler.c >>>> index 6e5483c..bb60f34 100644 >>>> --- a/checkpolicy/module_compiler.c >>>> +++ b/checkpolicy/module_compiler.c >>>> @@ -647,7 +647,6 @@ int require_symbol(uint32_t symbol_type, >>>> } else if (retval == -2) { >>>> /* ignore require statements if that symbol was >>>> * previously declared and is in current scope */ >>>> - int prev_declaration_ok = 0; >>>> if (is_id_in_scope(symbol_type, key)) { >>>> if (symbol_type == SYM_TYPES) { >>>> /* check that previous symbol has >>>> same >>>> @@ -661,23 +660,14 @@ int require_symbol(uint32_t symbol_type, >>>> >>>> table, key); >>>> assert(old_datum != NULL); >>>> unsigned char old_isattr = >>>> old_datum->flavor; >>>> - prev_declaration_ok = >>>> - (old_isattr == new_isattr ? 1 >>>> : 0); >>>> - } else { >>>> - prev_declaration_ok = 1; >>>> + if (old_isattr != new_isattr) >>>> + return -2; >>>> } >>>> - } >>>> - if (prev_declaration_ok) { >>>> /* ignore this require statement because >>>> it >>>> * was already declared within my scope */ >>>> stack_top->require_given = 1; >>>> - return 1; >>>> - } else { >>>> - /* previous declaration was not in scope >>>> or >>>> - * had a mismatched type/attribute, so >>>> - * generate an error */ >>>> - return -2; >>>> } >>>> + return 1; >>>> } else if (retval < 0) { >>>> return -3; >>>> } else { /* fall through possible if retval >>>> is 0 or 1 */ >>>> >>> >>> >>> _______________________________________________ >>> Selinux mailing list >>> Selinux@tycho.nsa.gov >>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >>> To get help, send an email containing "help" to Selinux-request@tycho >>> .nsa.gov. > > > > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. >
On 01/20/2017 04:16 PM, James Carter wrote: > On 01/19/2017 04:22 PM, Dominick Grift wrote: >> On 01/19/2017 06:21 PM, Stephen Smalley wrote: >>> On Wed, 2017-01-18 at 21:58 +0100, Dominick Grift wrote: >>>> On 01/18/2017 09:53 PM, James Carter wrote: >>>>> >>>>> Nicolas Iooss discovered that requiring a type in an optional block >>>>> after the type has already been declared in another optional block >>>>> results in a duplicate declaration error. >>>>> >>>> >>>> from what i have been told and from experience, types cannot, >>>> reliably, >>>> be declared in optional blocks. >>> >>> If true, that is likely a linker issue rather than a checkpolicy issue. >>> But that would make optionals rather useless if true. >> >> Let me present it in a different way then: >> >> Should one be able to, reliably, "blockinherit" in optionals? >> >> This is the commit in dssp1 where i hit the dead-end with declaring >> types in optionals: >> >> https://github.com/DefenSec/dssp-contrib/commit/45236da4030f0a541c26753e13a91f69411d022e >> >> >> I asked slawrence to have a look at the policy, and he indicated to me >> that blocks cannot be inherited in optionals. >> >> The compiler accepts it but the policy becomes inconsistent >> > > By inconsistent, do you mean that an the binary policy produced is not > valid, or something else? I used the word inconsistent because i actually forgot the details (it has been more than a year since I made this change and I tried hard to put this behind me, thinking that this was a fundamental limitation rather than a "bug"). I suspect the following: Rules end up in the policy but they aren't actually recognized/enforced. Either the above or: Rules do *not* end up in the policy even though they are specified. Maybe slawrence remembers what the issue exactly was (added to recipients) > > Macro definitions are not allowed in optionals, so if you have a macro > definition in a block, that should cause an error. Other than that, I > can't think of what would cause problems. That is mentioned in the secilc documentation. So why does secilc not disallow inheriting blocks in optionals? I might be wrong but I believe that i was not actually defining macros in my blocks back then, and still the behavior was "inconsistent". > > Jim > >>> >>>> >>>> if the above is true, then the compiler should not allow one to >>>> declare >>>> a type in an optional block in the first place >>>> >>>>> >>>>> The following policy will trigger the error. >>>>> >>>>> optional { >>>>> type T1; >>>>> } >>>>> >>>>> optional { >>>>> require { type T1; } >>>>> } >>>>> >>>>> In this case, although symtab_insert() in libsepol properly >>>>> identifies >>>>> that the first T1 is a declaration while the second is a require, >>>>> it >>>>> will return -2 which is the return value for a duplicate >>>>> declaration >>>>> and expect the calling code to properly handle the situation. The >>>>> caller is require_symbol() in checkpolicy and it checks if the >>>>> previous >>>>> declaration is in the current scope. If it is, then the require can >>>>> be >>>>> ignored. It also checks to see if the declaration is a type and the >>>>> require an attribute or vice versa which is an error. The problem >>>>> is >>>>> that -2 is returned if the declaration is not in scope which is >>>>> interpreted as a duplicate declaration error. >>>>> >>>>> The function should return 1 instead which means that they symbol >>>>> was not >>>>> added and needs to be freed later. >>>>> >>>>> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> >>>>> --- >>>>> checkpolicy/module_compiler.c | 16 +++------------- >>>>> 1 file changed, 3 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/checkpolicy/module_compiler.c >>>>> b/checkpolicy/module_compiler.c >>>>> index 6e5483c..bb60f34 100644 >>>>> --- a/checkpolicy/module_compiler.c >>>>> +++ b/checkpolicy/module_compiler.c >>>>> @@ -647,7 +647,6 @@ int require_symbol(uint32_t symbol_type, >>>>> } else if (retval == -2) { >>>>> /* ignore require statements if that symbol was >>>>> * previously declared and is in current scope */ >>>>> - int prev_declaration_ok = 0; >>>>> if (is_id_in_scope(symbol_type, key)) { >>>>> if (symbol_type == SYM_TYPES) { >>>>> /* check that previous symbol has >>>>> same >>>>> @@ -661,23 +660,14 @@ int require_symbol(uint32_t symbol_type, >>>>> >>>>> table, key); >>>>> assert(old_datum != NULL); >>>>> unsigned char old_isattr = >>>>> old_datum->flavor; >>>>> - prev_declaration_ok = >>>>> - (old_isattr == new_isattr ? 1 >>>>> : 0); >>>>> - } else { >>>>> - prev_declaration_ok = 1; >>>>> + if (old_isattr != new_isattr) >>>>> + return -2; >>>>> } >>>>> - } >>>>> - if (prev_declaration_ok) { >>>>> /* ignore this require statement because >>>>> it >>>>> * was already declared within my scope */ >>>>> stack_top->require_given = 1; >>>>> - return 1; >>>>> - } else { >>>>> - /* previous declaration was not in scope >>>>> or >>>>> - * had a mismatched type/attribute, so >>>>> - * generate an error */ >>>>> - return -2; >>>>> } >>>>> + return 1; >>>>> } else if (retval < 0) { >>>>> return -3; >>>>> } else { /* fall through possible if retval >>>>> is 0 or 1 */ >>>>> >>>> >>>> >>>> _______________________________________________ >>>> Selinux mailing list >>>> Selinux@tycho.nsa.gov >>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >>>> To get help, send an email containing "help" to Selinux-request@tycho >>>> .nsa.gov. >> >> >> >> >> _______________________________________________ >> Selinux mailing list >> Selinux@tycho.nsa.gov >> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >> To get help, send an email containing "help" to >> Selinux-request@tycho.nsa.gov. >> > >
On 01/20/2017 04:29 PM, Dominick Grift wrote: > On 01/20/2017 04:16 PM, James Carter wrote: >> On 01/19/2017 04:22 PM, Dominick Grift wrote: >>> On 01/19/2017 06:21 PM, Stephen Smalley wrote: >>>> On Wed, 2017-01-18 at 21:58 +0100, Dominick Grift wrote: >>>>> On 01/18/2017 09:53 PM, James Carter wrote: >>>>>> >>>>>> Nicolas Iooss discovered that requiring a type in an optional block >>>>>> after the type has already been declared in another optional block >>>>>> results in a duplicate declaration error. >>>>>> >>>>> >>>>> from what i have been told and from experience, types cannot, >>>>> reliably, >>>>> be declared in optional blocks. >>>> >>>> If true, that is likely a linker issue rather than a checkpolicy issue. >>>> But that would make optionals rather useless if true. >>> >>> Let me present it in a different way then: >>> >>> Should one be able to, reliably, "blockinherit" in optionals? >>> >>> This is the commit in dssp1 where i hit the dead-end with declaring >>> types in optionals: >>> >>> https://github.com/DefenSec/dssp-contrib/commit/45236da4030f0a541c26753e13a91f69411d022e >>> >>> >>> I asked slawrence to have a look at the policy, and he indicated to me >>> that blocks cannot be inherited in optionals. >>> >>> The compiler accepts it but the policy becomes inconsistent >>> >> >> By inconsistent, do you mean that an the binary policy produced is not >> valid, or something else? > > I used the word inconsistent because i actually forgot the details (it > has been more than a year since I made this change and I tried hard to > put this behind me, thinking that this was a fundamental limitation > rather than a "bug"). > > I suspect the following: > > Rules end up in the policy but they aren't actually recognized/enforced. > > Either the above or: > > Rules do *not* end up in the policy even though they are specified. > > Maybe slawrence remembers what the issue exactly was (added to recipients) > One thing is for sure, by moving my blockinherits out of the optional blocks my problems were solved. secilc did not disallow me to put the blockinherits in optionals, it did not complain, it just did not give the expected results, and things were fixed by moving the blocks out of the optionals >> >> Macro definitions are not allowed in optionals, so if you have a macro >> definition in a block, that should cause an error. Other than that, I >> can't think of what would cause problems. > > That is mentioned in the secilc documentation. So why does secilc not > disallow inheriting blocks in optionals? > > I might be wrong but I believe that i was not actually defining macros > in my blocks back then, and still the behavior was "inconsistent". > > >> >> Jim >> >>>> >>>>> >>>>> if the above is true, then the compiler should not allow one to >>>>> declare >>>>> a type in an optional block in the first place >>>>> >>>>>> >>>>>> The following policy will trigger the error. >>>>>> >>>>>> optional { >>>>>> type T1; >>>>>> } >>>>>> >>>>>> optional { >>>>>> require { type T1; } >>>>>> } >>>>>> >>>>>> In this case, although symtab_insert() in libsepol properly >>>>>> identifies >>>>>> that the first T1 is a declaration while the second is a require, >>>>>> it >>>>>> will return -2 which is the return value for a duplicate >>>>>> declaration >>>>>> and expect the calling code to properly handle the situation. The >>>>>> caller is require_symbol() in checkpolicy and it checks if the >>>>>> previous >>>>>> declaration is in the current scope. If it is, then the require can >>>>>> be >>>>>> ignored. It also checks to see if the declaration is a type and the >>>>>> require an attribute or vice versa which is an error. The problem >>>>>> is >>>>>> that -2 is returned if the declaration is not in scope which is >>>>>> interpreted as a duplicate declaration error. >>>>>> >>>>>> The function should return 1 instead which means that they symbol >>>>>> was not >>>>>> added and needs to be freed later. >>>>>> >>>>>> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> >>>>>> --- >>>>>> checkpolicy/module_compiler.c | 16 +++------------- >>>>>> 1 file changed, 3 insertions(+), 13 deletions(-) >>>>>> >>>>>> diff --git a/checkpolicy/module_compiler.c >>>>>> b/checkpolicy/module_compiler.c >>>>>> index 6e5483c..bb60f34 100644 >>>>>> --- a/checkpolicy/module_compiler.c >>>>>> +++ b/checkpolicy/module_compiler.c >>>>>> @@ -647,7 +647,6 @@ int require_symbol(uint32_t symbol_type, >>>>>> } else if (retval == -2) { >>>>>> /* ignore require statements if that symbol was >>>>>> * previously declared and is in current scope */ >>>>>> - int prev_declaration_ok = 0; >>>>>> if (is_id_in_scope(symbol_type, key)) { >>>>>> if (symbol_type == SYM_TYPES) { >>>>>> /* check that previous symbol has >>>>>> same >>>>>> @@ -661,23 +660,14 @@ int require_symbol(uint32_t symbol_type, >>>>>> >>>>>> table, key); >>>>>> assert(old_datum != NULL); >>>>>> unsigned char old_isattr = >>>>>> old_datum->flavor; >>>>>> - prev_declaration_ok = >>>>>> - (old_isattr == new_isattr ? 1 >>>>>> : 0); >>>>>> - } else { >>>>>> - prev_declaration_ok = 1; >>>>>> + if (old_isattr != new_isattr) >>>>>> + return -2; >>>>>> } >>>>>> - } >>>>>> - if (prev_declaration_ok) { >>>>>> /* ignore this require statement because >>>>>> it >>>>>> * was already declared within my scope */ >>>>>> stack_top->require_given = 1; >>>>>> - return 1; >>>>>> - } else { >>>>>> - /* previous declaration was not in scope >>>>>> or >>>>>> - * had a mismatched type/attribute, so >>>>>> - * generate an error */ >>>>>> - return -2; >>>>>> } >>>>>> + return 1; >>>>>> } else if (retval < 0) { >>>>>> return -3; >>>>>> } else { /* fall through possible if retval >>>>>> is 0 or 1 */ >>>>>> >>>>> >>>>> >>>>> _______________________________________________ >>>>> Selinux mailing list >>>>> Selinux@tycho.nsa.gov >>>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >>>>> To get help, send an email containing "help" to Selinux-request@tycho >>>>> .nsa.gov. >>> >>> >>> >>> >>> _______________________________________________ >>> Selinux mailing list >>> Selinux@tycho.nsa.gov >>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >>> To get help, send an email containing "help" to >>> Selinux-request@tycho.nsa.gov. >>> >> >> > >
On 01/20/2017 10:29 AM, Dominick Grift wrote: > On 01/20/2017 04:16 PM, James Carter wrote: >> On 01/19/2017 04:22 PM, Dominick Grift wrote: >>> On 01/19/2017 06:21 PM, Stephen Smalley wrote: >>>> On Wed, 2017-01-18 at 21:58 +0100, Dominick Grift wrote: >>>>> On 01/18/2017 09:53 PM, James Carter wrote: >>>>>> >>>>>> Nicolas Iooss discovered that requiring a type in an optional block >>>>>> after the type has already been declared in another optional block >>>>>> results in a duplicate declaration error. >>>>>> >>>>> >>>>> from what i have been told and from experience, types cannot, >>>>> reliably, >>>>> be declared in optional blocks. >>>> >>>> If true, that is likely a linker issue rather than a checkpolicy issue. >>>> But that would make optionals rather useless if true. >>> >>> Let me present it in a different way then: >>> >>> Should one be able to, reliably, "blockinherit" in optionals? >>> >>> This is the commit in dssp1 where i hit the dead-end with declaring >>> types in optionals: >>> >>> https://github.com/DefenSec/dssp-contrib/commit/45236da4030f0a541c26753e13a91f69411d022e >>> >>> >>> I asked slawrence to have a look at the policy, and he indicated to me >>> that blocks cannot be inherited in optionals. >>> >>> The compiler accepts it but the policy becomes inconsistent >>> >> >> By inconsistent, do you mean that an the binary policy produced is not >> valid, or something else? > > I used the word inconsistent because i actually forgot the details (it > has been more than a year since I made this change and I tried hard to > put this behind me, thinking that this was a fundamental limitation > rather than a "bug"). > > I suspect the following: > > Rules end up in the policy but they aren't actually recognized/enforced. > > Either the above or: > > Rules do *not* end up in the policy even though they are specified. > > Maybe slawrence remembers what the issue exactly was (added to recipients) > After digging through some past emails and discussions, I think part of the issue is that things just start to get tricky and difficult to deal with if you allow blockinherits (and for similar reasons blocks/macros) in optionals. For example, say you had something like this: (optional foo (block b (blockinherit c) ) ... ) (block d (blockinherit b)) If something in optional foo fails, then the entire block b goes away. So does that mean the blockinherit b in block d goes away too? Or does that stay? Removing it could be potentially tricky since we need to remove tree nodes that have been copied (we don't currently where things are copied to), and that removal might have side effects. This is a similar reason why we don't allow macros in optionals. Once they've been called, it becomes difficult to remove them if the macros go away. Not impossible, but difficult. And since blockinherits can pull in blocks, you're essentially allowing blocks inside optionals if you allow blockinherits inside optionals. So this is likely the reason for not allowing blockinherits in optionals. They can pull in blocks and macros, which aren't allowed to be optional. I agree that supporting optional blocks/macros/blockinherits/etc would probably be really nice, but things get difficult pretty fast. Re-resolve when optionals fail is a messy bit of code. Figuring out the right thing to do and when isn't obvious or easy. Now, we could add in some restrictions that make the changes less difficult (maybe), like blocks cannot be inside optionals, or blockinherits inside optionals cannot inherit blocks that contain other blocks. But again, that's still kindof a pain to implement and those are probably restrictions that you'd end up running into eventually anyways. So it would be best to just really figure out how to make it all work. I'm sure it can work, it's just not an easy problem to solve and takes time to figure out. - Steve
On 01/20/2017 06:14 PM, Steve Lawrence wrote: > On 01/20/2017 10:29 AM, Dominick Grift wrote: >> On 01/20/2017 04:16 PM, James Carter wrote: >>> On 01/19/2017 04:22 PM, Dominick Grift wrote: >>>> On 01/19/2017 06:21 PM, Stephen Smalley wrote: >>>>> On Wed, 2017-01-18 at 21:58 +0100, Dominick Grift wrote: >>>>>> On 01/18/2017 09:53 PM, James Carter wrote: >>>>>>> >>>>>>> Nicolas Iooss discovered that requiring a type in an optional block >>>>>>> after the type has already been declared in another optional block >>>>>>> results in a duplicate declaration error. >>>>>>> >>>>>> >>>>>> from what i have been told and from experience, types cannot, >>>>>> reliably, >>>>>> be declared in optional blocks. >>>>> >>>>> If true, that is likely a linker issue rather than a checkpolicy issue. >>>>> But that would make optionals rather useless if true. >>>> >>>> Let me present it in a different way then: >>>> >>>> Should one be able to, reliably, "blockinherit" in optionals? >>>> >>>> This is the commit in dssp1 where i hit the dead-end with declaring >>>> types in optionals: >>>> >>>> https://github.com/DefenSec/dssp-contrib/commit/45236da4030f0a541c26753e13a91f69411d022e >>>> >>>> >>>> I asked slawrence to have a look at the policy, and he indicated to me >>>> that blocks cannot be inherited in optionals. >>>> >>>> The compiler accepts it but the policy becomes inconsistent >>>> >>> >>> By inconsistent, do you mean that an the binary policy produced is not >>> valid, or something else? >> >> I used the word inconsistent because i actually forgot the details (it >> has been more than a year since I made this change and I tried hard to >> put this behind me, thinking that this was a fundamental limitation >> rather than a "bug"). >> >> I suspect the following: >> >> Rules end up in the policy but they aren't actually recognized/enforced. >> >> Either the above or: >> >> Rules do *not* end up in the policy even though they are specified. >> >> Maybe slawrence remembers what the issue exactly was (added to recipients) >> > > After digging through some past emails and discussions, I think part of > the issue is that things just start to get tricky and difficult to deal > with if you allow blockinherits (and for similar reasons blocks/macros) > in optionals. > > For example, say you had something like this: > > (optional foo > (block b > (blockinherit c) > ) > ... > ) > > (block d > (blockinherit b)) > > > If something in optional foo fails, then the entire block b goes away. > So does that mean the blockinherit b in block d goes away too? Or does > that stay? Removing it could be potentially tricky since we need to > remove tree nodes that have been copied (we don't currently where things > are copied to), and that removal might have side effects. This is a > similar reason why we don't allow macros in optionals. Once they've been > called, it becomes difficult to remove them if the macros go away. Not > impossible, but difficult. > > And since blockinherits can pull in blocks, you're essentially allowing > blocks inside optionals if you allow blockinherits inside optionals. So > this is likely the reason for not allowing blockinherits in optionals. > They can pull in blocks and macros, which aren't allowed to be optional. > > I agree that supporting optional blocks/macros/blockinherits/etc would > probably be really nice, but things get difficult pretty fast. > Re-resolve when optionals fail is a messy bit of code. Figuring out the > right thing to do and when isn't obvious or easy. > > Now, we could add in some restrictions that make the changes less > difficult (maybe), like blocks cannot be inside optionals, or > blockinherits inside optionals cannot inherit blocks that contain other > blocks. But again, that's still kindof a pain to implement and those are > probably restrictions that you'd end up running into eventually anyways. > So it would be best to just really figure out how to make it all work. > I'm sure it can work, it's just not an easy problem to solve and takes > time to figure out. > > - Steve > Thank you. I guess i somehow thought that the two issues were one and the same (ie. "corruption" when declaring types in optionals vs. "corruption" when inheriting blocks in optionals) Seems I was wrong there. The "type issue" is probably not CIL related and has been there for a very long time. The two issues have things in common though. Both declaring types in optionals, as well as inheriting blocks in optionals are unreliable and should be avoided.
On Wed, Jan 18, 2017 at 9:53 PM, James Carter <jwcart2@tycho.nsa.gov> wrote: > Nicolas Iooss discovered that requiring a type in an optional block > after the type has already been declared in another optional block > results in a duplicate declaration error. > > The following policy will trigger the error. > > optional { > type T1; > } > > optional { > require { type T1; } > } > > In this case, although symtab_insert() in libsepol properly identifies > that the first T1 is a declaration while the second is a require, it > will return -2 which is the return value for a duplicate declaration > and expect the calling code to properly handle the situation. The > caller is require_symbol() in checkpolicy and it checks if the previous > declaration is in the current scope. If it is, then the require can be > ignored. It also checks to see if the declaration is a type and the > require an attribute or vice versa which is an error. The problem is > that -2 is returned if the declaration is not in scope which is > interpreted as a duplicate declaration error. > > The function should return 1 instead which means that they symbol was not > added and needs to be freed later. Hello, I tested your patch with the following policy module written in a file named testmodule.te: module testmodule 1.0.0; require { class process { fork }; } optional { require { attribute ATTR; } type TYPE1, ATTR; } optional { require { type TYPE1; } allow TYPE1 self:process fork; } checkmodule failed to compile this module: testmodule.te:10:ERROR 'This block has no require section.' at token '}' on line 10: } allow TYPE1 self:process fork; Hence I modified the require statement of the second optional block to "require { type TYPE1, TYPE2; }" and checkmodule reported: testmodule.te:9:ERROR 'type TYPE1 is not within scope' at token ';' on line 9: require { type TYPE1, TYPE2; } allow TYPE1 self:process fork; It seems there is a scope issue with TYPE1 when it is used in a block where it is required. Is this a bug? Thanks, Nicolas PS: while debugging this issue I found some other memory leaks in checkpolicy. I will send some patches later.
On 01/19/2017 06:21 PM, Stephen Smalley wrote: > On Wed, 2017-01-18 at 21:58 +0100, Dominick Grift wrote: >> On 01/18/2017 09:53 PM, James Carter wrote: >>> >>> Nicolas Iooss discovered that requiring a type in an optional block >>> after the type has already been declared in another optional block >>> results in a duplicate declaration error. >>> >> >> from what i have been told and from experience, types cannot, >> reliably, >> be declared in optional blocks. > > If true, that is likely a linker issue rather than a checkpolicy issue. > But that would make optionals rather useless if true. > I beg to differ. optionals would not be useless if they would not allow declarations IMHO. In my view it makes little sense to declare anything in an optional. Optionals are a modularity feature. So lets look at that. For example the apache_content_template() and how it is currently used. For example the git module calls the apache_content_template() in an optional. So the httpd_git_content_t type is declared if the apache module is available, and not if it is not available. Same goes for a bunch of other modules. But that does not make things all that "modular". Because I cannot have the git module and the apache module without the httpd_git_content_t. What would make more sense if the apache_content_template(git) would be put in a separate module (example apache_git). Then i could have the git module , the apache module and not the "apache_git" module. That is modularity IMHO. >> >> if the above is true, then the compiler should not allow one to >> declare >> a type in an optional block in the first place >> >>> >>> The following policy will trigger the error. >>> >>> optional { >>> type T1; >>> } >>> >>> optional { >>> require { type T1; } >>> } >>> >>> In this case, although symtab_insert() in libsepol properly >>> identifies >>> that the first T1 is a declaration while the second is a require, >>> it >>> will return -2 which is the return value for a duplicate >>> declaration >>> and expect the calling code to properly handle the situation. The >>> caller is require_symbol() in checkpolicy and it checks if the >>> previous >>> declaration is in the current scope. If it is, then the require can >>> be >>> ignored. It also checks to see if the declaration is a type and the >>> require an attribute or vice versa which is an error. The problem >>> is >>> that -2 is returned if the declaration is not in scope which is >>> interpreted as a duplicate declaration error. >>> >>> The function should return 1 instead which means that they symbol >>> was not >>> added and needs to be freed later. >>> >>> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> >>> --- >>> checkpolicy/module_compiler.c | 16 +++------------- >>> 1 file changed, 3 insertions(+), 13 deletions(-) >>> >>> diff --git a/checkpolicy/module_compiler.c >>> b/checkpolicy/module_compiler.c >>> index 6e5483c..bb60f34 100644 >>> --- a/checkpolicy/module_compiler.c >>> +++ b/checkpolicy/module_compiler.c >>> @@ -647,7 +647,6 @@ int require_symbol(uint32_t symbol_type, >>> } else if (retval == -2) { >>> /* ignore require statements if that symbol was >>> * previously declared and is in current scope */ >>> - int prev_declaration_ok = 0; >>> if (is_id_in_scope(symbol_type, key)) { >>> if (symbol_type == SYM_TYPES) { >>> /* check that previous symbol has >>> same >>> @@ -661,23 +660,14 @@ int require_symbol(uint32_t symbol_type, >>> >>> table, key); >>> assert(old_datum != NULL); >>> unsigned char old_isattr = >>> old_datum->flavor; >>> - prev_declaration_ok = >>> - (old_isattr == new_isattr ? 1 >>> : 0); >>> - } else { >>> - prev_declaration_ok = 1; >>> + if (old_isattr != new_isattr) >>> + return -2; >>> } >>> - } >>> - if (prev_declaration_ok) { >>> /* ignore this require statement because >>> it >>> * was already declared within my scope */ >>> stack_top->require_given = 1; >>> - return 1; >>> - } else { >>> - /* previous declaration was not in scope >>> or >>> - * had a mismatched type/attribute, so >>> - * generate an error */ >>> - return -2; >>> } >>> + return 1; >>> } else if (retval < 0) { >>> return -3; >>> } else { /* fall through possible if retval >>> is 0 or 1 */ >>> >> >> >> _______________________________________________ >> Selinux mailing list >> Selinux@tycho.nsa.gov >> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >> To get help, send an email containing "help" to Selinux-request@tycho >> .nsa.gov.
On 01/21/2017 08:58 AM, Nicolas Iooss wrote: > On Wed, Jan 18, 2017 at 9:53 PM, James Carter <jwcart2@tycho.nsa.gov > <mailto:jwcart2@tycho.nsa.gov>> wrote: > > Nicolas Iooss discovered that requiring a type in an optional block > after the type has already been declared in another optional block > results in a duplicate declaration error. > > The following policy will trigger the error. > > optional { > type T1; > } > > optional { > require { type T1; } > } > > In this case, although symtab_insert() in libsepol properly identifies > that the first T1 is a declaration while the second is a require, it > will return -2 which is the return value for a duplicate declaration > and expect the calling code to properly handle the situation. The > caller is require_symbol() in checkpolicy and it checks if the previous > declaration is in the current scope. If it is, then the require can be > ignored. It also checks to see if the declaration is a type and the > require an attribute or vice versa which is an error. The problem is > that -2 is returned if the declaration is not in scope which is > interpreted as a duplicate declaration error. > > The function should return 1 instead which means that they symbol was not > added and needs to be freed later. > > > Hello, > I tested your patch with the following policy module written in a file named > testmodule.te: > > module testmodule 1.0.0; > require { class process { fork }; } > optional { > require { attribute ATTR; } > type TYPE1, ATTR; > } > optional { > require { type TYPE1; } > allow TYPE1 self:process fork; > } > > checkmodule failed to compile this module: > > testmodule.te:10:ERROR 'This block has no require section.' at token '}' on > line 10: > } > allow TYPE1 self:process fork; > I was looking at what my patch did late Friday and I thought that this might happen. > Hence I modified the require statement of the second optional block to "require > { type TYPE1, TYPE2; }" and checkmodule reported: > > testmodule.te:9:ERROR 'type TYPE1 is not within scope' at token ';' on line 9: > require { type TYPE1, TYPE2; } > allow TYPE1 self:process fork; > > It seems there is a scope issue with TYPE1 when it is used in a block where it > is required. Is this a bug? > This is not the desired behavior. I am looking at refactoring this code. Thanks for the report. Jim > Thanks, > Nicolas > > PS: while debugging this issue I found some other memory leaks in checkpolicy. I > will send some patches later.
diff --git a/checkpolicy/module_compiler.c b/checkpolicy/module_compiler.c index 6e5483c..bb60f34 100644 --- a/checkpolicy/module_compiler.c +++ b/checkpolicy/module_compiler.c @@ -647,7 +647,6 @@ int require_symbol(uint32_t symbol_type, } else if (retval == -2) { /* ignore require statements if that symbol was * previously declared and is in current scope */ - int prev_declaration_ok = 0; if (is_id_in_scope(symbol_type, key)) { if (symbol_type == SYM_TYPES) { /* check that previous symbol has same @@ -661,23 +660,14 @@ int require_symbol(uint32_t symbol_type, table, key); assert(old_datum != NULL); unsigned char old_isattr = old_datum->flavor; - prev_declaration_ok = - (old_isattr == new_isattr ? 1 : 0); - } else { - prev_declaration_ok = 1; + if (old_isattr != new_isattr) + return -2; } - } - if (prev_declaration_ok) { /* ignore this require statement because it * was already declared within my scope */ stack_top->require_given = 1; - return 1; - } else { - /* previous declaration was not in scope or - * had a mismatched type/attribute, so - * generate an error */ - return -2; } + return 1; } else if (retval < 0) { return -3; } else { /* fall through possible if retval is 0 or 1 */
Nicolas Iooss discovered that requiring a type in an optional block after the type has already been declared in another optional block results in a duplicate declaration error. The following policy will trigger the error. optional { type T1; } optional { require { type T1; } } In this case, although symtab_insert() in libsepol properly identifies that the first T1 is a declaration while the second is a require, it will return -2 which is the return value for a duplicate declaration and expect the calling code to properly handle the situation. The caller is require_symbol() in checkpolicy and it checks if the previous declaration is in the current scope. If it is, then the require can be ignored. It also checks to see if the declaration is a type and the require an attribute or vice versa which is an error. The problem is that -2 is returned if the declaration is not in scope which is interpreted as a duplicate declaration error. The function should return 1 instead which means that they symbol was not added and needs to be freed later. Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> --- checkpolicy/module_compiler.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)