Re: [PATCH] libsepol/cil: better error message with duplicate aliases + support aliases to aliases

[Dominick Grift]

On Fri, Jun 02, 2017 at 07:12:25AM -0400, Steve Lawrence wrote:
> On 06/02/2017 05:18 AM, Dominick Grift wrote:
> > On Thu, Jun 01, 2017 at 11:37:11PM +0200, Nicolas Iooss wrote:
> >> On Thu, Jun 1, 2017 at 7:05 PM, jwcart2  wrote:
> >>> On 06/01/2017 09:23 AM, Steve Lawrence wrote:
> 
>  - If two typealiasactual statements exist for the same typealias, we get
> a confusing error message mentioning that the actual arguement is not
> an alias, which is clearly allowed. This poor error occurs because the
> first typealiasactual statement resolves correctly, but when we
> resolve the alias in the second typealiasactual statement,
> cil_resolve_name tries to return what the alias points to, which is a
> type and not the required typealias. This patch creates a new function
> that does not perform the alias to actual conversion, used when we
> want an alias and not what the alias points to. This allows the
> cil_resolve_aliasactual to continue and reach the check for duplicate
> typealiasactual statements, resulting in a more meaningful error
> message.
> 
>  - Add back support for aliases to aliases (broken in 5c9fcb02e),
> while still ensuring that aliases point to either the correct actual
> flavor or alias flavor, and not something else like a typeattribute.
> 
>  Signed-off-by: Steve Lawrence 
> >>>
> >>>
> >>> I didn't even think of the case of an alias of an alias. Applied.
> >>>
> >>> Thanks,
> >>> Jim
> >>
> >> Hello,
> >> This patch broke secilc's test case. From secilc/ directory:
> >>
> >> $ ./secilc test/policy.cil
> >> cat0 is not a category. Only categories are allowed in
> >> categoryorder statements
> >> Failed to compile cildb: -1
> >>
> >> cat0 is defined as a categoryalias in secilc/test/policy.cil and is
> >> used in "(categoryorder (cat0 c1 c2 c3))". I do not have time right
> >> now to investigate further what is causing the issue, but reverting
> >> this commit (e501d3b6e8d2) fixes "make test".
> 
> Looks like an incorrect error check in the recent patch I sent out. It
> appears like typealiases are handled in many parts of the code and do
> not rely on cil_resolve_name to convert to the actual, so typealias
> appear to not be affected. But category and sensitivity resolution does
> rely on cil_resolve_name to do the conversion. A patch is coming to fix
> this.
> 
> >> Nicolas
> >> PS: if anyone is interested in the Travis-CI output of this bug, it is
> >> available on https://travis-ci.org/fishilico/selinux/builds/238505788
> >> .
> > 
> > There is still at least one typealias issue:
> > 
> > for example, as said, i have:
> > 
> > (typealias rpm_script_t)
> > (typealiasactual rpm_script_t rpm.script.subj)
> > 
> > where rpm.script.subj is a valid declared type
> > 
> > However when i, in addition and by mistake, try to declare rpm.script.subj 
> > typealias as per:
> > 
> > (typealias rpm.script.subj)
> 
> This is illegal syntax. Just like you cannot define a type as
> 
>   (type foo.bar.baz)
> 
> Instead, you need to use blocks if you want to namespace a typealias.
> 
>   (block rpm
> (block script
>   (typealias subj)
> )
>   )
> 
> > 
> > then it does not return: Re-declaration of typealias rpm.script.subj
> 
> Re-declaration checks happen in resolution, but since this is a syntax
> error it never even gets that far to do the check. The below error is
> expected.

Makes sense, thanks

> 
> > 
> > instead it returns:
> > 
> > Invalid character "." in rpm.script.subj
> > Invalid name
> > Failed to create node
> > 
> > This does "work" however with non-name spaced types:
> > 
> > (type a)
> > (typealias b)
> > (typealiasactual b a)
> > (typealias a)
> 
> This causes an error for me:

Yes, that is what i mean with "does work". It fails as expected but the error 
message makes sense.

Alright thanks that settles that then

