From: James Carter <jwcart2@gmail.com>
To: Nicolas Iooss <nicolas.iooss@m4x.org>
Cc: SElinux list <selinux@vger.kernel.org>
Subject: Re: [PATCH 1/4] libsepol/cil: Provide option to allow qualified names in declarations
Date: Mon, 28 Jun 2021 16:48:27 -0400 [thread overview]
Message-ID: <CAP+JOzS+Bn4L-y9BL=5+3+GZ__TCX+9kG4RAta-JP3LoSTJCdw@mail.gmail.com> (raw)
In-Reply-To: <CAJfZ7==EtyPWdrC4Wz3bK=yf0PFF18b6CcVnjHfXF+4knjopPg@mail.gmail.com>
On Sat, Jun 26, 2021 at 5:46 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Thu, Jun 24, 2021 at 9:59 PM James Carter <jwcart2@gmail.com> 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@gmail.com>
> > ---
> > 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.
Thanks for the review, I will work on a v2.
Jim
> Cheers,
> Nicolas
>
next prev parent reply other threads:[~2021-06-28 20:48 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-24 19:59 [PATCH 1/4] libsepol/cil: Provide option to allow qualified names in declarations James Carter
2021-06-24 19:59 ` [PATCH 2/4] secilc: Add support for using qualified names to secilc James Carter
2021-06-24 19:59 ` [PATCH 3/4] libsepol/cil: Add support for using qualified names to secil2tree James Carter
2021-06-24 19:59 ` [PATCH 4/4] libsepol/cil: Add support for using qualified names to secil2conf James Carter
2021-06-24 20:20 ` [PATCH 1/4] libsepol/cil: Provide option to allow qualified names in declarations Dominick Grift
2021-06-24 20:35 ` James Carter
2021-06-24 20:38 ` Dominick Grift
2021-06-26 9:46 ` Nicolas Iooss
2021-06-28 20:48 ` James Carter [this message]
2021-06-30 18:55 ` Nicolas Iooss
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAP+JOzS+Bn4L-y9BL=5+3+GZ__TCX+9kG4RAta-JP3LoSTJCdw@mail.gmail.com' \
--to=jwcart2@gmail.com \
--cc=nicolas.iooss@m4x.org \
--cc=selinux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).