[PATCH 1/1] libsepol: cil: Add ability to redeclare types[attributes]

[Daniel Cashman]

From: Dan Cashman <***@google.com>

Signed-off-by: Dan Cashman <***@android.com>
Change-Id: I5a72f2e6c339baee8ebc07e3e9176296320e2c80
---
libsepol/cil/include/cil/cil.h | 1 +
libsepol/cil/src/cil.c | 5 +++++
libsepol/cil/src/cil_build_ast.c | 46 ++++++++++++++++++++++++++++++++++------
libsepol/cil/src/cil_internal.h | 1 +
secilc/secilc.c | 9 +++++++-
5 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/libsepol/cil/include/cil/cil.h b/libsepol/cil/include/cil/cil.h
index 4507892c..4df646a0 100644
--- a/libsepol/cil/include/cil/cil.h
+++ b/libsepol/cil/include/cil/cil.h
@@ -46,6 +46,7 @@ extern int cil_userprefixes_to_string(cil_db_t *db, char **out, size_t *size);
extern int cil_selinuxusers_to_string(cil_db_t *db, char **out, size_t *size);
extern int cil_filecons_to_string(cil_db_t *db, char **out, size_t *size);
extern void cil_set_disable_dontaudit(cil_db_t *db, int disable_dontaudit);
+extern void cil_set_multiple_decls(cil_db_t *db, int multiple_decls);
extern void cil_set_disable_neverallow(cil_db_t *db, int disable_neverallow);
extern void cil_set_preserve_tunables(cil_db_t *db, int preserve_tunables);
extern int cil_set_handle_unknown(cil_db_t *db, int handle_unknown);
diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
index 9b9ccc36..e8bbbfdf 100644
--- a/libsepol/cil/src/cil.c
+++ b/libsepol/cil/src/cil.c
@@ -1675,6 +1675,11 @@ void cil_set_mls(struct cil_db *db, int mls)
db->mls = mls;
}

+void cil_set_multiple_decls(struct cil_db *db, int multiple_decls)
+{
+ db->multiple_decls = multiple_decls;
+}
+
void cil_set_target_platform(struct cil_db *db, int target_platform)
{
db->target_platform = target_platform;
diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 36cc6735..9a2fd0d5 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -82,10 +82,33 @@ exit:
return rc;
}

