[PATCH 0/3] Add warnings if module name different from filename

[PATCH 0/3] Add warnings if module name different from filename

[James Carter]

Since CIL treats files as modules and does not have a separate
module statement it can cause confusion when a Refpolicy module
has a name that is not the same as its base filename because
older SELinux userspaces will refer to the module by its module
name, but CIL will refer to the module by its filename.

Warn when the module name is different from the filename when converting
a policy package to CIL or when compiling a module.

James Carter (3):
  libsepol: Add function to check if module name matches filename
  policycoreutils/hll/pp: Warn if module name different from filenames
  checkpolicy: Warn if module name different than filenames

 checkpolicy/checkmodule.c       | 13 +++++++++++++
 libsepol/include/sepol/module.h |  3 +++
 libsepol/src/libsepol.map.in    |  2 ++
 libsepol/src/module.c           | 36 ++++++++++++++++++++++++++++++++++++
 policycoreutils/hll/pp/pp.c     | 29 +++++++++++++++++++++++++----
 5 files changed, 79 insertions(+), 4 deletions(-)

-- 
2.5.5

[PATCH 1/3] libsepol: Add function to check if module name matches filename

[James Carter]

The function sepol_module_check_name_matches_filename() compares
the module name with a filename (after stripping off path and file
extension) and returns 0 if they match. The function
sepol_module_get_name() returns the name of the module.

Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
---
 libsepol/include/sepol/module.h |  3 +++
 libsepol/src/libsepol.map.in    |  2 ++
 libsepol/src/module.c           | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/libsepol/include/sepol/module.h b/libsepol/include/sepol/module.h
index ff27f96..4e31d6e 100644
--- a/libsepol/include/sepol/module.h
+++ b/libsepol/include/sepol/module.h
@@ -82,5 +82,8 @@ extern int sepol_expand_module(sepol_handle_t * handle,
 			       sepol_policydb_t * base,
 			       sepol_policydb_t * out, int verbose, int check);
 
+char *sepol_module_get_name(sepol_policydb_t *module);
+int sepol_module_check_name_matches_filename(sepol_policydb_t *module, const char *path);
+
 __END_DECLS
 #endif
diff --git a/libsepol/src/libsepol.map.in b/libsepol/src/libsepol.map.in
index 0a46b09..db2241b 100644
--- a/libsepol/src/libsepol.map.in
+++ b/libsepol/src/libsepol.map.in
@@ -48,5 +48,7 @@ LIBSEPOL_1.1 {
 	sepol_ppfile_to_module_package;
 	sepol_module_package_to_cil;
 	sepol_module_policydb_to_cil;
+	sepol_module_get_name;
+	sepol_module_check_name_matches_filename;
   local: *;
 } LIBSEPOL_1.0;
diff --git a/libsepol/src/module.c b/libsepol/src/module.c
index 1665ede..ce514d5 100644
--- a/libsepol/src/module.c
+++ b/libsepol/src/module.c
@@ -30,6 +30,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <limits.h>
+#include <libgen.h>
 
 #define SEPOL_PACKAGE_SECTION_FC 0xf97cff90
 #define SEPOL_PACKAGE_SECTION_SEUSER 0x97cff91
@@ -1006,3 +1007,38 @@ int sepol_expand_module(sepol_handle_t * handle,
 {
 	return expand_module(handle, &base->p, &out->p, verbose, check);
 }
+
+char *sepol_module_get_name(sepol_policydb_t *module)
+{
+	return module->p.name;
+}
+
+int sepol_module_check_name_matches_filename(sepol_policydb_t *module, const char *path)
+{
+	char *filepath, *filename, *separator;
+	int rc = -1;
+
+	if (module->p.policy_type == POLICY_BASE)
+		return 0;
+
+	filepath = strdup(path);
+	filename = basename(filepath);
+
+	if (strcmp(module->p.name, filename) != 0) {
+		separator = strrchr(filename, '.');
+		if (separator == NULL)
+			goto exit;
+
+		*separator = '\0';
+
+		if (strcmp(module->p.name, filename) != 0)
+			goto exit;
+	}
+
+	rc = 0;
+
+exit:
+	free(filepath);
+
+	return rc;
+}
-- 
2.5.5

[PATCH 2/3] policycoreutils/hll/pp: Warn if module name different from filenames

[James Carter]

Since CIL treats files as modules and does not have a separate
module statement it can cause confusion when a Refpolicy module
has a name that is not the same as its base filename because
older SELinux userspaces will refer to the module by its module
name, but CIL will refer to the module by its filename.

When converting a policy package to CIL warn if the module name is
different from the pp filename or the CIL filename.

Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
---
 policycoreutils/hll/pp/pp.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/policycoreutils/hll/pp/pp.c b/policycoreutils/hll/pp/pp.c
index 866734f..22cef0d 100644
--- a/policycoreutils/hll/pp/pp.c
+++ b/policycoreutils/hll/pp/pp.c
@@ -28,6 +28,7 @@
 
 #include <sepol/module.h>
 #include <sepol/module_to_cil.h>
+#include <sepol/policydb/module.h>
 
 char *progname;
 
@@ -68,6 +69,8 @@ int main(int argc, char **argv)
 		{ NULL, 0, NULL, 0 }
 	};
 	struct sepol_module_package *mod_pkg = NULL;
