libsepol: error in CIL if a permission cannot be resolved

Commit Message

Yuli Khodorkovskiy June 13, 2019, 8:18 p.m. UTC

In the following example, "relabeltoo" is not a valid permission
in the loaded policy nor in the new module. Before, CIL would not
complain about the invalid permission and proceed to install the module:

	$ cat test.cil

	(mlsconstrain (db_procedure (create relabeltoo)) (eq l2 h2))

With this patch, an error is now prompted to a user:

	$ sudo semodule -i foo.cil

	Failed to resolve permission relabeltoo
	Failed to resolve mlsconstrain statement at /etc/selinux/mls/tmp/modules/400/test/cil:1
	semodule:  Failed!

Signed-off-by: Yuli Khodorkovskiy <[e-mail]>
---
 libsepol/cil/src/cil_resolve_ast.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

James Carter June 14, 2019, 3:35 p.m. UTC | #1

On 6/13/19 4:18 PM, Yuli Khodorkovskiy wrote:
> In the following example, "relabeltoo" is not a valid permission
> in the loaded policy nor in the new module. Before, CIL would not
> complain about the invalid permission and proceed to install the module:
> 
> 	$ cat test.cil
> 
> 	(mlsconstrain (db_procedure (create relabeltoo)) (eq l2 h2))
> 
> With this patch, an error is now prompted to a user:
> 
> 	$ sudo semodule -i foo.cil
> 
> 	Failed to resolve permission relabeltoo
> 	Failed to resolve mlsconstrain statement at /etc/selinux/mls/tmp/modules/400/test/cil:1
> 	semodule:  Failed!
> 
> Signed-off-by: Yuli Khodorkovskiy <[e-mail]>
> ---
>   libsepol/cil/src/cil_resolve_ast.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index ea08087d..22d37f05 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -135,8 +135,11 @@ static int __cil_resolve_perms(symtab_t *class_symtab, symtab_t *common_symtab,
>   				if (class_flavor == CIL_MAP_CLASS) {
>   					cil_log(CIL_ERR, "Failed to resolve permission %s for map class\n", (char*)curr->data);
>   					goto exit;
> +				} else if (class_flavor == CIL_CLASS) {
> +					cil_log(CIL_ERR, "Failed to resolve permission %s\n", (char*)curr->data);
> +					goto exit;
>   				}
> -				cil_log(CIL_WARN, "Failed to resolve permission %s\n", (char*)curr->data);
> +
>   				/* Use an empty list to represent unknown perm */
>   				cil_list_init(&empty_list, perm_strs->flavor);
>   				cil_list_append(*perm_datums, CIL_LIST, empty_list);
> 

You essentially want to revert commits 46e157b47, da51020d6, and 2eefb20d8 from 
back in July to November 2016.

I did not do a good job of specifying the motivation in the initial patch in 
that series, but here is Steve's reply to Steve Lawrence's understandable objection.

 From Steve Smalley's email "Re: [PATCH] libsepol/cil: Warn instead of fail if 
permission is not resolve" on 7/28/16
> The specific motivation for the change is that Fedora is trying to
> expunge permissions from its policy that were never upstream (e.g.
> ptrace_child in class process, compromise_kernel in class capability2,
> etc).  This presently breaks policy updates when there is a third party
> or local policy module already installed that was built against the
> older selinux-policy-devel that had those permissions.  What is worse is
> that rpm thinks that the policy was updated, since only semodule fails
> during %post, so you could easily brick a system this way on an upgrade
> because you'll end up running the old policy but rpm will think it has
> succeeded and proceed to install any other updated packages, which may
> depend on the new policy.


I would be fine with reverting the error handling back to what it was initially. 
It seems unlikely that we are still worried about local policy modules built on 
three year old policy.

Is this still a concern to anyone?
Jim

Patch

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index ea08087d..22d37f05 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -135,8 +135,11 @@  static int __cil_resolve_perms(symtab_t *class_symtab, symtab_t *common_symtab,
 				if (class_flavor == CIL_MAP_CLASS) {
 					cil_log(CIL_ERR, "Failed to resolve permission %s for map class\n", (char*)curr->data);
 					goto exit;
+				} else if (class_flavor == CIL_CLASS) {
+					cil_log(CIL_ERR, "Failed to resolve permission %s\n", (char*)curr->data);
+					goto exit;
 				}
-				cil_log(CIL_WARN, "Failed to resolve permission %s\n", (char*)curr->data);
+
 				/* Use an empty list to represent unknown perm */
 				cil_list_init(&empty_list, perm_strs->flavor);
 				cil_list_append(*perm_datums, CIL_LIST, empty_list);