[0/3] libsepol and checkpolicy: Output CIL or policy.conf from kernel policy
diff mbox

Message ID CAJfZ7==2e9bN7Z94sLDO3boHDgVPx8Gr+=pXWsvNEbdqkjc8fg@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Nicolas Iooss March 11, 2017, 8:02 p.m. UTC
On Fri, Mar 10, 2017 at 8:49 PM, James Carter <jwcart2@tycho.nsa.gov> wrote:
> It would sometimes be helpful for debugging or verification purposes to be able to convert
> a binary policy to a human-readable form.
>
> This patchset adds libsepol functions that take a kernel policydb in and outputs either
> a CIL or policy.conf text.
>
> Checkpolicy is modified to generate CIL text from a binary policy if using the "-C" option
> and to add the "-F" option to generate policy.conf text from a binary policy.
>
> Where possible rules are sorted in alphabetical or numerical order to aid in debugging.
>
> James Carter (3):
>   libsepol: Add ability to convert binary policy to CIL
>   libsepol: Add ability to convert binary policy to policy.conf file
>   checkpolicy: Add options to convert binary policy to CIL or a
>     policy.conf

Hello,
I agree such features are useful when analyzing policies. I have
tested the patches and compiled with some warnings flags (I have not
done a "real" code review). Here are some comments:

- First, the documentation (at least checkpolicy manpage) needs to be
updated with the new options. Also I spent some time trying to
understand what I was doing wrong with options -C and -F until I read
the third patch and understood that "-b" needs to be used as well.

- Then, when using "checkpolicy -bF" on my policy, I got blocks such as:

if ((git_cgi_enable_homedirs && use_samba_home_dirs)) {
            allow httpd_git_script_t cifs_t:dir { getattr search open };
        allow httpd_git_script_t cifs_t:dir { ioctl read getattr lock
search open };
        allow httpd_git_script_t cifs_t:dir { ioctl read getattr lock
search open };
        allow httpd_git_script_t cifs_t:file { ioctl read getattr lock open };
        allow httpd_git_script_t cifs_t:filesystem { getattr };
    } else {        dontaudit httpd_git_script_t cifs_t:file { ioctl
read getattr lock open };
}