+	char *ifile = NULL;
+	char *ofile = NULL;
 	FILE *in = NULL;
 	FILE *out = NULL;
 	int outfd = -1;
@@ -89,9 +92,10 @@ int main(int argc, char **argv)
 	}
 
 	if (argc >= optind + 1 && strcmp(argv[1], "-") != 0) {
-		in = fopen(argv[1], "rb");
+		ifile = argv[1];
+		in = fopen(ifile, "rb");
 		if (in == NULL) {
-			log_err("Failed to open %s: %s", argv[1], strerror(errno));
+			log_err("Failed to open %s: %s", ifile, strerror(errno));
 			rc = -1;
 			goto exit;
 		}
@@ -100,9 +104,10 @@ int main(int argc, char **argv)
 	}
 
 	if (argc >= optind + 2 && strcmp(argv[2], "-") != 0) {
-		out = fopen(argv[2], "w");
+		ofile = argv[2];
+		out = fopen(ofile, "w");
 		if (out == NULL) {
-			log_err("Failed to open %s: %s", argv[2], strerror(errno));
+			log_err("Failed to open %s: %s", ofile, strerror(errno));
 			rc = -1;
 			goto exit;
 		}
@@ -122,6 +127,22 @@ int main(int argc, char **argv)
 	fclose(in);
 	in = NULL;
 
+	if (ifile) {
+		rc = sepol_module_check_name_matches_filename(mod_pkg->policy, ifile);
+		if (rc != 0) {
+			fprintf(stderr,	"Module name %s does not match pp file %s\n",
+				sepol_module_get_name(mod_pkg->policy), ifile);
+		}
+	}
+
+	if (ofile) {
+		rc = sepol_module_check_name_matches_filename(mod_pkg->policy, ofile);
+		if (rc != 0) {
+			fprintf(stderr,	"Module name %s does not match cil file %s\n",
+				sepol_module_get_name(mod_pkg->policy), ofile);
+		}
+	}
+
 	rc = sepol_module_package_to_cil(out, mod_pkg);
 	if (rc != 0) {
 		goto exit;
-- 
2.5.5

[PATCH 3/3] checkpolicy: Warn if module name different than filenames

[James Carter]

Since the usual convention is for the module name to be same as the
base filename of the module, provide a warning message if they are
different. Also warn if the output filename is different than the
module name.

Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
---
 checkpolicy/checkmodule.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/checkpolicy/checkmodule.c b/checkpolicy/checkmodule.c
index 5957d29..5d1e219 100644
--- a/checkpolicy/checkmodule.c
+++ b/checkpolicy/checkmodule.c
@@ -258,6 +258,19 @@ int main(int argc, char **argv)
 		}
 	}
 
