* [PATCH] libsemanage: use pp module headers as a source for a module name
@ 2016-09-23 9:23 Petr Lautrbach
2016-09-23 15:31 ` James Carter
0 siblings, 1 reply; 7+ messages in thread
From: Petr Lautrbach @ 2016-09-23 9:23 UTC (permalink / raw)
To: selinux
When a user installs a module, the filename is used as the module name.
This change was introduced with CIL language where a module name is not
stored in the module itself. It means that when a pp module has
different filename and stored module name, the filename is used instead
of the stored module name. It brings problems with compatibility for
scripts and modules which were built and used on older system and were
migrated to the new userspace.
This patch changes the behavior of semanage_direct_install_file() which
is used by 'semodule -i' so that when a module with pp language
extension is installed, it tries to get and use a stored module name
instead of a filename.
Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
libsemanage/src/direct_api.c | 62 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 61 insertions(+), 1 deletion(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 2187b65..f98d3d1 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -363,6 +363,48 @@ static int semanage_direct_begintrans(semanage_handle_t * sh)
/********************* utility functions *********************/
+/* Takes a module stored in 'module_data' and parses its headers.
+ * Sets reference variables 'module_name' to module's name, and
+ * 'version' to module's version. The caller is responsible for
+ * free()ing 'module_name', and 'version'; they will be
+ * set to NULL upon entering this function. Returns 0 on success, -1
+ * if out of memory, or -2 if data did not represent a module.
+ */
+static int parse_module_headers(semanage_handle_t * sh, char *module_data,
+ size_t data_len, char **module_name,
+ char **version)
+{
+ struct sepol_policy_file *pf;
+ int file_type;
+ *module_name = *version = NULL;
+
+ if (sepol_policy_file_create(&pf)) {
+ ERR(sh, "Out of memory!");
+ return -1;
+ }
+ sepol_policy_file_set_mem(pf, module_data, data_len);
+ sepol_policy_file_set_handle(pf, sh->sepolh);
+ if (module_data == NULL ||
+ data_len == 0 ||
+ sepol_module_package_info(pf, &file_type, module_name,
+ version) == -1) {
+ sepol_policy_file_free(pf);
+ ERR(sh, "Could not parse module data.");
+ return -2;
+ }
+ sepol_policy_file_free(pf);
+ if (file_type != SEPOL_POLICY_MOD) {
+ if (file_type == SEPOL_POLICY_BASE)
+ ERR(sh,
+ "Received a base module, expected a non-base module.");
+ else
+ ERR(sh, "Data did not represent a module.");
+ return -2;
+ }
+
+ return 0;
+}
+
#include <stdlib.h>
#include <bzlib.h>
#include <string.h>
@@ -1524,7 +1566,9 @@ static int semanage_direct_install_file(semanage_handle_t * sh,
char *path = NULL;
char *filename;
char *lang_ext = NULL;
+ char *module_name = NULL;
char *separator;
+ char *version = NULL;
if ((data_len = map_file(sh, install_filename, &data, &compressed)) <= 0) {
ERR(sh, "Unable to read file %s\n", install_filename);
@@ -1564,10 +1608,26 @@ static int semanage_direct_install_file(semanage_handle_t * sh,
lang_ext = separator + 1;
}
- retval = semanage_direct_install(sh, data, data_len, filename, lang_ext);
+ if (strcmp(lang_ext, "pp") != 0)
+ module_name = strdup(filename);
+ else {
+ if (parse_module_headers(sh, data, data_len, &module_name, &version) != 0) {
+ free(module_name);
+ module_name = strdup(filename);
+ }
+ free(version);
+ }
+ if (module_name == NULL) {
+ ERR(sh, "No memory available for module_name.\n");
+ retval = -1;
+ goto cleanup;
+ }
+
+ retval = semanage_direct_install(sh, data, data_len, module_name, lang_ext);
cleanup:
if (data_len > 0) munmap(data, data_len);
+ free(module_name);
free(path);
return retval;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] libsemanage: use pp module headers as a source for a module name
2016-09-23 9:23 [PATCH] libsemanage: use pp module headers as a source for a module name Petr Lautrbach
@ 2016-09-23 15:31 ` James Carter
2016-09-23 16:05 ` Petr Lautrbach
0 siblings, 1 reply; 7+ messages in thread
From: James Carter @ 2016-09-23 15:31 UTC (permalink / raw)
To: Petr Lautrbach, selinux
On 09/23/2016 05:23 AM, Petr Lautrbach wrote:
> When a user installs a module, the filename is used as the module name.
> This change was introduced with CIL language where a module name is not
> stored in the module itself. It means that when a pp module has
> different filename and stored module name, the filename is used instead
> of the stored module name. It brings problems with compatibility for
> scripts and modules which were built and used on older system and were
> migrated to the new userspace.
>
> This patch changes the behavior of semanage_direct_install_file() which
> is used by 'semodule -i' so that when a module with pp language
> extension is installed, it tries to get and use a stored module name
> instead of a filename.
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
I am not in favor of this for several reasons:
1) There would be conflicts if a module name is the same as a different module's
file name.
2) It would be confusing to have some modules referred to by their module name
and other modules by their file name.
3) In practice, very few modules use a module name that is different from their
file name.
In April checkmodule was modified to fail compiling a module when the module
name was different from the file name and pp (which does pp to CIL conversion)
will give a warning message when converting a pp file to CIL. Is that not
sufficient?
Jim
> ---
> libsemanage/src/direct_api.c | 62 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index 2187b65..f98d3d1 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -363,6 +363,48 @@ static int semanage_direct_begintrans(semanage_handle_t * sh)
>
> /********************* utility functions *********************/
>
> +/* Takes a module stored in 'module_data' and parses its headers.
> + * Sets reference variables 'module_name' to module's name, and
> + * 'version' to module's version. The caller is responsible for
> + * free()ing 'module_name', and 'version'; they will be
> + * set to NULL upon entering this function. Returns 0 on success, -1
> + * if out of memory, or -2 if data did not represent a module.
> + */
> +static int parse_module_headers(semanage_handle_t * sh, char *module_data,
> + size_t data_len, char **module_name,
> + char **version)
> +{
> + struct sepol_policy_file *pf;
> + int file_type;
> + *module_name = *version = NULL;
> +
> + if (sepol_policy_file_create(&pf)) {
> + ERR(sh, "Out of memory!");
> + return -1;
> + }
> + sepol_policy_file_set_mem(pf, module_data, data_len);
> + sepol_policy_file_set_handle(pf, sh->sepolh);
> + if (module_data == NULL ||
> + data_len == 0 ||
> + sepol_module_package_info(pf, &file_type, module_name,
> + version) == -1) {
> + sepol_policy_file_free(pf);
> + ERR(sh, "Could not parse module data.");
> + return -2;
> + }
> + sepol_policy_file_free(pf);
> + if (file_type != SEPOL_POLICY_MOD) {
> + if (file_type == SEPOL_POLICY_BASE)
> + ERR(sh,
> + "Received a base module, expected a non-base module.");
> + else
> + ERR(sh, "Data did not represent a module.");
> + return -2;
> + }
> +
> + return 0;
> +}
> +
> #include <stdlib.h>
> #include <bzlib.h>
> #include <string.h>
> @@ -1524,7 +1566,9 @@ static int semanage_direct_install_file(semanage_handle_t * sh,
> char *path = NULL;
> char *filename;
> char *lang_ext = NULL;
> + char *module_name = NULL;
> char *separator;
> + char *version = NULL;
>
> if ((data_len = map_file(sh, install_filename, &data, &compressed)) <= 0) {
> ERR(sh, "Unable to read file %s\n", install_filename);
> @@ -1564,10 +1608,26 @@ static int semanage_direct_install_file(semanage_handle_t * sh,
> lang_ext = separator + 1;
> }
>
> - retval = semanage_direct_install(sh, data, data_len, filename, lang_ext);
> + if (strcmp(lang_ext, "pp") != 0)
> + module_name = strdup(filename);
> + else {
> + if (parse_module_headers(sh, data, data_len, &module_name, &version) != 0) {
> + free(module_name);
> + module_name = strdup(filename);
> + }
> + free(version);
> + }
> + if (module_name == NULL) {
> + ERR(sh, "No memory available for module_name.\n");
> + retval = -1;
> + goto cleanup;
> + }
> +
> + retval = semanage_direct_install(sh, data, data_len, module_name, lang_ext);
>
> cleanup:
> if (data_len > 0) munmap(data, data_len);
> + free(module_name);
> free(path);
>
> return retval;
>
--
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libsemanage: use pp module headers as a source for a module name
2016-09-23 15:31 ` James Carter
@ 2016-09-23 16:05 ` Petr Lautrbach
2016-09-23 17:37 ` James Carter
0 siblings, 1 reply; 7+ messages in thread
From: Petr Lautrbach @ 2016-09-23 16:05 UTC (permalink / raw)
To: James Carter, selinux
On 09/23/2016 05:31 PM, James Carter wrote:
> On 09/23/2016 05:23 AM, Petr Lautrbach wrote:
>> When a user installs a module, the filename is used as the module name.
>> This change was introduced with CIL language where a module name is not
>> stored in the module itself. It means that when a pp module has
>> different filename and stored module name, the filename is used instead
>> of the stored module name. It brings problems with compatibility for
>> scripts and modules which were built and used on older system and were
>> migrated to the new userspace.
>>
>> This patch changes the behavior of semanage_direct_install_file() which
>> is used by 'semodule -i' so that when a module with pp language
>> extension is installed, it tries to get and use a stored module name
>> instead of a filename.
>>
>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>
> I am not in favor of this for several reasons:
> 1) There would be conflicts if a module name is the same as a different
> module's file name.
Do you mean a case where you try to install two modules moduleA.pp
moduleB.pp, where moduleA.pp uses "module moduleB 1.0;" ?
> 2) It would be confusing to have some modules referred to by their
> module name and other modules by their file name.
It's only pp modules specific and only for the initial install. 2.3
userspace used the same concept.
> 3) In practice, very few modules use a module name that is different
> from their file name.
I've already seen few reports related to this problem. We fixed it in
Fedora and RHEL but there are 3rd party modules which have this problem
> In April checkmodule was modified to fail compiling a module when the
> module name was different from the file name and pp (which does pp to
> CIL conversion) will give a warning message when converting a pp file to
> CIL. Is that not sufficient?
>
Unfortunately it doesn't affect semodule itself, there's no warning:
# /usr/libexec/selinux/hll/pp testfile.pp testfile.out
Warning: SELinux userspace will refer to the module from testfile.pp as
testfile rather than testmod
# semodule -l | grep test
# semodule -i testfile.pp
# semodule -l | grep test
testfile
# /usr/libexec/selinux/hll/pp testfile.pp testfile.out
Warning: SELinux userspace will refer to the module from testfile.pp as
testfile rather than testmod
The same warning should be either part of the patch and I' ready to add
it to the patch. Or at least there should be warning that the module
will use its filename not a name specified in the module.
Petr
>
>> ---
>> libsemanage/src/direct_api.c | 62
>> +++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 61 insertions(+), 1 deletion(-)
>>
>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
>> index 2187b65..f98d3d1 100644
>> --- a/libsemanage/src/direct_api.c
>> +++ b/libsemanage/src/direct_api.c
>> @@ -363,6 +363,48 @@ static int
>> semanage_direct_begintrans(semanage_handle_t * sh)
>>
>> /********************* utility functions *********************/
>>
>> +/* Takes a module stored in 'module_data' and parses its headers.
>> + * Sets reference variables 'module_name' to module's name, and
>> + * 'version' to module's version. The caller is responsible for
>> + * free()ing 'module_name', and 'version'; they will be
>> + * set to NULL upon entering this function. Returns 0 on success, -1
>> + * if out of memory, or -2 if data did not represent a module.
>> + */
>> +static int parse_module_headers(semanage_handle_t * sh, char
>> *module_data,
>> + size_t data_len, char **module_name,
>> + char **version)
>> +{
>> + struct sepol_policy_file *pf;
>> + int file_type;
>> + *module_name = *version = NULL;
>> +
>> + if (sepol_policy_file_create(&pf)) {
>> + ERR(sh, "Out of memory!");
>> + return -1;
>> + }
>> + sepol_policy_file_set_mem(pf, module_data, data_len);
>> + sepol_policy_file_set_handle(pf, sh->sepolh);
>> + if (module_data == NULL ||
>> + data_len == 0 ||
>> + sepol_module_package_info(pf, &file_type, module_name,
>> + version) == -1) {
>> + sepol_policy_file_free(pf);
>> + ERR(sh, "Could not parse module data.");
>> + return -2;
>> + }
>> + sepol_policy_file_free(pf);
>> + if (file_type != SEPOL_POLICY_MOD) {
>> + if (file_type == SEPOL_POLICY_BASE)
>> + ERR(sh,
>> + "Received a base module, expected a
>> non-base module.");
>> + else
>> + ERR(sh, "Data did not represent a module.");
>> + return -2;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> #include <stdlib.h>
>> #include <bzlib.h>
>> #include <string.h>
>> @@ -1524,7 +1566,9 @@ static int
>> semanage_direct_install_file(semanage_handle_t * sh,
>> char *path = NULL;
>> char *filename;
>> char *lang_ext = NULL;
>> + char *module_name = NULL;
>> char *separator;
>> + char *version = NULL;
>>
>> if ((data_len = map_file(sh, install_filename, &data,
>> &compressed)) <= 0) {
>> ERR(sh, "Unable to read file %s\n", install_filename);
>> @@ -1564,10 +1608,26 @@ static int
>> semanage_direct_install_file(semanage_handle_t * sh,
>> lang_ext = separator + 1;
>> }
>>
>> - retval = semanage_direct_install(sh, data, data_len, filename,
>> lang_ext);
>> + if (strcmp(lang_ext, "pp") != 0)
>> + module_name = strdup(filename);
>> + else {
>> + if (parse_module_headers(sh, data, data_len, &module_name,
>> &version) != 0) {
>> + free(module_name);
>> + module_name = strdup(filename);
>> + }
>> + free(version);
>> + }
>> + if (module_name == NULL) {
>> + ERR(sh, "No memory available for module_name.\n");
>> + retval = -1;
>> + goto cleanup;
>> + }
>> +
>> + retval = semanage_direct_install(sh, data, data_len, module_name,
>> lang_ext);
>>
>> cleanup:
>> if (data_len > 0) munmap(data, data_len);
>> + free(module_name);
>> free(path);
>>
>> return retval;
>>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libsemanage: use pp module headers as a source for a module name
2016-09-23 16:05 ` Petr Lautrbach
@ 2016-09-23 17:37 ` James Carter
2016-09-23 18:11 ` Petr Lautrbach
2016-09-25 21:41 ` Petr Lautrbach
0 siblings, 2 replies; 7+ messages in thread
From: James Carter @ 2016-09-23 17:37 UTC (permalink / raw)
To: Petr Lautrbach, selinux
On 09/23/2016 12:05 PM, Petr Lautrbach wrote:
> On 09/23/2016 05:31 PM, James Carter wrote:
>> On 09/23/2016 05:23 AM, Petr Lautrbach wrote:
>>> When a user installs a module, the filename is used as the module name.
>>> This change was introduced with CIL language where a module name is not
>>> stored in the module itself. It means that when a pp module has
>>> different filename and stored module name, the filename is used instead
>>> of the stored module name. It brings problems with compatibility for
>>> scripts and modules which were built and used on older system and were
>>> migrated to the new userspace.
>>>
>>> This patch changes the behavior of semanage_direct_install_file() which
>>> is used by 'semodule -i' so that when a module with pp language
>>> extension is installed, it tries to get and use a stored module name
>>> instead of a filename.
>>>
>>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>>
>> I am not in favor of this for several reasons:
>> 1) There would be conflicts if a module name is the same as a different
>> module's file name.
>
> Do you mean a case where you try to install two modules moduleA.pp
> moduleB.pp, where moduleA.pp uses "module moduleB 1.0;" ?
>
>
>> 2) It would be confusing to have some modules referred to by their
>> module name and other modules by their file name.
>
> It's only pp modules specific and only for the initial install. 2.3
> userspace used the same concept.
>
Looking closer at the patch and libsemange, it looks like this will use the
module name of a pp file as the filename. After installation the module name and
filename would be the same. Is that correct?
If so, then I don't have a problem with the approach at all. I thought you were
proposing that pp modules would use their module name for operations while being
saved in the module store by their filename.
>> 3) In practice, very few modules use a module name that is different
>> from their file name.
>
> I've already seen few reports related to this problem. We fixed it in
> Fedora and RHEL but there are 3rd party modules which have this problem
>
>> In April checkmodule was modified to fail compiling a module when the
>> module name was different from the file name and pp (which does pp to
>> CIL conversion) will give a warning message when converting a pp file to
>> CIL. Is that not sufficient?
>>
>
> Unfortunately it doesn't affect semodule itself, there's no warning:
>
> # /usr/libexec/selinux/hll/pp testfile.pp testfile.out
> Warning: SELinux userspace will refer to the module from testfile.pp as
> testfile rather than testmod
>
> # semodule -l | grep test
> # semodule -i testfile.pp
> # semodule -l | grep test
> testfile
>
> # /usr/libexec/selinux/hll/pp testfile.pp testfile.out
> Warning: SELinux userspace will refer to the module from testfile.pp as
> testfile rather than testmod
>
>
> The same warning should be either part of the patch and I' ready to add
> it to the patch. Or at least there should be warning that the module
> will use its filename not a name specified in the module.
>
>
I tested pp, but must not have tested how it worked when used by semodule. We
should have a warning here and I would appreciate you adding it to the patch.
Thanks,
Jim
>
> Petr
>
>
>>
>>> ---
>>> libsemanage/src/direct_api.c | 62
>>> +++++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 61 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
>>> index 2187b65..f98d3d1 100644
>>> --- a/libsemanage/src/direct_api.c
>>> +++ b/libsemanage/src/direct_api.c
>>> @@ -363,6 +363,48 @@ static int
>>> semanage_direct_begintrans(semanage_handle_t * sh)
>>>
>>> /********************* utility functions *********************/
>>>
>>> +/* Takes a module stored in 'module_data' and parses its headers.
>>> + * Sets reference variables 'module_name' to module's name, and
>>> + * 'version' to module's version. The caller is responsible for
>>> + * free()ing 'module_name', and 'version'; they will be
>>> + * set to NULL upon entering this function. Returns 0 on success, -1
>>> + * if out of memory, or -2 if data did not represent a module.
>>> + */
>>> +static int parse_module_headers(semanage_handle_t * sh, char
>>> *module_data,
>>> + size_t data_len, char **module_name,
>>> + char **version)
>>> +{
>>> + struct sepol_policy_file *pf;
>>> + int file_type;
>>> + *module_name = *version = NULL;
>>> +
>>> + if (sepol_policy_file_create(&pf)) {
>>> + ERR(sh, "Out of memory!");
>>> + return -1;
>>> + }
>>> + sepol_policy_file_set_mem(pf, module_data, data_len);
>>> + sepol_policy_file_set_handle(pf, sh->sepolh);
>>> + if (module_data == NULL ||
>>> + data_len == 0 ||
>>> + sepol_module_package_info(pf, &file_type, module_name,
>>> + version) == -1) {
>>> + sepol_policy_file_free(pf);
>>> + ERR(sh, "Could not parse module data.");
>>> + return -2;
>>> + }
>>> + sepol_policy_file_free(pf);
>>> + if (file_type != SEPOL_POLICY_MOD) {
>>> + if (file_type == SEPOL_POLICY_BASE)
>>> + ERR(sh,
>>> + "Received a base module, expected a
>>> non-base module.");
>>> + else
>>> + ERR(sh, "Data did not represent a module.");
>>> + return -2;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> #include <stdlib.h>
>>> #include <bzlib.h>
>>> #include <string.h>
>>> @@ -1524,7 +1566,9 @@ static int
>>> semanage_direct_install_file(semanage_handle_t * sh,
>>> char *path = NULL;
>>> char *filename;
>>> char *lang_ext = NULL;
>>> + char *module_name = NULL;
>>> char *separator;
>>> + char *version = NULL;
>>>
>>> if ((data_len = map_file(sh, install_filename, &data,
>>> &compressed)) <= 0) {
>>> ERR(sh, "Unable to read file %s\n", install_filename);
>>> @@ -1564,10 +1608,26 @@ static int
>>> semanage_direct_install_file(semanage_handle_t * sh,
>>> lang_ext = separator + 1;
>>> }
>>>
>>> - retval = semanage_direct_install(sh, data, data_len, filename,
>>> lang_ext);
>>> + if (strcmp(lang_ext, "pp") != 0)
>>> + module_name = strdup(filename);
>>> + else {
>>> + if (parse_module_headers(sh, data, data_len, &module_name,
>>> &version) != 0) {
>>> + free(module_name);
>>> + module_name = strdup(filename);
>>> + }
>>> + free(version);
>>> + }
>>> + if (module_name == NULL) {
>>> + ERR(sh, "No memory available for module_name.\n");
>>> + retval = -1;
>>> + goto cleanup;
>>> + }
>>> +
>>> + retval = semanage_direct_install(sh, data, data_len, module_name,
>>> lang_ext);
>>>
>>> cleanup:
>>> if (data_len > 0) munmap(data, data_len);
>>> + free(module_name);
>>> free(path);
>>>
>>> return retval;
>>>
>>
>>
--
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libsemanage: use pp module headers as a source for a module name
2016-09-23 17:37 ` James Carter
@ 2016-09-23 18:11 ` Petr Lautrbach
2016-09-25 21:41 ` Petr Lautrbach
1 sibling, 0 replies; 7+ messages in thread
From: Petr Lautrbach @ 2016-09-23 18:11 UTC (permalink / raw)
To: James Carter; +Cc: selinux
On Fri, Sep 23, 2016 at 01:37:26PM -0400, James Carter wrote:
> On 09/23/2016 12:05 PM, Petr Lautrbach wrote:
> > On 09/23/2016 05:31 PM, James Carter wrote:
> > > On 09/23/2016 05:23 AM, Petr Lautrbach wrote:
> > > > When a user installs a module, the filename is used as the module name.
> > > > This change was introduced with CIL language where a module name is not
> > > > stored in the module itself. It means that when a pp module has
> > > > different filename and stored module name, the filename is used instead
> > > > of the stored module name. It brings problems with compatibility for
> > > > scripts and modules which were built and used on older system and were
> > > > migrated to the new userspace.
> > > >
> > > > This patch changes the behavior of semanage_direct_install_file() which
> > > > is used by 'semodule -i' so that when a module with pp language
> > > > extension is installed, it tries to get and use a stored module name
> > > > instead of a filename.
> > > >
> > > > Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> > >
> > > I am not in favor of this for several reasons:
> > > 1) There would be conflicts if a module name is the same as a different
> > > module's file name.
> >
> > Do you mean a case where you try to install two modules moduleA.pp
> > moduleB.pp, where moduleA.pp uses "module moduleB 1.0;" ?
> >
> >
> > > 2) It would be confusing to have some modules referred to by their
> > > module name and other modules by their file name.
> >
> > It's only pp modules specific and only for the initial install. 2.3
> > userspace used the same concept.
> >
>
> Looking closer at the patch and libsemange, it looks like this will use the
> module name of a pp file as the filename. After installation the module name
> and filename would be the same. Is that correct?
The original filename is dropped and only the name stored in the module
is used. The filename is used only as a fallback when it can't resolve
the name from the module.
# semodule -i testfile.pp
# semodule -l | grep test
testmod
# ls /etc/selinux/targeted/active/modules/400/testmod
cil hll lang_ext
> If so, then I don't have a problem with the approach at all. I thought you
> were proposing that pp modules would use their module name for operations
> while being saved in the module store by their filename.
>
>
> > > 3) In practice, very few modules use a module name that is different
> > > from their file name.
> >
> > I've already seen few reports related to this problem. We fixed it in
> > Fedora and RHEL but there are 3rd party modules which have this problem
> >
> > > In April checkmodule was modified to fail compiling a module when the
> > > module name was different from the file name and pp (which does pp to
> > > CIL conversion) will give a warning message when converting a pp file to
> > > CIL. Is that not sufficient?
> > >
> >
> > Unfortunately it doesn't affect semodule itself, there's no warning:
> >
> > # /usr/libexec/selinux/hll/pp testfile.pp testfile.out
> > Warning: SELinux userspace will refer to the module from testfile.pp as
> > testfile rather than testmod
> >
> > # semodule -l | grep test
> > # semodule -i testfile.pp
> > # semodule -l | grep test
> > testfile
> >
> > # /usr/libexec/selinux/hll/pp testfile.pp testfile.out
> > Warning: SELinux userspace will refer to the module from testfile.pp as
> > testfile rather than testmod
> >
> >
> > The same warning should be either part of the patch and I' ready to add
> > it to the patch. Or at least there should be warning that the module
> > will use its filename not a name specified in the module.
> >
> >
>
> I tested pp, but must not have tested how it worked when used by semodule.
> We should have a warning here and I would appreciate you adding it to the
> patch.
>
> Thanks,
> Jim
>
> >
> > Petr
> >
> >
> > >
> > > > ---
> > > > libsemanage/src/direct_api.c | 62
> > > > +++++++++++++++++++++++++++++++++++++++++++-
> > > > 1 file changed, 61 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> > > > index 2187b65..f98d3d1 100644
> > > > --- a/libsemanage/src/direct_api.c
> > > > +++ b/libsemanage/src/direct_api.c
> > > > @@ -363,6 +363,48 @@ static int
> > > > semanage_direct_begintrans(semanage_handle_t * sh)
> > > >
> > > > /********************* utility functions *********************/
> > > >
> > > > +/* Takes a module stored in 'module_data' and parses its headers.
> > > > + * Sets reference variables 'module_name' to module's name, and
> > > > + * 'version' to module's version. The caller is responsible for
> > > > + * free()ing 'module_name', and 'version'; they will be
> > > > + * set to NULL upon entering this function. Returns 0 on success, -1
> > > > + * if out of memory, or -2 if data did not represent a module.
> > > > + */
> > > > +static int parse_module_headers(semanage_handle_t * sh, char
> > > > *module_data,
> > > > + size_t data_len, char **module_name,
> > > > + char **version)
> > > > +{
> > > > + struct sepol_policy_file *pf;
> > > > + int file_type;
> > > > + *module_name = *version = NULL;
> > > > +
> > > > + if (sepol_policy_file_create(&pf)) {
> > > > + ERR(sh, "Out of memory!");
> > > > + return -1;
> > > > + }
> > > > + sepol_policy_file_set_mem(pf, module_data, data_len);
> > > > + sepol_policy_file_set_handle(pf, sh->sepolh);
> > > > + if (module_data == NULL ||
> > > > + data_len == 0 ||
> > > > + sepol_module_package_info(pf, &file_type, module_name,
> > > > + version) == -1) {
> > > > + sepol_policy_file_free(pf);
> > > > + ERR(sh, "Could not parse module data.");
> > > > + return -2;
> > > > + }
> > > > + sepol_policy_file_free(pf);
> > > > + if (file_type != SEPOL_POLICY_MOD) {
> > > > + if (file_type == SEPOL_POLICY_BASE)
> > > > + ERR(sh,
> > > > + "Received a base module, expected a
> > > > non-base module.");
> > > > + else
> > > > + ERR(sh, "Data did not represent a module.");
> > > > + return -2;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > #include <stdlib.h>
> > > > #include <bzlib.h>
> > > > #include <string.h>
> > > > @@ -1524,7 +1566,9 @@ static int
> > > > semanage_direct_install_file(semanage_handle_t * sh,
> > > > char *path = NULL;
> > > > char *filename;
> > > > char *lang_ext = NULL;
> > > > + char *module_name = NULL;
> > > > char *separator;
> > > > + char *version = NULL;
> > > >
> > > > if ((data_len = map_file(sh, install_filename, &data,
> > > > &compressed)) <= 0) {
> > > > ERR(sh, "Unable to read file %s\n", install_filename);
> > > > @@ -1564,10 +1608,26 @@ static int
> > > > semanage_direct_install_file(semanage_handle_t * sh,
> > > > lang_ext = separator + 1;
> > > > }
> > > >
> > > > - retval = semanage_direct_install(sh, data, data_len, filename,
> > > > lang_ext);
> > > > + if (strcmp(lang_ext, "pp") != 0)
> > > > + module_name = strdup(filename);
> > > > + else {
> > > > + if (parse_module_headers(sh, data, data_len, &module_name,
> > > > &version) != 0) {
> > > > + free(module_name);
> > > > + module_name = strdup(filename);
> > > > + }
> > > > + free(version);
> > > > + }
> > > > + if (module_name == NULL) {
> > > > + ERR(sh, "No memory available for module_name.\n");
> > > > + retval = -1;
> > > > + goto cleanup;
> > > > + }
> > > > +
> > > > + retval = semanage_direct_install(sh, data, data_len, module_name,
> > > > lang_ext);
> > > >
> > > > cleanup:
> > > > if (data_len > 0) munmap(data, data_len);
> > > > + free(module_name);
> > > > free(path);
> > > >
> > > > return retval;
> > > >
> > >
> > >
>
>
> --
> James Carter <jwcart2@tycho.nsa.gov>
> National Security Agency
--
Petr Lautrbach
SELinux Solutions
Red Hat
Better technology. Faster innovation. Powered by community collaboration.
See how it works at redhat.com.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libsemanage: use pp module headers as a source for a module name
2016-09-23 17:37 ` James Carter
2016-09-23 18:11 ` Petr Lautrbach
@ 2016-09-25 21:41 ` Petr Lautrbach
2016-09-26 15:51 ` James Carter
1 sibling, 1 reply; 7+ messages in thread
From: Petr Lautrbach @ 2016-09-25 21:41 UTC (permalink / raw)
To: James Carter; +Cc: selinux
On Fri, Sep 23, 2016 at 01:37:26PM -0400, James Carter wrote:
> On 09/23/2016 12:05 PM, Petr Lautrbach wrote:
> > On 09/23/2016 05:31 PM, James Carter wrote:
> > > On 09/23/2016 05:23 AM, Petr Lautrbach wrote:
> > > > When a user installs a module, the filename is used as the module name.
> > > > This change was introduced with CIL language where a module name is not
> > > > stored in the module itself. It means that when a pp module has
> > > > different filename and stored module name, the filename is used instead
> > > > of the stored module name. It brings problems with compatibility for
> > > > scripts and modules which were built and used on older system and were
> > > > migrated to the new userspace.
> > > >
> > > > This patch changes the behavior of semanage_direct_install_file() which
> > > > is used by 'semodule -i' so that when a module with pp language
> > > > extension is installed, it tries to get and use a stored module name
> > > > instead of a filename.
> > > >
> > > > Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> > >
> > > I am not in favor of this for several reasons:
> > > 1) There would be conflicts if a module name is the same as a different
> > > module's file name.
> >
> > Do you mean a case where you try to install two modules moduleA.pp
> > moduleB.pp, where moduleA.pp uses "module moduleB 1.0;" ?
> >
> >
> > > 2) It would be confusing to have some modules referred to by their
> > > module name and other modules by their file name.
> >
> > It's only pp modules specific and only for the initial install. 2.3
> > userspace used the same concept.
> >
>
> Looking closer at the patch and libsemange, it looks like this will use the
> module name of a pp file as the filename. After installation the module name
> and filename would be the same. Is that correct?
>
> If so, then I don't have a problem with the approach at all. I thought you
> were proposing that pp modules would use their module name for operations
> while being saved in the module store by their filename.
>
>
> > > 3) In practice, very few modules use a module name that is different
> > > from their file name.
> >
> > I've already seen few reports related to this problem. We fixed it in
> > Fedora and RHEL but there are 3rd party modules which have this problem
> >
> > > In April checkmodule was modified to fail compiling a module when the
> > > module name was different from the file name and pp (which does pp to
> > > CIL conversion) will give a warning message when converting a pp file to
> > > CIL. Is that not sufficient?
> > >
> >
> > Unfortunately it doesn't affect semodule itself, there's no warning:
> >
> > # /usr/libexec/selinux/hll/pp testfile.pp testfile.out
> > Warning: SELinux userspace will refer to the module from testfile.pp as
> > testfile rather than testmod
> >
> > # semodule -l | grep test
> > # semodule -i testfile.pp
> > # semodule -l | grep test
> > testfile
> >
> > # /usr/libexec/selinux/hll/pp testfile.pp testfile.out
> > Warning: SELinux userspace will refer to the module from testfile.pp as
> > testfile rather than testmod
> >
> >
> > The same warning should be either part of the patch and I' ready to add
> > it to the patch. Or at least there should be warning that the module
> > will use its filename not a name specified in the module.
> >
> >
>
> I tested pp, but must not have tested how it worked when used by semodule.
> We should have a warning here and I would appreciate you adding it to the
> patch.
>
Sent as [PATCH v2] libsemanage: Use pp module name instead of filename.
There's a warning now and I changed the warning in hll/pp to reflect
this change as well.
But I guess your objections still apply.
I'll be offline for next 7 days and I won't be able respond to anything.
Thanks,
Petr
>
> >
> > Petr
> >
> >
> > >
> > > > ---
> > > > libsemanage/src/direct_api.c | 62
> > > > +++++++++++++++++++++++++++++++++++++++++++-
> > > > 1 file changed, 61 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> > > > index 2187b65..f98d3d1 100644
> > > > --- a/libsemanage/src/direct_api.c
> > > > +++ b/libsemanage/src/direct_api.c
> > > > @@ -363,6 +363,48 @@ static int
> > > > semanage_direct_begintrans(semanage_handle_t * sh)
> > > >
> > > > /********************* utility functions *********************/
> > > >
> > > > +/* Takes a module stored in 'module_data' and parses its headers.
> > > > + * Sets reference variables 'module_name' to module's name, and
> > > > + * 'version' to module's version. The caller is responsible for
> > > > + * free()ing 'module_name', and 'version'; they will be
> > > > + * set to NULL upon entering this function. Returns 0 on success, -1
> > > > + * if out of memory, or -2 if data did not represent a module.
> > > > + */
> > > > +static int parse_module_headers(semanage_handle_t * sh, char
> > > > *module_data,
> > > > + size_t data_len, char **module_name,
> > > > + char **version)
> > > > +{
> > > > + struct sepol_policy_file *pf;
> > > > + int file_type;
> > > > + *module_name = *version = NULL;
> > > > +
> > > > + if (sepol_policy_file_create(&pf)) {
> > > > + ERR(sh, "Out of memory!");
> > > > + return -1;
> > > > + }
> > > > + sepol_policy_file_set_mem(pf, module_data, data_len);
> > > > + sepol_policy_file_set_handle(pf, sh->sepolh);
> > > > + if (module_data == NULL ||
> > > > + data_len == 0 ||
> > > > + sepol_module_package_info(pf, &file_type, module_name,
> > > > + version) == -1) {
> > > > + sepol_policy_file_free(pf);
> > > > + ERR(sh, "Could not parse module data.");
> > > > + return -2;
> > > > + }
> > > > + sepol_policy_file_free(pf);
> > > > + if (file_type != SEPOL_POLICY_MOD) {
> > > > + if (file_type == SEPOL_POLICY_BASE)
> > > > + ERR(sh,
> > > > + "Received a base module, expected a
> > > > non-base module.");
> > > > + else
> > > > + ERR(sh, "Data did not represent a module.");
> > > > + return -2;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > #include <stdlib.h>
> > > > #include <bzlib.h>
> > > > #include <string.h>
> > > > @@ -1524,7 +1566,9 @@ static int
> > > > semanage_direct_install_file(semanage_handle_t * sh,
> > > > char *path = NULL;
> > > > char *filename;
> > > > char *lang_ext = NULL;
> > > > + char *module_name = NULL;
> > > > char *separator;
> > > > + char *version = NULL;
> > > >
> > > > if ((data_len = map_file(sh, install_filename, &data,
> > > > &compressed)) <= 0) {
> > > > ERR(sh, "Unable to read file %s\n", install_filename);
> > > > @@ -1564,10 +1608,26 @@ static int
> > > > semanage_direct_install_file(semanage_handle_t * sh,
> > > > lang_ext = separator + 1;
> > > > }
> > > >
> > > > - retval = semanage_direct_install(sh, data, data_len, filename,
> > > > lang_ext);
> > > > + if (strcmp(lang_ext, "pp") != 0)
> > > > + module_name = strdup(filename);
> > > > + else {
> > > > + if (parse_module_headers(sh, data, data_len, &module_name,
> > > > &version) != 0) {
> > > > + free(module_name);
> > > > + module_name = strdup(filename);
> > > > + }
> > > > + free(version);
> > > > + }
> > > > + if (module_name == NULL) {
> > > > + ERR(sh, "No memory available for module_name.\n");
> > > > + retval = -1;
> > > > + goto cleanup;
> > > > + }
> > > > +
> > > > + retval = semanage_direct_install(sh, data, data_len, module_name,
> > > > lang_ext);
> > > >
> > > > cleanup:
> > > > if (data_len > 0) munmap(data, data_len);
> > > > + free(module_name);
> > > > free(path);
> > > >
> > > > return retval;
> > > >
> > >
> > >
>
>
> --
> James Carter <jwcart2@tycho.nsa.gov>
> National Security Agency
--
Petr Lautrbach
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libsemanage: use pp module headers as a source for a module name
2016-09-25 21:41 ` Petr Lautrbach
@ 2016-09-26 15:51 ` James Carter
0 siblings, 0 replies; 7+ messages in thread
From: James Carter @ 2016-09-26 15:51 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: selinux
On 09/25/2016 05:41 PM, Petr Lautrbach wrote:
> On Fri, Sep 23, 2016 at 01:37:26PM -0400, James Carter wrote:
>> On 09/23/2016 12:05 PM, Petr Lautrbach wrote:
>>> On 09/23/2016 05:31 PM, James Carter wrote:
>>>> On 09/23/2016 05:23 AM, Petr Lautrbach wrote:
>>>>> When a user installs a module, the filename is used as the module name.
>>>>> This change was introduced with CIL language where a module name is not
>>>>> stored in the module itself. It means that when a pp module has
>>>>> different filename and stored module name, the filename is used instead
>>>>> of the stored module name. It brings problems with compatibility for
>>>>> scripts and modules which were built and used on older system and were
>>>>> migrated to the new userspace.
>>>>>
>>>>> This patch changes the behavior of semanage_direct_install_file() which
>>>>> is used by 'semodule -i' so that when a module with pp language
>>>>> extension is installed, it tries to get and use a stored module name
>>>>> instead of a filename.
>>>>>
>>>>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>>>>
>>>> I am not in favor of this for several reasons:
>>>> 1) There would be conflicts if a module name is the same as a different
>>>> module's file name.
>>>
>>> Do you mean a case where you try to install two modules moduleA.pp
>>> moduleB.pp, where moduleA.pp uses "module moduleB 1.0;" ?
>>>
>>>
>>>> 2) It would be confusing to have some modules referred to by their
>>>> module name and other modules by their file name.
>>>
>>> It's only pp modules specific and only for the initial install. 2.3
>>> userspace used the same concept.
>>>
>>
>> Looking closer at the patch and libsemange, it looks like this will use the
>> module name of a pp file as the filename. After installation the module name
>> and filename would be the same. Is that correct?
>>
>> If so, then I don't have a problem with the approach at all. I thought you
>> were proposing that pp modules would use their module name for operations
>> while being saved in the module store by their filename.
>>
>>
>>>> 3) In practice, very few modules use a module name that is different
>>>> from their file name.
>>>
>>> I've already seen few reports related to this problem. We fixed it in
>>> Fedora and RHEL but there are 3rd party modules which have this problem
>>>
>>>> In April checkmodule was modified to fail compiling a module when the
>>>> module name was different from the file name and pp (which does pp to
>>>> CIL conversion) will give a warning message when converting a pp file to
>>>> CIL. Is that not sufficient?
>>>>
>>>
>>> Unfortunately it doesn't affect semodule itself, there's no warning:
>>>
>>> # /usr/libexec/selinux/hll/pp testfile.pp testfile.out
>>> Warning: SELinux userspace will refer to the module from testfile.pp as
>>> testfile rather than testmod
>>>
>>> # semodule -l | grep test
>>> # semodule -i testfile.pp
>>> # semodule -l | grep test
>>> testfile
>>>
>>> # /usr/libexec/selinux/hll/pp testfile.pp testfile.out
>>> Warning: SELinux userspace will refer to the module from testfile.pp as
>>> testfile rather than testmod
>>>
>>>
>>> The same warning should be either part of the patch and I' ready to add
>>> it to the patch. Or at least there should be warning that the module
>>> will use its filename not a name specified in the module.
>>>
>>>
>>
>> I tested pp, but must not have tested how it worked when used by semodule.
>> We should have a warning here and I would appreciate you adding it to the
>> patch.
>>
>
> Sent as [PATCH v2] libsemanage: Use pp module name instead of filename.
> There's a warning now and I changed the warning in hll/pp to reflect
> this change as well.
>
> But I guess your objections still apply.
>
My objections were based on my faulty understanding of what the patch was doing.
Sorry about that.
Jim
> I'll be offline for next 7 days and I won't be able respond to anything.
>
> Thanks,
>
> Petr
>
>
>>
>>>
>>> Petr
>>>
>>>
>>>>
>>>>> ---
>>>>> libsemanage/src/direct_api.c | 62
>>>>> +++++++++++++++++++++++++++++++++++++++++++-
>>>>> 1 file changed, 61 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
>>>>> index 2187b65..f98d3d1 100644
>>>>> --- a/libsemanage/src/direct_api.c
>>>>> +++ b/libsemanage/src/direct_api.c
>>>>> @@ -363,6 +363,48 @@ static int
>>>>> semanage_direct_begintrans(semanage_handle_t * sh)
>>>>>
>>>>> /********************* utility functions *********************/
>>>>>
>>>>> +/* Takes a module stored in 'module_data' and parses its headers.
>>>>> + * Sets reference variables 'module_name' to module's name, and
>>>>> + * 'version' to module's version. The caller is responsible for
>>>>> + * free()ing 'module_name', and 'version'; they will be
>>>>> + * set to NULL upon entering this function. Returns 0 on success, -1
>>>>> + * if out of memory, or -2 if data did not represent a module.
>>>>> + */
>>>>> +static int parse_module_headers(semanage_handle_t * sh, char
>>>>> *module_data,
>>>>> + size_t data_len, char **module_name,
>>>>> + char **version)
>>>>> +{
>>>>> + struct sepol_policy_file *pf;
>>>>> + int file_type;
>>>>> + *module_name = *version = NULL;
>>>>> +
>>>>> + if (sepol_policy_file_create(&pf)) {
>>>>> + ERR(sh, "Out of memory!");
>>>>> + return -1;
>>>>> + }
>>>>> + sepol_policy_file_set_mem(pf, module_data, data_len);
>>>>> + sepol_policy_file_set_handle(pf, sh->sepolh);
>>>>> + if (module_data == NULL ||
>>>>> + data_len == 0 ||
>>>>> + sepol_module_package_info(pf, &file_type, module_name,
>>>>> + version) == -1) {
>>>>> + sepol_policy_file_free(pf);
>>>>> + ERR(sh, "Could not parse module data.");
>>>>> + return -2;
>>>>> + }
>>>>> + sepol_policy_file_free(pf);
>>>>> + if (file_type != SEPOL_POLICY_MOD) {
>>>>> + if (file_type == SEPOL_POLICY_BASE)
>>>>> + ERR(sh,
>>>>> + "Received a base module, expected a
>>>>> non-base module.");
>>>>> + else
>>>>> + ERR(sh, "Data did not represent a module.");
>>>>> + return -2;
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> #include <stdlib.h>
>>>>> #include <bzlib.h>
>>>>> #include <string.h>
>>>>> @@ -1524,7 +1566,9 @@ static int
>>>>> semanage_direct_install_file(semanage_handle_t * sh,
>>>>> char *path = NULL;
>>>>> char *filename;
>>>>> char *lang_ext = NULL;
>>>>> + char *module_name = NULL;
>>>>> char *separator;
>>>>> + char *version = NULL;
>>>>>
>>>>> if ((data_len = map_file(sh, install_filename, &data,
>>>>> &compressed)) <= 0) {
>>>>> ERR(sh, "Unable to read file %s\n", install_filename);
>>>>> @@ -1564,10 +1608,26 @@ static int
>>>>> semanage_direct_install_file(semanage_handle_t * sh,
>>>>> lang_ext = separator + 1;
>>>>> }
>>>>>
>>>>> - retval = semanage_direct_install(sh, data, data_len, filename,
>>>>> lang_ext);
>>>>> + if (strcmp(lang_ext, "pp") != 0)
>>>>> + module_name = strdup(filename);
>>>>> + else {
>>>>> + if (parse_module_headers(sh, data, data_len, &module_name,
>>>>> &version) != 0) {
>>>>> + free(module_name);
>>>>> + module_name = strdup(filename);
>>>>> + }
>>>>> + free(version);
>>>>> + }
>>>>> + if (module_name == NULL) {
>>>>> + ERR(sh, "No memory available for module_name.\n");
>>>>> + retval = -1;
>>>>> + goto cleanup;
>>>>> + }
>>>>> +
>>>>> + retval = semanage_direct_install(sh, data, data_len, module_name,
>>>>> lang_ext);
>>>>>
>>>>> cleanup:
>>>>> if (data_len > 0) munmap(data, data_len);
>>>>> + free(module_name);
>>>>> free(path);
>>>>>
>>>>> return retval;
>>>>>
>>>>
>>>>
>>
>>
>> --
>> James Carter <jwcart2@tycho.nsa.gov>
>> National Security Agency
>
--
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-09-26 15:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-23 9:23 [PATCH] libsemanage: use pp module headers as a source for a module name Petr Lautrbach
2016-09-23 15:31 ` James Carter
2016-09-23 16:05 ` Petr Lautrbach
2016-09-23 17:37 ` James Carter
2016-09-23 18:11 ` Petr Lautrbach
2016-09-25 21:41 ` Petr Lautrbach
2016-09-26 15:51 ` James Carter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.