> 
>   Re-declaration of typealias a
>   Failed to create node
>   Bad typealias declaration at policy.cil:13
> 
> That error is correct. You cannot define a type and typealias with the
> same name.
> 
> > regardless: the segfaults are gone. Thanks for this
> > 
> >>
> >>>
> >>>
>  ---
>    libsepol/cil/src/cil_resolve_ast.c | 48
>  --
>    libsepol/cil/src/cil_resolve_ast.h |  1 +
>    2 files changed, 32 insertions(+), 17 deletions(-)
> 
>  diff --git a/libsepol/cil/src/cil_resolve_ast.c
>  b/libsepol/cil/src/cil_resolve_ast.c
>  index 5c26530..fc44a5e 100644
>  --- a/libsepol/cil/src/cil_resolve_ast.c
>  +++ b/libsepol/cil/src/cil_resolve_ast.c
>  @@ -515,7 +515,7 @@ int cil_resolve_aliasactual(struct cil_tree_node
>  *current, void *extra_args, enu
>  goto exit;
>  }
>    - rc = cil_resolve_name(current, aliasactual->alias_str, sym_index,
>  

Re: [PATCH] libsepol/cil: better error message with duplicate aliases + support aliases to aliases

[Steve Lawrence]

On 06/02/2017 05:18 AM, Dominick Grift wrote:
> On Thu, Jun 01, 2017 at 11:37:11PM +0200, Nicolas Iooss wrote:
>> On Thu, Jun 1, 2017 at 7:05 PM, jwcart2  wrote:
>>> On 06/01/2017 09:23 AM, Steve Lawrence wrote:

 - If two typealiasactual statements exist for the same typealias, we get
a confusing error message mentioning that the actual arguement is not
an alias, which is clearly allowed. This poor error occurs because the
first typealiasactual statement resolves correctly, but when we
resolve the alias in the second typealiasactual statement,
cil_resolve_name tries to return what the alias points to, which is a
type and not the required typealias. This patch creates a new function
that does not perform the alias to actual conversion, used when we
want an alias and not what the alias points to. This allows the
cil_resolve_aliasactual to continue and reach the check for duplicate
typealiasactual statements, resulting in a more meaningful error
message.

 - Add back support for aliases to aliases (broken in 5c9fcb02e),
while still ensuring that aliases point to either the correct actual
flavor or alias flavor, and not something else like a typeattribute.

 Signed-off-by: Steve Lawrence 
>>>
>>>
>>> I didn't even think of the case of an alias of an alias. Applied.
>>>
>>> Thanks,
>>> Jim
>>
>> Hello,
>> This patch broke secilc's test case. From secilc/ directory:
>>
>> $ ./secilc test/policy.cil
>> cat0 is not a category. Only categories are allowed in
>> categoryorder statements
>> Failed to compile cildb: -1
>>
>> cat0 is defined as a categoryalias in secilc/test/policy.cil and is
>> used in "(categoryorder (cat0 c1 c2 c3))". I do not have time right
>> now to investigate further what is causing the issue, but reverting
>> this commit (e501d3b6e8d2) fixes "make test".

Looks like an incorrect error check in the recent patch I sent out. It
appears like typealiases are handled in many parts of the code and do
not rely on cil_resolve_name to convert to the actual, so typealias
appear to not be affected. But category and sensitivity resolution does
rely on cil_resolve_name to do the conversion. A patch is coming to fix
this.

>> Nicolas
>> PS: if anyone is interested in the Travis-CI output of this bug, it is
>> available on https://travis-ci.org/fishilico/selinux/builds/238505788
>> .
> 
> There is still at least one typealias issue:
> 
> for example, as said, i have:
> 
> (typealias rpm_script_t)
> (typealiasactual rpm_script_t rpm.script.subj)
> 
> where rpm.script.subj is a valid declared type
> 
> However when i, in addition and by mistake, try to declare rpm.script.subj 
> typealias as per:
> 
> (typealias rpm.script.subj)

This is illegal syntax. Just like you cannot define a type as

  (type foo.bar.baz)

Instead, you need to use blocks if you want to namespace a typealias.

  (block rpm
(block script
  (typealias subj)
)
  )

> 
> then it does not return: Re-declaration of typealias rpm.script.subj

Re-declaration checks happen in resolution, but since this is a syntax
error it never even gets that far to do the check. The below error is
expected.

> 
> instead it returns:
> 
> Invalid character "." in rpm.script.subj
> Invalid name
> Failed to create node
> 
> This does "work" however with non-name spaced types:
> 
> (type a)
> (typealias b)
> (typealiasactual b a)
> (typealias a)

This causes an error for me:

  Re-declaration of typealias a
  Failed to create node
  Bad typealias declaration at policy.cil:13

That error is correct. You cannot define a type and typealias with the
same name.

> regardless: the segfaults are gone. Thanks for this
> 
>>
>>>
>>>
 ---
   libsepol/cil/src/cil_resolve_ast.c | 48
 --
   libsepol/cil/src/cil_resolve_ast.h |  1 +
   2 files changed, 32 insertions(+), 17 deletions(-)

 diff --git a/libsepol/cil/src/cil_resolve_ast.c
 b/libsepol/cil/src/cil_resolve_ast.c
 index 5c26530..fc44a5e 100644
 --- a/libsepol/cil/src/cil_resolve_ast.c
 +++ b/libsepol/cil/src/cil_resolve_ast.c
 @@ -515,7 +515,7 @@ int cil_resolve_aliasactual(struct cil_tree_node
 *current, void *extra_args, enu
 goto exit;
 }
   - rc = cil_resolve_name(current, aliasactual->alias_str, sym_index,
 extra_args, _datum);
 +   rc = cil_resolve_name_keep_aliases(current,
 aliasactual->alias_str, sym_index, extra_args, _datum);
 if (rc != SEPOL_OK) {
 goto exit;
 }
 @@ -530,7 +530,7 @@ int cil_resolve_aliasactual(struct cil_tree_node
 *current, void *extra_args, enu
 goto exit;
 }
   - if (NODE(actual_datum)->flavor != flavor) {
 +   if (NODE(actual_datum)->flavor != flavor &&

Re: [PATCH] libsepol/cil: better error message with duplicate aliases + support aliases to aliases

[Dominick Grift]

On Thu, Jun 01, 2017 at 11:37:11PM +0200, Nicolas Iooss wrote:
> On Thu, Jun 1, 2017 at 7:05 PM, jwcart2  wrote:
> > On 06/01/2017 09:23 AM, Steve Lawrence wrote:
> >>
> >> - If two typealiasactual statements exist for the same typealias, we get
> >>a confusing error message mentioning that the actual arguement is not
> >>an alias, which is clearly allowed. This poor error occurs because the
> >>first typealiasactual statement resolves correctly, but when we
> >>resolve the alias in the second typealiasactual statement,
> >>cil_resolve_name tries to return what the alias points to, which is a
> >>type and not the required typealias. This patch creates a new function
> >>that does not perform the alias to actual conversion, used when we
> >>want an alias and not what the alias points to. This allows the
> >>cil_resolve_aliasactual to continue and reach the check for duplicate
> >>typealiasactual statements, resulting in a more meaningful error
> >>message.
> >>
> >> - Add back support for aliases to aliases (broken in 5c9fcb02e),
> >>while still ensuring that aliases point to either the correct actual
> >>flavor or alias flavor, and not something else like a typeattribute.
> >>
> >> Signed-off-by: Steve Lawrence 
> >
> >
> > I didn't even think of the case of an alias of an alias. Applied.
> >
> > Thanks,
> > Jim
> 
> Hello,
> This patch broke secilc's test case. From secilc/ directory:
> 
> $ ./secilc test/policy.cil
> cat0 is not a category. Only categories are allowed in
> categoryorder statements
> Failed to compile cildb: -1
> 
> cat0 is defined as a categoryalias in secilc/test/policy.cil and is
> used in "(categoryorder (cat0 c1 c2 c3))". I do not have time right
> now to investigate further what is causing the issue, but reverting
> this commit (e501d3b6e8d2) fixes "make test".
> 
> Nicolas
> PS: if anyone is interested in the Travis-CI output of this bug, it is
> available on https://travis-ci.org/fishilico/selinux/builds/238505788
> .

There is still at least one typealias issue:

for example, as said, i have:

(typealias rpm_script_t)
(typealiasactual rpm_script_t rpm.script.subj)

where rpm.script.subj is a valid declared type

However when i, in addition and by mistake, try to declare rpm.script.subj 
typealias as per:

(typealias rpm.script.subj)

then it does not return: Re-declaration of typealias rpm.script.subj

instead it returns:

Invalid character "." in rpm.script.subj
Invalid name
Failed to create node

This does "work" however with non-name spaced types:

(type a)
(typealias b)
(typealiasactual b a)
(typealias a)

regardless: the segfaults are gone. Thanks for this

> 
> >
> >
> >> ---
> >>   libsepol/cil/src/cil_resolve_ast.c | 48
> >> --
> >>   libsepol/cil/src/cil_resolve_ast.h |  1 +
> >>   2 files changed, 32 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/libsepol/cil/src/cil_resolve_ast.c
> >> b/libsepol/cil/src/cil_resolve_ast.c
> >> index 5c26530..fc44a5e 100644
> >> --- a/libsepol/cil/src/cil_resolve_ast.c
> >> +++ b/libsepol/cil/src/cil_resolve_ast.c
> >> @@ -515,7 +515,7 @@ int cil_resolve_aliasactual(struct cil_tree_node
> >> *current, void *extra_args, enu
> >> goto exit;
> >> }
> >>   - rc = cil_resolve_name(current, aliasactual->alias_str, sym_index,
> >> extra_args, _datum);
> >> +   rc = cil_resolve_name_keep_aliases(current,
> >> aliasactual->alias_str, sym_index, extra_args, _datum);
> >> if (rc != SEPOL_OK) {
> >> goto exit;
> >> }
> >> @@ -530,7 +530,7 @@ int cil_resolve_aliasactual(struct cil_tree_node
> >> *current, void *extra_args, enu
> >> goto exit;
> >> }
> >>   - if (NODE(actual_datum)->flavor != flavor) {
> >> +   if (NODE(actual_datum)->flavor != flavor &&
> >> NODE(actual_datum)->flavor != alias_flavor) {
> >> cil_log(CIL_ERR, "%s is a %s, but aliases a %s\n",
> >> alias_datum->name, cil_node_to_string(NODE(alias_datum)),
> >> cil_node_to_string(NODE(actual_datum)));
> >> rc = SEPOL_ERR;
> >> goto exit;
> >> @@ -539,7 +539,7 @@ int cil_resolve_aliasactual(struct cil_tree_node
> >> *current, void *extra_args, enu
> >> alias = (struct cil_alias *)alias_datum;
> >> if (alias->actual != NULL) {
> >> -   cil_log(CIL_ERR, "Alias cannot bind more than one
> >> value\n");
> >> +   cil_log(CIL_ERR, "%s %s cannot bind more than one
> >> value\n", cil_node_to_string(NODE(alias_datum)), alias_datum->name);
> >> rc = SEPOL_ERR;
> >> goto exit;
> >> }
> >> @@ -4122,6 +4122,34 @@ static int __cil_resolve_name_helper(struct cil_db
> >> *db, struct cil_tree_node *no
> >>   int cil_resolve_name(struct cil_tree_node *ast_node, char *name, enum
> >> cil_sym_index sym_index, void *extra_args, struct 

[PATCH] libsepol/cil: better error message with duplicate aliases + support aliases to aliases

[Steve Lawrence]

- If two typealiasactual statements exist for the same typealias, we get
  a confusing error message mentioning that the actual arguement is not
  an alias, which is clearly allowed. This poor error occurs because the
  first typealiasactual statement resolves correctly, but when we
  resolve the alias in the second typealiasactual statement,
  cil_resolve_name tries to return what the alias points to, which is a
  type and not the required typealias. This patch creates a new function
  that does not perform the alias to actual conversion, used when we
  want an alias and not what the alias points to. This allows the
  cil_resolve_aliasactual to continue and reach the check for duplicate
  typealiasactual statements, resulting in a more meaningful error
  message.

- Add back support for aliases to aliases (broken in 5c9fcb02e),
  while still ensuring that aliases point to either the correct actual
  flavor or alias flavor, and not something else like a typeattribute.

Signed-off-by: Steve Lawrence 
---
 libsepol/cil/src/cil_resolve_ast.c | 48 --
 libsepol/cil/src/cil_resolve_ast.h |  1 +
 2 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/libsepol/cil/src/cil_resolve_ast.c 
b/libsepol/cil/src/cil_resolve_ast.c
index 5c26530..fc44a5e 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -515,7 +515,7 @@ int cil_resolve_aliasactual(struct cil_tree_node *current, 
void *extra_args, enu
goto exit;
}
 
-   rc = cil_resolve_name(current, aliasactual->alias_str, sym_index, 
extra_args, _datum);
+   rc = cil_resolve_name_keep_aliases(current, aliasactual->alias_str, 
sym_index, extra_args, _datum);
if (rc != SEPOL_OK) {
goto exit;
}
@@ -530,7 +530,7 @@ int cil_resolve_aliasactual(struct cil_tree_node *current, 
void *extra_args, enu
goto exit;
}
 
-   if (NODE(actual_datum)->flavor != flavor) {
+   if (NODE(actual_datum)->flavor != flavor && NODE(actual_datum)->flavor 
!= alias_flavor) {
cil_log(CIL_ERR, "%s is a %s, but aliases a %s\n", 
alias_datum->name, cil_node_to_string(NODE(alias_datum)), 
cil_node_to_string(NODE(actual_datum)));
rc = SEPOL_ERR;
goto exit;
@@ -539,7 +539,7 @@ int cil_resolve_aliasactual(struct cil_tree_node *current, 
void *extra_args, enu
alias = (struct cil_alias *)alias_datum;
 
if (alias->actual != NULL) {
-   cil_log(CIL_ERR, "Alias cannot bind more than one value\n");
+   cil_log(CIL_ERR, "%s %s cannot bind more than one value\n", 
cil_node_to_string(NODE(alias_datum)), alias_datum->name);
rc = SEPOL_ERR;
goto exit;
}
@@ -4122,6 +4122,34 @@ static int __cil_resolve_name_helper(struct cil_db *db, 
struct cil_tree_node *no
 int cil_resolve_name(struct cil_tree_node *ast_node, char *name, enum 
cil_sym_index sym_index, void *extra_args, struct cil_symtab_datum **datum)
 {
int rc = SEPOL_ERR;
+   struct cil_tree_node *node = NULL;
+
+   rc = cil_resolve_name_keep_aliases(ast_node, name, sym_index, 
extra_args, datum);
+   if (rc != SEPOL_ERR) {
+   goto exit;
+   }
+
+   /* If this datum is an alias, then return the actual node
+* This depends on aliases already being processed
+*/
+   node = NODE(*datum);
+   if (node->flavor == CIL_TYPEALIAS || node->flavor == CIL_SENSALIAS
+   || node->flavor == CIL_CATALIAS) {
+   struct cil_alias *alias = (struct cil_alias *)(*datum);
+   if (alias->actual) {
+   *datum = alias->actual;
+   }
+   }
+
+   rc = SEPOL_OK;
+
+exit:
+   return rc;
+}
+
+int cil_resolve_name_keep_aliases(struct cil_tree_node *ast_node, char *name, 
enum cil_sym_index sym_index, void *extra_args, struct cil_symtab_datum **datum)
+{
+   int rc = SEPOL_ERR;
struct cil_args_resolve *args = extra_args;
struct cil_db *db = args->db;
struct cil_tree_node *node = NULL;
@@ -4208,20 +4236,6 @@ exit:
*datum = NULL;
}
 
-   if (*datum != NULL) {
-   /* If this datum is an alias, then return the actual node
-* This depends on aliases already being processed
-*/
-   node = NODE(*datum);
-   if (node->flavor == CIL_TYPEALIAS || node->flavor == 
CIL_SENSALIAS
-   || node->flavor == CIL_CATALIAS) {
-   struct cil_alias *alias = (struct cil_alias *)(*datum);
-   if (alias->actual) {
-   *datum = alias->actual;
-   }
-   }
-   }
-
args->last_resolved_name = name;
 
return rc;
diff --git a/libsepol/cil/src/cil_resolve_ast.h 
b/libsepol/cil/src/cil_resolve_ast.h