SELinuxProject / selint

Static code analysis of refpolicy style SELinux policy
Apache License 2.0
38 stars 17 forks source link

argument parsing doesn't allow ranges #213

Closed jcassagnol-public closed 2 years ago

jcassagnol-public commented 3 years ago

This one I'm not sure how to handle, but if you have the following te file: (edit: see below comments for update, the original assumption that ranges were allowed by the compiler was wrong)

policy_module(example, 1.0.0)

type abc_t;
type abc_exec_t;
allow abc_t abc_exec_t:file {read };
example_template(abc_t, s0:c0-s15:c100.c102);

and the following if file:

interface(`example_template',`
    gen_require(`
                type abc_t;
    ')

    range_transition $1 abc_t:file $2;
')

selint fails to parse as it considers it an unexpected COLON:

example.te:           6: (F): syntax error, unexpected COLON (F-001)
    6 | example_template(abc_t, s0:c0-s15:c100.c102);
      |                           ^
example.te:           6: (F): Error: Invalid statement (F-001)
    6 | example_template(abc_t, s0:c0-s15:c100.c102);
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
Error: Failed to parse files

Just adding mls_range to arg results in a shift-reduce error, which I think is due to the combo of the xperm_list and sl_item both parsing out to dashes at some point, but I'm not sure, as the output of YACC is vague.

cgzones commented 3 years ago

Does the literal mls range s0:c0-s15:c100.c102 work for you? I needed to add spaces around the dash (cause a dash is a valid identifier character) in order for the compiler to accept it.

214 accepts single quoted MLS ranges.

jcassagnol-public commented 3 years ago

Apologies for the delay in getting back to you. The original bug was

policy_module(example, 1.0.0)

type abc_t;
type abc_exec_t;
allow abc_t abc_exec_t:file {read };
example_template(abc_t, s0:c0);

The SELinux complier is happy to compile that line for some reason.

I thought that the MLS ranges were treated similarly by the SELinux compiler and wrote the bug report for what I thought would be the generic case (but didn't bother to test that one).

The PR is a good patch, I'm starting to think that the SELinux compiler is accepting example_template(abc_t, s0:c0); by accident. :(

jcassagnol-public commented 3 years ago

Yeah this is odd, the compiler even accepts

policy_module(example, 1.0.0)

type abc_t;
type abc_exec_t;
allow abc_t abc_exec_t:file {read };
example_template(abc_t, s15:c10.c100);

But does not like the range statement without quotes or without the space. Does this behavior make sense?

cgzones commented 3 years ago

Yes, - is a valid character for identifiers, so when using s0:c0-s15:c100.c102 checkmodule(8)/checkpolicy(8) does not parse it as s0(sensitivity identifier) :(category separator) c0(category identifier) -(range separator) s15(sensitivity identifier) ... but s0(sensitivity identifier) :(category separator) c0-s15(category identifier) :(invalid colon!)

jcassagnol-public commented 3 years ago

verified #214 fixes mls ranges in the argument list.

Could the arg parser be changed to also parse mls components as below? It would kinda be nice. I have a feeling we're one of the only folks out there who use MLS, it would let us pass the mls components without quoting to the arguments. 😁

diff --git a/src/parse.y b/src/parse.y
index c4fa267..ee4d47c 100644
--- a/src/parse.y
+++ b/src/parse.y
@@ -720,6 +720,13 @@ m4_argument:
        ;

 arg:
+       mls_component COLON mls_component { char* strcpy;
+                       size_t len = strlen($1) + strlen($3) + 1 /* COLON */ + 1 /* NT */;
+                       strcpy = malloc(len);
+                       snprintf(strcpy, len, "%s:%s", $1, $3);
+                       $$ = sl_from_str_consume(strcpy);
+                       free($1); free($3); }
+       |
        xperm_list
        |
        QUOTED_STRING { $$ = sl_from_str_consume($1); }

edit: I waive any right to copyright on the patch given.