+/*
+ * Determine whether or not multiple declarations of the same key can share a
+ * datum, given the new datum and the one already present in a given symtab.
+ */
+int cil_is_datum_multiple_decl(__attribute__((unused)) struct cil_symtab_datum *cur,
+ __attribute__((unused)) struct cil_symtab_datum *old,
+ enum cil_flavor f)
+{
+ int rc = CIL_FALSE;
+
+ switch (f) {
+ case CIL_TYPE:
+ case CIL_TYPEATTRIBUTE:
+ /* type and typeattribute statements insert empty datums, ret true */
+ rc = CIL_TRUE;
+ break;
+ default:
+ break;
+ }
+ return rc;
+}
+
int cil_gen_node(__attribute__((unused)) struct cil_db *db, struct cil_tree_node *ast_node, struct cil_symtab_datum *datum, hashtab_key_t key, enum cil_sym_index sflavor, enum cil_flavor nflavor)
{
int rc = SEPOL_ERR;
symtab_t *symtab = NULL;
+ struct cil_symtab_datum *prev;

rc = __cil_verify_name((const char*)key);
if (rc != SEPOL_OK) {
@@ -103,15 +126,24 @@ int cil_gen_node(__attribute__((unused)) struct cil_db *db, struct cil_tree_node
if (symtab != NULL) {
rc = cil_symtab_insert(symtab, (hashtab_key_t)key, datum, ast_node);
if (rc == SEPOL_EEXIST) {
- cil_log(CIL_ERR, "Re-declaration of %s %s\n",
- cil_node_to_string(ast_node), key);
- if (cil_symtab_get_datum(symtab, key, &datum) == SEPOL_OK) {
- if (sflavor == CIL_SYM_BLOCKS) {
- struct cil_tree_node *node = datum->nodes->head->data;
- cil_tree_log(node, CIL_ERR, "Previous declaration");
+ if (!db->multiple_decls ||
+ cil_symtab_get_datum(symtab, (hashtab_key_t)key, &prev) != SEPOL_OK ||
+ !cil_is_datum_multiple_decl(datum, prev, nflavor)) {
+
+ /* multiple_decls not ok, ret error */
+ cil_log(CIL_ERR, "Re-declaration of %s %s\n",
+ cil_node_to_string(ast_node), key);
+ if (cil_symtab_get_datum(symtab, key, &datum) == SEPOL_OK) {
+ if (sflavor == CIL_SYM_BLOCKS) {
+ struct cil_tree_node *node = datum->nodes->head->data;
+ cil_tree_log(node, CIL_ERR, "Previous declaration");
+ }
}
+ goto exit;
}
- goto exit;
+ /* multiple_decls is enabled and works for this datum type, add node */
+ cil_list_append(prev->nodes, CIL_NODE, ast_node);
+ ast_node->data = prev;
}
}

diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
index aee3f00c..abfacd8d 100644
--- a/libsepol/cil/src/cil_internal.h
+++ b/libsepol/cil/src/cil_internal.h
@@ -312,6 +312,7 @@ struct cil_db {
int preserve_tunables;
int handle_unknown;
int mls;
+ int multiple_decls;
int target_platform;
int policy_version;
};
diff --git a/secilc/secilc.c b/secilc/secilc.c
index f2232e72..0be6975b 100644
--- a/secilc/secilc.c
+++ b/secilc/secilc.c
@@ -63,6 +63,7 @@ static __attribute__((__noreturn__)) void usage(const char *prog)
printf(" statement if present in the policy\n");
printf(" -D, --disable-dontaudit do not add dontaudit rules to the binary policy\n");
printf(" -P, --preserve-tunables treat tunables as booleans\n");
+ printf(" -m, --multiple-decls allow some statements to be re-declared\n");
printf(" -N, --disable-neverallow do not check neverallow rules\n");
printf(" -G, --expand-generated Expand and remove auto-generated attributes\n");
printf(" -X, --expand-size <SIZE> Expand type attributes with fewer than <SIZE>\n");
@@ -89,6 +90,7 @@ int main(int argc, char *argv[])
int target = SEPOL_TARGET_SELINUX;
int mls = -1;
int disable_dontaudit = 0;
+ int multiple_decls = 0;
int disable_neverallow = 0;
int preserve_tunables = 0;
int handle_unknown = -1;
@@ -108,6 +110,7 @@ int main(int argc, char *argv[])
{"policyversion", required_argument, 0, 'c'},
{"handle-unknown", required_argument, 0, 'U'},
{"disable-dontaudit", no_argument, 0, 'D'},
+ {"multiple-decls", no_argument, 0, 'm'},
{"disable-neverallow", no_argument, 0, 'N'},
{"preserve-tunables", no_argument, 0, 'P'},
{"output", required_argument, 0, 'o'},
@@ -119,7 +122,7 @@ int main(int argc, char *argv[])
int i;

while (1) {
- opt_char = getopt_long(argc, argv, "o:f:U:hvt:M:PDNc:GX:", long_opts, &opt_index);
+ opt_char = getopt_long(argc, argv, "o:f:U:hvt:M:PDmNc:GX:", long_opts, &opt_index);
if (opt_char == -1) {
break;
}
@@ -175,6 +178,9 @@ int main(int argc, char *argv[])
case 'D':
disable_dontaudit = 1;
break;
+ case 'm':
+ multiple_decls = 1;
+ break;
case 'N':
disable_neverallow = 1;
break;
@@ -223,6 +229,7 @@ int main(int argc, char *argv[])

cil_db_init(&db);
cil_set_disable_dontaudit(db, disable_dontaudit);
+ cil_set_multiple_decls(db, multiple_decls);
cil_set_disable_neverallow(db, disable_neverallow);
cil_set_preserve_tunables(db, preserve_tunables);
if (handle_unknown != -1) {

[jwcart2]

Post by Daniel Cashman
Change-Id: I5a72f2e6c339baee8ebc07e3e9176296320e2c80
---
libsepol/cil/include/cil/cil.h | 1 +
libsepol/cil/src/cil.c | 5 +++++
libsepol/cil/src/cil_build_ast.c | 46 ++++++++++++++++++++++++++++++++++------
libsepol/cil/src/cil_internal.h | 1 +
secilc/secilc.c | 9 +++++++-
5 files changed, 54 insertions(+), 8 deletions(-)
diff --git a/libsepol/cil/include/cil/cil.h b/libsepol/cil/include/cil/cil.h
index 4507892c..4df646a0 100644
--- a/libsepol/cil/include/cil/cil.h
+++ b/libsepol/cil/include/cil/cil.h
@@ -46,6 +46,7 @@ extern int cil_userprefixes_to_string(cil_db_t *db, char **out, size_t *size);
extern int cil_selinuxusers_to_string(cil_db_t *db, char **out, size_t *size);
extern int cil_filecons_to_string(cil_db_t *db, char **out, size_t *size);
extern void cil_set_disable_dontaudit(cil_db_t *db, int disable_dontaudit);
+extern void cil_set_multiple_decls(cil_db_t *db, int multiple_decls);
extern void cil_set_disable_neverallow(cil_db_t *db, int disable_neverallow);
extern void cil_set_preserve_tunables(cil_db_t *db, int preserve_tunables);
extern int cil_set_handle_unknown(cil_db_t *db, int handle_unknown);
diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
index 9b9ccc36..e8bbbfdf 100644
--- a/libsepol/cil/src/cil.c
+++ b/libsepol/cil/src/cil.c
@@ -1675,6 +1675,11 @@ void cil_set_mls(struct cil_db *db, int mls)
db->mls = mls;
}
+void cil_set_multiple_decls(struct cil_db *db, int multiple_decls)
+{
+ db->multiple_decls = multiple_decls;
+}
+
void cil_set_target_platform(struct cil_db *db, int target_platform)
{
db->target_platform = target_platform;
diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 36cc6735..9a2fd0d5 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
return rc;
}
+/*
+ * Determine whether or not multiple declarations of the same key can share a
+ * datum, given the new datum and the one already present in a given symtab.
+ */
+int cil_is_datum_multiple_decl(__attribute__((unused)) struct cil_symtab_datum *cur,
+ __attribute__((unused)) struct cil_symtab_datum *old,
+ enum cil_flavor f)
+{
+ int rc = CIL_FALSE;
+
+ switch (f) {
+ /* type and typeattribute statements insert empty datums, ret true */
+ rc = CIL_TRUE;
+ break;
+ break;
+ }
+ return rc;
+}
+
int cil_gen_node(__attribute__((unused)) struct cil_db *db, struct cil_tree_node *ast_node, struct cil_symtab_datum *datum, hashtab_key_t key, enum cil_sym_index sflavor, enum cil_flavor nflavor)
{
int rc = SEPOL_ERR;
symtab_t *symtab = NULL;
+ struct cil_symtab_datum *prev;
rc = __cil_verify_name((const char*)key);
if (rc != SEPOL_OK) {
@@ -103,15 +126,24 @@ int cil_gen_node(__attribute__((unused)) struct cil_db *db, struct cil_tree_node
if (symtab != NULL) {
rc = cil_symtab_insert(symtab, (hashtab_key_t)key, datum, ast_node);
if (rc == SEPOL_EEXIST) {
- cil_log(CIL_ERR, "Re-declaration of %s %s\n",
- cil_node_to_string(ast_node), key);
- if (cil_symtab_get_datum(symtab, key, &datum) == SEPOL_OK) {
- if (sflavor == CIL_SYM_BLOCKS) {
- struct cil_tree_node *node = datum->nodes->head->data;
- cil_tree_log(node, CIL_ERR, "Previous declaration");
+ if (!db->multiple_decls ||
+ cil_symtab_get_datum(symtab, (hashtab_key_t)key, &prev) != SEPOL_OK ||
+ !cil_is_datum_multiple_decl(datum, prev, nflavor)) {
+
+ /* multiple_decls not ok, ret error */
+ cil_log(CIL_ERR, "Re-declaration of %s %s\n",
+ cil_node_to_string(ast_node), key);
+ if (cil_symtab_get_datum(symtab, key, &datum) == SEPOL_OK) {
+ if (sflavor == CIL_SYM_BLOCKS) {
+ struct cil_tree_node *node = datum->nodes->head->data;
+ cil_tree_log(node, CIL_ERR, "Previous declaration");
+ }
}
+ goto exit;
}
- goto exit;
+ /* multiple_decls is enabled and works for this datum type, add node */
+ cil_list_append(prev->nodes, CIL_NODE, ast_node);
+ ast_node->data = prev;

You also need:
cil_symtab_datum_destroy(datum);
free(datum);

Post by Daniel Cashman
}
}
diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
index aee3f00c..abfacd8d 100644
--- a/libsepol/cil/src/cil_internal.h
+++ b/libsepol/cil/src/cil_internal.h
@@ -312,6 +312,7 @@ struct cil_db {
int preserve_tunables;
int handle_unknown;
int mls;
+ int multiple_decls;
int target_platform;
int policy_version;
};

You also need to add cil_set_multiple_decls to libsepol/src/libsepol.map.in

Post by Daniel Cashman
diff --git a/secilc/secilc.c b/secilc/secilc.c
index f2232e72..0be6975b 100644
--- a/secilc/secilc.c
+++ b/secilc/secilc.c
@@ -63,6 +63,7 @@ static __attribute__((__noreturn__)) void usage(const char *prog)
printf(" statement if present in the policy\n");
printf(" -D, --disable-dontaudit do not add dontaudit rules to the binary policy\n");
printf(" -P, --preserve-tunables treat tunables as booleans\n");
+ printf(" -m, --multiple-decls allow some statements to be re-declared\n");
printf(" -N, --disable-neverallow do not check neverallow rules\n");
printf(" -G, --expand-generated Expand and remove auto-generated attributes\n");
printf(" -X, --expand-size <SIZE> Expand type attributes with fewer than <SIZE>\n");
@@ -89,6 +90,7 @@ int main(int argc, char *argv[])
int target = SEPOL_TARGET_SELINUX;
int mls = -1;
int disable_dontaudit = 0;
+ int multiple_decls = 0;
int disable_neverallow = 0;
int preserve_tunables = 0;
int handle_unknown = -1;
@@ -108,6 +110,7 @@ int main(int argc, char *argv[])
{"policyversion", required_argument, 0, 'c'},
{"handle-unknown", required_argument, 0, 'U'},
{"disable-dontaudit", no_argument, 0, 'D'},
+ {"multiple-decls", no_argument, 0, 'm'},
{"disable-neverallow", no_argument, 0, 'N'},
{"preserve-tunables", no_argument, 0, 'P'},
{"output", required_argument, 0, 'o'},
@@ -119,7 +122,7 @@ int main(int argc, char *argv[])
int i;
while (1) {
- opt_char = getopt_long(argc, argv, "o:f:U:hvt:M:PDNc:GX:", long_opts, &opt_index);
+ opt_char = getopt_long(argc, argv, "o:f:U:hvt:M:PDmNc:GX:", long_opts, &opt_index);
if (opt_char == -1) {
break;
}
@@ -175,6 +178,9 @@ int main(int argc, char *argv[])
disable_dontaudit = 1;
break;
+ multiple_decls = 1;
+ break;
disable_neverallow = 1;
break;
@@ -223,6 +229,7 @@ int main(int argc, char *argv[])
cil_db_init(&db);
cil_set_disable_dontaudit(db, disable_dontaudit);
+ cil_set_multiple_decls(db, multiple_decls);
cil_set_disable_neverallow(db, disable_neverallow);
cil_set_preserve_tunables(db, preserve_tunables);
if (handle_unknown != -1) {

Overall, the patch seems to work. I am a little worried that I might be missing
a particular corner case where it won't.

Jim

[jwcart2]

In Android O, the SELinux policy was split from a monolithic policy
created at build-time for each device into two main components, one
on /system and one on /vendor, which get combined at boot. This
introduced several new challenges, including the creation of a need
to maintain compatibility between the two policies, i.e. new policy
on the /system image needs to work with the /vendor policy that is
based on the policy it's replacing, and updates to the /vendor policy
need to work with the /system policy.
The ultimate goal of this is to allow updates to /system without
changing /vendor. Android currently accomplishes this by exporting
'public' policy types that /vendor policy can use. It makes sure that
changes in object labels don't break the /vendor policy by changing all
public types into attributes, copying the public policy into the /vendor
policy and keepign a mapping between public types and the attributes
that are included in the /vendor policy.
This leads to complications, though, when trying to use different /system
images with the same /vendor image: the /vendor policy might rely on a
/system policy to provide a type definition or an attribute definition,
but another, non-customized /system policy might not provide it.

I can see were this can help prevent the policy failing to compile, but it seems
like there can still be run time problems. If one system policy defines type A
and has rules for type A and another doesn't, then, even if the vendor policy
defines type A (so there is no compile error), it would still be missing the
right allow rules and it could experience runtime denials.

That being said, this could be useful in some cases, such as virus scanning in
Fedora's policy, where multiple modules would like to use the same type and it
might be possible to have more than one of them loaded.

Jim

Luckily, CIL statements are often reduced to the minimum amount of
information needed and are generally order-independent. Type statements,
for instance, are just the type keyword followed by an identifier, and I
believe nothing is lost in allowing them to be repeated.
This patch is a proposal for one way to allow multiple declarations. At
the moment, android is primarily concerned with types and attributes, but
this could (should!) be extended to many other declaration types.
The proposal here is to modify cil_gen_node() to check to see if the cil_db
supports multiple declarations, and if so, to check whether or not the
repeated symbol is eligible to share the existing, already-stored datum. The
only types considered so far have been CIL_TYPE and CIL_TYPEATTRIBUTE, both
of which intall empty datums during AST building, so they automatically return
true.
libsepol: cil: Add ability to redeclare types[attributes]
libsepol/cil/include/cil/cil.h | 1 +
libsepol/cil/src/cil.c | 5 +++++
libsepol/cil/src/cil_build_ast.c | 46 ++++++++++++++++++++++++++++++++++------
libsepol/cil/src/cil_internal.h | 1 +
secilc/secilc.c | 9 +++++++-
5 files changed, 54 insertions(+), 8 deletions(-)

[Dan Cashman]

Thanks for the response and sorry for the delay; I was on a long
excursion to experience less sunlight than in a typical week (went on
vacation to see the eclipse, among other things), without much connectivity.

Post by jwcart2

In Android O, the SELinux policy was split from a monolithic policy
created at build-time for each device into two main components, one
on /system and one on /vendor, which get combined at boot. This
introduced several new challenges, including the creation of a need
to maintain compatibility between the two policies, i.e. new policy
on the /system image needs to work with the /vendor policy that is
based on the policy it's replacing, and updates to the /vendor policy
need to work with the /system policy.
The ultimate goal of this is to allow updates to /system without
changing /vendor. Android currently accomplishes this by exporting
'public' policy types that /vendor policy can use. It makes sure that
changes in object labels don't break the /vendor policy by changing all
public types into attributes, copying the public policy into the /vendor
policy and keepign a mapping between public types and the attributes
that are included in the /vendor policy.
This leads to complications, though, when trying to use different /system
images with the same /vendor image: the /vendor policy might rely on a
/system policy to provide a type definition or an attribute definition,
but another, non-customized /system policy might not provide it.

I can see were this can help prevent the policy failing to compile, but
it seems like there can still be run time problems. If one system policy
defines type A and has rules for type A and another doesn't, then, even
if the vendor policy defines type A (so there is no compile error), it
would still be missing the right allow rules and it could experience
runtime denials.
That being said, this could be useful in some cases, such as virus
scanning in Fedora's policy, where multiple modules would like to use
the same type and it might be possible to have more than one of them
loaded.
Jim

We hope to avoid runtime issues by copying the 'public' policy on which
the policy in the /vendor image may rely into that policy directly. As
a result, the allow rules are copied over as well (but the types are
converted to attributes to attempt to prevent other run-time breakage
due to changing labels). As an aside, "attribute-izing" can be seen in
the modifications to cil in libsepol here:
https://android.googlesource.com/platform/external/selinux/+/master/libsepol/cil/include/cil/android.h
which I'd also like to begin the process of upstreaming, but view as
independent of this patch.

It sounds like this would be acceptable in theory, so I'll fix up the
patch to address your comments and submit again.

Thanks,
Dan