+	if (policy_type != POLICY_BASE) {
+		sepol_policydb_t *module = (sepol_policydb_t *)&modpolicydb;
+		if (sepol_module_check_name_matches_filename(module, file)) {
+			fprintf(stderr,	"Module name %s does not match input file %s\n",
+				sepol_module_get_name(module), file);
+		}
+		if (outfile) {
+			if (sepol_module_check_name_matches_filename(module, outfile)) {
+				fprintf(stderr,	"Module name %s does not match output file %s\n", sepol_module_get_name(module), outfile);
+			}
+		}
+	}
+
 	if (modpolicydb.policy_type == POLICY_BASE && !cil) {
 		/* Verify that we can successfully expand the base module. */
 		policydb_t kernpolicydb;
-- 
2.5.5

Re: [PATCH 2/3] policycoreutils/hll/pp: Warn if module name different from filenames

[Stephen Smalley]

On 03/25/2016 02:04 PM, James Carter wrote:
> Since CIL treats files as modules and does not have a separate
> module statement it can cause confusion when a Refpolicy module
> has a name that is not the same as its base filename because
> older SELinux userspaces will refer to the module by its module
> name, but CIL will refer to the module by its filename.
> 
> When converting a policy package to CIL warn if the module name is
> different from the pp filename or the CIL filename.
> 
> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
> ---
>  policycoreutils/hll/pp/pp.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/policycoreutils/hll/pp/pp.c b/policycoreutils/hll/pp/pp.c
> index 866734f..22cef0d 100644
> --- a/policycoreutils/hll/pp/pp.c
> +++ b/policycoreutils/hll/pp/pp.c
> @@ -28,6 +28,7 @@
>  
>  #include <sepol/module.h>
>  #include <sepol/module_to_cil.h>
> +#include <sepol/policydb/module.h>
>  
>  char *progname;
>  
> @@ -68,6 +69,8 @@ int main(int argc, char **argv)
>  		{ NULL, 0, NULL, 0 }
>  	};
>  	struct sepol_module_package *mod_pkg = NULL;
> +	char *ifile = NULL;
> +	char *ofile = NULL;
>  	FILE *in = NULL;
>  	FILE *out = NULL;
>  	int outfd = -1;
> @@ -89,9 +92,10 @@ int main(int argc, char **argv)
>  	}
>  
>  	if (argc >= optind + 1 && strcmp(argv[1], "-") != 0) {
> -		in = fopen(argv[1], "rb");
> +		ifile = argv[1];
> +		in = fopen(ifile, "rb");
>  		if (in == NULL) {
> -			log_err("Failed to open %s: %s", argv[1], strerror(errno));
> +			log_err("Failed to open %s: %s", ifile, strerror(errno));
>  			rc = -1;
>  			goto exit;
>  		}
> @@ -100,9 +104,10 @@ int main(int argc, char **argv)
>  	}
>  
>  	if (argc >= optind + 2 && strcmp(argv[2], "-") != 0) {
> -		out = fopen(argv[2], "w");
> +		ofile = argv[2];
> +		out = fopen(ofile, "w");
>  		if (out == NULL) {
> -			log_err("Failed to open %s: %s", argv[2], strerror(errno));
> +			log_err("Failed to open %s: %s", ofile, strerror(errno));
>  			rc = -1;
>  			goto exit;
>  		}
> @@ -122,6 +127,22 @@ int main(int argc, char **argv)
>  	fclose(in);
>  	in = NULL;
>  
> +	if (ifile) {
> +		rc = sepol_module_check_name_matches_filename(mod_pkg->policy, ifile);
> +		if (rc != 0) {
> +			fprintf(stderr,	"Module name %s does not match pp file %s\n",
> +				sepol_module_get_name(mod_pkg->policy), ifile);
> +		}
> +	}
> +
> +	if (ofile) {
> +		rc = sepol_module_check_name_matches_filename(mod_pkg->policy, ofile);
> +		if (rc != 0) {
> +			fprintf(stderr,	"Module name %s does not match cil file %s\n",
> +				sepol_module_get_name(mod_pkg->policy), ofile);
> +		}
> +	}

So what, if anything, should the user take away from such warnings?  We
likely ought to prefix them with "Warning:" or similar to indicate that
it is non-fatal.  And perhaps tell them which name will need to be used
for subsequent commands.

> +
>  	rc = sepol_module_package_to_cil(out, mod_pkg);
>  	if (rc != 0) {
>  		goto exit;
> 

Re: [PATCH 3/3] checkpolicy: Warn if module name different than filenames

[Stephen Smalley]

On 03/25/2016 02:04 PM, James Carter wrote:
> Since the usual convention is for the module name to be same as the
> base filename of the module, provide a warning message if they are
> different. Also warn if the output filename is different than the
> module name.
> 
> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
> ---
>  checkpolicy/checkmodule.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/checkpolicy/checkmodule.c b/checkpolicy/checkmodule.c
> index 5957d29..5d1e219 100644
> --- a/checkpolicy/checkmodule.c
> +++ b/checkpolicy/checkmodule.c
> @@ -258,6 +258,19 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> +	if (policy_type != POLICY_BASE) {
> +		sepol_policydb_t *module = (sepol_policydb_t *)&modpolicydb;
> +		if (sepol_module_check_name_matches_filename(module, file)) {
> +			fprintf(stderr,	"Module name %s does not match input file %s\n",
> +				sepol_module_get_name(module), file);
> +		}
> +		if (outfile) {
> +			if (sepol_module_check_name_matches_filename(module, outfile)) {
> +				fprintf(stderr,	"Module name %s does not match output file %s\n", sepol_module_get_name(module), outfile);
> +			}
> +		}
> +	}

Probably want a "Warning:" prefix here as well, and possibly some hint
as to which name is preferred or will be used by the system.

Do we actually care about the input file name?

> +
>  	if (modpolicydb.policy_type == POLICY_BASE && !cil) {
>  		/* Verify that we can successfully expand the base module. */
>  		policydb_t kernpolicydb;
> 

Re: [PATCH 2/3] policycoreutils/hll/pp: Warn if module name different from filenames

[James Carter]

On 03/25/2016 02:45 PM, Stephen Smalley wrote:
> On 03/25/2016 02:04 PM, James Carter wrote:
>> Since CIL treats files as modules and does not have a separate
>> module statement it can cause confusion when a Refpolicy module
>> has a name that is not the same as its base filename because
>> older SELinux userspaces will refer to the module by its module
>> name, but CIL will refer to the module by its filename.
>>
>> When converting a policy package to CIL warn if the module name is
>> different from the pp filename or the CIL filename.
>>
>> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
>> ---
>>   policycoreutils/hll/pp/pp.c | 29 +++++++++++++++++++++++++----
>>   1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/policycoreutils/hll/pp/pp.c b/policycoreutils/hll/pp/pp.c
>> index 866734f..22cef0d 100644
>> --- a/policycoreutils/hll/pp/pp.c
>> +++ b/policycoreutils/hll/pp/pp.c
>> @@ -28,6 +28,7 @@
>>
>>   #include <sepol/module.h>
>>   #include <sepol/module_to_cil.h>
>> +#include <sepol/policydb/module.h>
>>
>>   char *progname;
>>
>> @@ -68,6 +69,8 @@ int main(int argc, char **argv)
>>   		{ NULL, 0, NULL, 0 }
>>   	};
>>   	struct sepol_module_package *mod_pkg = NULL;
>> +	char *ifile = NULL;
>> +	char *ofile = NULL;
>>   	FILE *in = NULL;
>>   	FILE *out = NULL;
>>   	int outfd = -1;
>> @@ -89,9 +92,10 @@ int main(int argc, char **argv)
>>   	}
>>
>>   	if (argc >= optind + 1 && strcmp(argv[1], "-") != 0) {
>> -		in = fopen(argv[1], "rb");
>> +		ifile = argv[1];
>> +		in = fopen(ifile, "rb");
>>   		if (in == NULL) {
>> -			log_err("Failed to open %s: %s", argv[1], strerror(errno));
>> +			log_err("Failed to open %s: %s", ifile, strerror(errno));
>>   			rc = -1;
>>   			goto exit;
>>   		}
>> @@ -100,9 +104,10 @@ int main(int argc, char **argv)
>>   	}
>>
>>   	if (argc >= optind + 2 && strcmp(argv[2], "-") != 0) {
>> -		out = fopen(argv[2], "w");
>> +		ofile = argv[2];
>> +		out = fopen(ofile, "w");
>>   		if (out == NULL) {
>> -			log_err("Failed to open %s: %s", argv[2], strerror(errno));
>> +			log_err("Failed to open %s: %s", ofile, strerror(errno));
>>   			rc = -1;
>>   			goto exit;
>>   		}
>> @@ -122,6 +127,22 @@ int main(int argc, char **argv)
>>   	fclose(in);
>>   	in = NULL;
>>
>> +	if (ifile) {
>> +		rc = sepol_module_check_name_matches_filename(mod_pkg->policy, ifile);
>> +		if (rc != 0) {
>> +			fprintf(stderr,	"Module name %s does not match pp file %s\n",
>> +				sepol_module_get_name(mod_pkg->policy), ifile);
>> +		}
>> +	}
>> +
>> +	if (ofile) {
>> +		rc = sepol_module_check_name_matches_filename(mod_pkg->policy, ofile);
>> +		if (rc != 0) {
>> +			fprintf(stderr,	"Module name %s does not match cil file %s\n",
>> +				sepol_module_get_name(mod_pkg->policy), ofile);
>> +		}
>> +	}
>
> So what, if anything, should the user take away from such warnings?  We
> likely ought to prefix them with "Warning:" or similar to indicate that
> it is non-fatal.  And perhaps tell them which name will need to be used
> for subsequent commands.
>

What they need to take away is that the module is going to be referred to by its 
filename from now on, so telling them that would be helpful. I originally had 
prefixed the message with "Warning:", but I thought that might be too strong and 
scary.

Jim

>> +
>>   	rc = sepol_module_package_to_cil(out, mod_pkg);
>>   	if (rc != 0) {
>>   		goto exit;
>>


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency

Re: [PATCH 3/3] checkpolicy: Warn if module name different than filenames

[James Carter]

On 03/25/2016 02:48 PM, Stephen Smalley wrote:
> On 03/25/2016 02:04 PM, James Carter wrote:
>> Since the usual convention is for the module name to be same as the
>> base filename of the module, provide a warning message if they are
>> different. Also warn if the output filename is different than the
>> module name.
>>
>> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
>> ---
>>   checkpolicy/checkmodule.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/checkpolicy/checkmodule.c b/checkpolicy/checkmodule.c
>> index 5957d29..5d1e219 100644
>> --- a/checkpolicy/checkmodule.c
>> +++ b/checkpolicy/checkmodule.c
>> @@ -258,6 +258,19 @@ int main(int argc, char **argv)
>>   		}
>>   	}
>>
>> +	if (policy_type != POLICY_BASE) {
>> +		sepol_policydb_t *module = (sepol_policydb_t *)&modpolicydb;
>> +		if (sepol_module_check_name_matches_filename(module, file)) {
>> +			fprintf(stderr,	"Module name %s does not match input file %s\n",
>> +				sepol_module_get_name(module), file);
>> +		}
>> +		if (outfile) {
>> +			if (sepol_module_check_name_matches_filename(module, outfile)) {
>> +				fprintf(stderr,	"Module name %s does not match output file %s\n", sepol_module_get_name(module), outfile);
>> +			}
>> +		}
>> +	}
>
> Probably want a "Warning:" prefix here as well, and possibly some hint
> as to which name is preferred or will be used by the system.
>
> Do we actually care about the input file name?
>

I went back and forth with that myself. I don't think so, but I left in in case 
others cared.

Jim

>> +
>>   	if (modpolicydb.policy_type == POLICY_BASE && !cil) {
>>   		/* Verify that we can successfully expand the base module. */
>>   		policydb_t kernpolicydb;
>>


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency

Re: [PATCH 2/3] policycoreutils/hll/pp: Warn if module name different from filenames

[Daniel J Walsh]

On 03/25/2016 02:57 PM, James Carter wrote:
> On 03/25/2016 02:45 PM, Stephen Smalley wrote:
>> On 03/25/2016 02:04 PM, James Carter wrote:
>>> Since CIL treats files as modules and does not have a separate
>>> module statement it can cause confusion when a Refpolicy module
>>> has a name that is not the same as its base filename because
>>> older SELinux userspaces will refer to the module by its module
>>> name, but CIL will refer to the module by its filename.
>>>
>>> When converting a policy package to CIL warn if the module name is
>>> different from the pp filename or the CIL filename.
>>>
>>> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
>>> ---
>>>   policycoreutils/hll/pp/pp.c | 29 +++++++++++++++++++++++++----
>>>   1 file changed, 25 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/policycoreutils/hll/pp/pp.c b/policycoreutils/hll/pp/pp.c
>>> index 866734f..22cef0d 100644
>>> --- a/policycoreutils/hll/pp/pp.c
>>> +++ b/policycoreutils/hll/pp/pp.c
>>> @@ -28,6 +28,7 @@
>>>
>>>   #include <sepol/module.h>
>>>   #include <sepol/module_to_cil.h>
>>> +#include <sepol/policydb/module.h>
>>>
>>>   char *progname;
>>>
>>> @@ -68,6 +69,8 @@ int main(int argc, char **argv)
>>>           { NULL, 0, NULL, 0 }
>>>       };
>>>       struct sepol_module_package *mod_pkg = NULL;
>>> +    char *ifile = NULL;
>>> +    char *ofile = NULL;
>>>       FILE *in = NULL;
>>>       FILE *out = NULL;
>>>       int outfd = -1;
>>> @@ -89,9 +92,10 @@ int main(int argc, char **argv)
>>>       }
>>>
>>>       if (argc >= optind + 1 && strcmp(argv[1], "-") != 0) {
>>> -        in = fopen(argv[1], "rb");
>>> +        ifile = argv[1];
>>> +        in = fopen(ifile, "rb");
>>>           if (in == NULL) {
>>> -            log_err("Failed to open %s: %s", argv[1], 
>>> strerror(errno));
>>> +            log_err("Failed to open %s: %s", ifile, strerror(errno));
>>>               rc = -1;
>>>               goto exit;
>>>           }
>>> @@ -100,9 +104,10 @@ int main(int argc, char **argv)
>>>       }
>>>
>>>       if (argc >= optind + 2 && strcmp(argv[2], "-") != 0) {
>>> -        out = fopen(argv[2], "w");
>>> +        ofile = argv[2];
>>> +        out = fopen(ofile, "w");
>>>           if (out == NULL) {
>>> -            log_err("Failed to open %s: %s", argv[2], 
>>> strerror(errno));
>>> +            log_err("Failed to open %s: %s", ofile, strerror(errno));
>>>               rc = -1;
>>>               goto exit;
>>>           }
>>> @@ -122,6 +127,22 @@ int main(int argc, char **argv)
>>>       fclose(in);
>>>       in = NULL;
>>>
>>> +    if (ifile) {
>>> +        rc = 
>>> sepol_module_check_name_matches_filename(mod_pkg->policy, ifile);
>>> +        if (rc != 0) {
>>> +            fprintf(stderr,    "Module name %s does not match pp 
>>> file %s\n",
>>> +                sepol_module_get_name(mod_pkg->policy), ifile);
>>> +        }
>>> +    }
>>> +
>>> +    if (ofile) {
>>> +        rc = 
>>> sepol_module_check_name_matches_filename(mod_pkg->policy, ofile);
>>> +        if (rc != 0) {
>>> +            fprintf(stderr,    "Module name %s does not match cil 
>>> file %s\n",
>>> +                sepol_module_get_name(mod_pkg->policy), ofile);
>>> +        }
>>> +    }
>>
>> So what, if anything, should the user take away from such warnings?  We
>> likely ought to prefix them with "Warning:" or similar to indicate that
>> it is non-fatal.  And perhaps tell them which name will need to be used
>> for subsequent commands.
>>
>
> What they need to take away is that the module is going to be referred 
> to by its filename from now on, so telling them that would be helpful. 
> I originally had prefixed the message with "Warning:", but I thought 
> that might be too strong and scary.
>
> Jim
>
Why not make it fatal. Is there any reason a user would ever want to do 
this, I would think this is almost always a mistake.
>>> +
>>>       rc = sepol_module_package_to_cil(out, mod_pkg);
>>>       if (rc != 0) {
>>>           goto exit;
>>>
>
>

Re: [PATCH 2/3] policycoreutils/hll/pp: Warn if module name different from filenames

[James Carter]

On 03/26/2016 04:07 PM, Daniel J Walsh wrote:
>
>
> On 03/25/2016 02:57 PM, James Carter wrote:
>> On 03/25/2016 02:45 PM, Stephen Smalley wrote:
>>> On 03/25/2016 02:04 PM, James Carter wrote:
>>>> Since CIL treats files as modules and does not have a separate
>>>> module statement it can cause confusion when a Refpolicy module
>>>> has a name that is not the same as its base filename because
>>>> older SELinux userspaces will refer to the module by its module
>>>> name, but CIL will refer to the module by its filename.
>>>>
>>>> When converting a policy package to CIL warn if the module name is
>>>> different from the pp filename or the CIL filename.
>>>>
>>>> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov>
>>>> ---
>>>>   policycoreutils/hll/pp/pp.c | 29 +++++++++++++++++++++++++----
>>>>   1 file changed, 25 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/policycoreutils/hll/pp/pp.c b/policycoreutils/hll/pp/pp.c
>>>> index 866734f..22cef0d 100644
>>>> --- a/policycoreutils/hll/pp/pp.c
>>>> +++ b/policycoreutils/hll/pp/pp.c
>>>> @@ -28,6 +28,7 @@
>>>>
>>>>   #include <sepol/module.h>
>>>>   #include <sepol/module_to_cil.h>
>>>> +#include <sepol/policydb/module.h>
>>>>
>>>>   char *progname;
>>>>
>>>> @@ -68,6 +69,8 @@ int main(int argc, char **argv)
>>>>           { NULL, 0, NULL, 0 }
>>>>       };
>>>>       struct sepol_module_package *mod_pkg = NULL;
>>>> +    char *ifile = NULL;
>>>> +    char *ofile = NULL;
>>>>       FILE *in = NULL;
>>>>       FILE *out = NULL;
>>>>       int outfd = -1;
>>>> @@ -89,9 +92,10 @@ int main(int argc, char **argv)
>>>>       }
>>>>
>>>>       if (argc >= optind + 1 && strcmp(argv[1], "-") != 0) {
>>>> -        in = fopen(argv[1], "rb");
>>>> +        ifile = argv[1];
>>>> +        in = fopen(ifile, "rb");
>>>>           if (in == NULL) {
>>>> -            log_err("Failed to open %s: %s", argv[1], strerror(errno));
>>>> +            log_err("Failed to open %s: %s", ifile, strerror(errno));
>>>>               rc = -1;
>>>>               goto exit;
>>>>           }
>>>> @@ -100,9 +104,10 @@ int main(int argc, char **argv)
>>>>       }
>>>>
>>>>       if (argc >= optind + 2 && strcmp(argv[2], "-") != 0) {
>>>> -        out = fopen(argv[2], "w");
>>>> +        ofile = argv[2];
>>>> +        out = fopen(ofile, "w");
>>>>           if (out == NULL) {
>>>> -            log_err("Failed to open %s: %s", argv[2], strerror(errno));
>>>> +            log_err("Failed to open %s: %s", ofile, strerror(errno));
>>>>               rc = -1;
>>>>               goto exit;
>>>>           }
>>>> @@ -122,6 +127,22 @@ int main(int argc, char **argv)
>>>>       fclose(in);
>>>>       in = NULL;
>>>>
>>>> +    if (ifile) {
>>>> +        rc = sepol_module_check_name_matches_filename(mod_pkg->policy, ifile);
>>>> +        if (rc != 0) {
>>>> +            fprintf(stderr,    "Module name %s does not match pp file %s\n",
>>>> +                sepol_module_get_name(mod_pkg->policy), ifile);
>>>> +        }
>>>> +    }
>>>> +
>>>> +    if (ofile) {
>>>> +        rc = sepol_module_check_name_matches_filename(mod_pkg->policy, ofile);
>>>> +        if (rc != 0) {
>>>> +            fprintf(stderr,    "Module name %s does not match cil file %s\n",
>>>> +                sepol_module_get_name(mod_pkg->policy), ofile);
>>>> +        }
>>>> +    }
>>>
>>> So what, if anything, should the user take away from such warnings?  We
>>> likely ought to prefix them with "Warning:" or similar to indicate that
>>> it is non-fatal.  And perhaps tell them which name will need to be used
>>> for subsequent commands.
>>>
>>
>> What they need to take away is that the module is going to be referred to by
>> its filename from now on, so telling them that would be helpful. I originally
>> had prefixed the message with "Warning:", but I thought that might be too
>> strong and scary.
>>
>> Jim
>>
> Why not make it fatal. Is there any reason a user would ever want to do this, I
> would think this is almost always a mistake.

I think that it is almost always a mistake, but there were, at least in the 
past, modules that did this and I didn't want to cause problems if they are 
still in use.

Jim

>>>> +
>>>>       rc = sepol_module_package_to_cil(out, mod_pkg);
>>>>       if (rc != 0) {
>>>>           goto exit;
>>>>
>>
>>
>


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency