On Mon, Jun 28, 2021 at 10:48 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> On Sat, Jun 26, 2021 at 5:46 AM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote:
> >
> > On Thu, Jun 24, 2021 at 9:59 PM James Carter <jwcart2@xxxxxxxxx> wrote:
> > >
> > > Qualified names have "dots" in them. They are generated when a CIL
> > > policy is compiled and come from declarations in blocks. If a kernel
> > > policy is decompiled into a CIL policy, the resulting policy could
> > > have decarations that use qualified names. Compiling this policy would
> >
> > Misspelling: decarations -> declarations
> >
>
> Thanks, I'll fix that in v2.
>
> > > result in an error because "dots" in declarations are not allowed.
> > >
> > > Qualified names in a policy are normally used to refer to the name of
> > > identifiers, blocks, macros, or optionals that are declared in a
> > > different block (that is not a parent). Name resolution is based on
> > > splitting a name based on the "dots", searching the parents up to the
> > > global namespace for the first block using the first part of the name,
> > > using the second part of the name to lookup the next block using the
> > > first block's symbol tables, looking up the third block in the second's
> > > symbol tables, and so on.
> > >
> > > To allow the option of using qualified names in declarations:
> > >
> > > 1) Create a field in the struct cil_db called "qualified_names" which
> > > is set to CIL_TRUE when qualified names are to be used. This field is
> > > checked in cil_verify_name() and "dots" are allowed if qualified names
> > > are being allowed.
> > >
> > > 2) Only allow the direct lookup of the whole name in the global symbol
> > > table. This means that blocks, blockinherits, blockabstracts, and in-
> > > statements cannot be allowed. Use the "qualified_names" field of the
> > > cil_db to know when using one of these should result in an error.
> > >
> > > 3) Create the function cil_set_qualified_names() that is used to set
> > > the "qualified_names" field. Export the function in libsepol.
> > >
> > > Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> > > ---
> > > libsepol/cil/include/cil/cil.h | 1 +
> > > libsepol/cil/src/cil.c | 6 ++++++
> > > libsepol/cil/src/cil_build_ast.c | 24 ++++++++++++++++++++++--
> > > libsepol/cil/src/cil_internal.h | 1 +
> > > libsepol/cil/src/cil_resolve_ast.c | 4 ++--
> > > libsepol/cil/src/cil_verify.c | 19 ++++++++++++++-----
> > > libsepol/cil/src/cil_verify.h | 2 +-
> > > libsepol/src/libsepol.map.in | 1 +
> > > 8 files changed, 48 insertions(+), 10 deletions(-)
> > >
> > [...]
> > > diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
> > > index 59397f70..9cb1a6f6 100644
> > > --- a/libsepol/cil/src/cil_verify.c
> > > +++ b/libsepol/cil/src/cil_verify.c
> > > @@ -92,7 +92,7 @@ static int __cil_is_reserved_name(const char *name, enum cil_flavor flavor)
> > > return CIL_FALSE;
> > > }
> > >
> > > -int cil_verify_name(const char *name, enum cil_flavor flavor)
> > > +int cil_verify_name(struct cil_db *db, const char *name, enum cil_flavor flavor)
> > > {
> > > int rc = SEPOL_ERR;
> > > int len;
> > > @@ -116,10 +116,19 @@ int cil_verify_name(const char *name, enum cil_flavor flavor)
> > > goto exit;
> > > }
> > >
> > > - for (i = 1; i < len; i++) {
> > > - if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
> > > - cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> > > - goto exit;
> > > + if (db->qualified_names == CIL_FALSE) {
> > > + for (i = 1; i < len; i++) {
> > > + if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
> > > + cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> > > + goto exit;
> > > + }
> > > + }
> > > + } else {
> > > + for (i = 1; i < len; i++) {
> > > + if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-' && name[i] != '.') {
> > > + cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> > > + goto exit;
> > > + }
> > > }
> > > }
> >
> > As cil_verify_name does not modify db (and would be seen as a function
> > which does not modify anything and only checks some rules), it would
> > be better to add a const qualifier:
> >
> > int cil_verify_name(const struct cil_db *db, const char *name,
> > enum cil_flavor flavor)
> >
>
> Good suggestion.
>
> > Other than that, the documentation of the new option,
> > "--qualified-names Use qualified names." make me feel like the
> > wording can be improved. More precisely, a commit message contains
> > "Using qualified names means that declaration names can have dots in
> > them" and this definition should also be in the documentation. So I am
> > suggesting to replace the documentation with:
> >
> > Allow names containing dots (qualified names). Blocks,
> > blockinherits, blockabstracts, and in-statements will not be allowed.
> >
>
> Sounds clearer.
>
> > Other than that, when I tested "secil2tree -A resolve -o test.cil
> > secilc/test/policy.cil && secilc -Q test.cil" I encountered other
> > errors, which are not related to these patches. When modifying the
> > resulting "test.cil" to make it compile (by removing blocks and
> > renaming some objects), option -Q worked fine. It would be nice if
> > such a command was integrated in secilc's tests, in order to prevent
> > future regressions. Nevertheless such work can be done once this
> > series is merged.
> >
>
> I might not have been clear. This patch doesn't allow you to compile
> after using "-A resolve". I will be working on that next.
> I am thinking of creating a generic block "<block>", so blocks that
> are resolved can be re-written using that.
>
> So when writing out the AST for the resolve phase,
>
> (block b
> (type t)
> (allow t self (CLASS (PERM)))
> )
>
> would be written as
>
> ;block b
> (<block>
> (type b.t)
> (allow b.t self (CLASS (PERM)))
> )
>
> or, maybe I would also create a comment statement that wouldn't be
> discarded by the parser
>
> (<comment> "block b")
> (<block>
> (type b.t)
> (allow b.t self (CLASS (PERM)))
> )
>
> I need to do similar things to blockinherits, blockabstracts, and
> in-statements. I also would like to be able add a comment when an
> optional is disabled.
>
> I am open to suggestions.
These suggestions look good to me. Actually I even began writing a
small patch to show resolved blockinherit statements as "( ;
blockinherit b", because otherwise the parentheses are wrong in the
output of "secil2tree -A resolve secilc/test/policy.cil".
About introducing a new comment statement, it sounds like a whole new
feature just designed to make a niche tool work in a simple way. In my
humble opinion I prefer seeing regular comments in secil2tree dumps,
which has the advantage of being an existing syntax, being easy to
read, etc. But this is what I feel and of course if you prefer the way
you suggest or if someone else has a different preference, it would be
all right. And I might have missed the whole point of introducing
comments which are not discarded by CIL parsers and what advantage
this approach gives.
Cheers,
Nicolas