The blocks have indentation issues (the first line of the if statement
has too much spaces and a "\n" is missing at the beginning of the else
block. Moreover the permissions are not sorted in alphabetical order
but this may be included in the "Where possible" part of your message.

- Third, the first patch introduces functions with printf-like
arguments and defines them with __attribute__((format(printf...))) in
the .c file. This is fine for the static functions but the other ones
need to have attributes in kernel_to_common.h instead (this enables
the compiler to check the format strings at build time when compiling
other files).

- Finally when compiling with -Wwrite-strings, gcc reports some issues
with literal strings assigned to non-const char* variables. I have
quickly written a short patch to make some variables const char* and
checkpolicy seems to work fine with it. The patch is attached to this
email if you want to consider it.

Cheers,
Nicolas

Comments

James Carter March 13, 2017, 3:32 p.m. UTC | #1
On 03/11/2017 03:02 PM, Nicolas Iooss wrote:
> On Fri, Mar 10, 2017 at 8:49 PM, James Carter <jwcart2@tycho.nsa.gov> wrote:
>> It would sometimes be helpful for debugging or verification purposes to be able to convert
>> a binary policy to a human-readable form.
>>
>> This patchset adds libsepol functions that take a kernel policydb in and outputs either
>> a CIL or policy.conf text.
>>
>> Checkpolicy is modified to generate CIL text from a binary policy if using the "-C" option
>> and to add the "-F" option to generate policy.conf text from a binary policy.
>>
>> Where possible rules are sorted in alphabetical or numerical order to aid in debugging.
>>
>> James Carter (3):
>>   libsepol: Add ability to convert binary policy to CIL
>>   libsepol: Add ability to convert binary policy to policy.conf file
>>   checkpolicy: Add options to convert binary policy to CIL or a
>>     policy.conf
>
> Hello,
> I agree such features are useful when analyzing policies. I have
> tested the patches and compiled with some warnings flags (I have not
> done a "real" code review). Here are some comments:
>
> - First, the documentation (at least checkpolicy manpage) needs to be
> updated with the new options. Also I spent some time trying to
> understand what I was doing wrong with options -C and -F until I read
> the third patch and understood that "-b" needs to be used as well.
>
> - Then, when using "checkpolicy -bF" on my policy, I got blocks such as:
>
> if ((git_cgi_enable_homedirs && use_samba_home_dirs)) {
>             allow httpd_git_script_t cifs_t:dir { getattr search open };
>         allow httpd_git_script_t cifs_t:dir { ioctl read getattr lock
> search open };
>         allow httpd_git_script_t cifs_t:dir { ioctl read getattr lock
> search open };
>         allow httpd_git_script_t cifs_t:file { ioctl read getattr lock open };
>         allow httpd_git_script_t cifs_t:filesystem { getattr };
>     } else {        dontaudit httpd_git_script_t cifs_t:file { ioctl
> read getattr lock open };
> }
>
> The blocks have indentation issues (the first line of the if statement
> has too much spaces and a "\n" is missing at the beginning of the else
> block. Moreover the permissions are not sorted in alphabetical order
> but this may be included in the "Where possible" part of your message.
>
> - Third, the first patch introduces functions with printf-like
> arguments and defines them with __attribute__((format(printf...))) in
> the .c file. This is fine for the static functions but the other ones
> need to have attributes in kernel_to_common.h instead (this enables
> the compiler to check the format strings at build time when compiling
> other files).
>
> - Finally when compiling with -Wwrite-strings, gcc reports some issues
> with literal strings assigned to non-const char* variables. I have
> quickly written a short patch to make some variables const char* and
> checkpolicy seems to work fine with it. The patch is attached to this
> email if you want to consider it.
>

Thanks for testing it out. I agree with all of your comments and suggestions 
above. Thanks for the patch as well.

Jim

> Cheers,
> Nicolas
>

Patch
diff mbox

From c25a08efc3f6596c0f3bfdcb2e36abca68b5b3e8 Mon Sep 17 00:00:00 2001
From: Nicolas Iooss <nicolas.iooss@m4x.org>
Date: Sat, 11 Mar 2017 10:26:45 +0100
Subject: [PATCH 1/1] fix -Wwrite-string warnings

---
 libsepol/src/kernel_to_cil.c  | 11 ++++++-----
 libsepol/src/kernel_to_conf.c | 12 +++++++-----
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
index 143f7faa4a07..0854426fae33 100644
--- a/libsepol/src/kernel_to_cil.c
+++ b/libsepol/src/kernel_to_cil.c
@@ -266,7 +266,7 @@  static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
 	char *expr = NULL;
 	int is_mls;
 	char *perms;
-	char *format_str;
+	const char *format_str;
 	struct strs *strs;
 
 	for (curr = constraint_rules; curr != NULL; curr = curr->next) {
@@ -307,7 +307,7 @@  static int class_validatetrans_rules_to_strs(struct policydb *pdb, char *classke
 	struct constraint_node *curr;
 	char *expr = NULL;
 	int is_mls;
-	char *format_str;
+	const char *format_str;
 	struct strs *strs;
 	int rc = 0;
 
@@ -994,7 +994,8 @@  static char *cats_ebitmap_to_str(struct ebitmap *cats, char **val_to_name)
 {
 	struct ebitmap_node *node;
 	uint32_t i, start, range;
-	char *catsbuf, *p, *fmt;
+	char *catsbuf, *p;
+	const char *fmt;
 	int len, remaining;
 
 	remaining = (int)cats_ebitmap_len(cats, val_to_name);
@@ -1605,10 +1606,10 @@  static char *xperms_to_str(avtab_extended_perms_t *xperms)
 
 static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_datum_t *datum)
 {
-	const char *flavor;
+	const char *flavor, *tgt;
 	uint32_t data = datum->data;
 	type_datum_t *type;
-	char *src, *tgt, *class, *perms, *new;
+	char *src, *class, *perms, *new;
 	char *rule = NULL;
 
 	switch (0xFFF & key->specified) {
diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
index 5e5a8647b426..898fd1baf38f 100644
--- a/libsepol/src/kernel_to_conf.c
+++ b/libsepol/src/kernel_to_conf.c
@@ -260,7 +260,8 @@  static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
 	int rc = 0;
 	struct constraint_node *curr;
 	int is_mls;
-	char *format_str, *flavor, *perms, *expr;
+	const char *format_str, *flavor;
+	char *perms, *expr;
 	struct strs *strs;
 
 	for (curr = constraint_rules; curr != NULL; curr = curr->next) {
@@ -305,7 +306,8 @@  static int class_validatetrans_rules_to_strs(struct policydb *pdb, char *classke
 {
 	struct constraint_node *curr;
 	int is_mls;
-	char *flavor, *expr;
+	const char *flavor;
+	char *expr;
 	struct strs *strs;
 	int rc = 0;
 
@@ -969,7 +971,8 @@  static char *cats_ebitmap_to_str(struct ebitmap *cats, char **val_to_name)
 {
 	struct ebitmap_node *node;
 	uint32_t i, start, range, first;
-	char *catsbuf, *p, *fmt;
+	char *catsbuf, *p;
+	const char *fmt;
 	char sep;
 	int len, remaining;
 
@@ -1573,10 +1576,9 @@  exit:
 
 static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_datum_t *datum)
 {
-	const char *flavor;
+	const char *flavor, *src, *tgt, *class, *perms, *new;
 	uint32_t data = datum->data;
 	type_datum_t *type;
-	char *src, *tgt, *class, *perms, *new;
 	char *rule = NULL;
 
 	switch (0xFFF & key->specified) {
-- 
2.11.1