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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxx>
> National Security Agency
--
Petr Lautrbach
SELinux Solutions
Red Hat
Better technology. Faster innovation. Powered by community collaboration.
See how it works at redhat.com.
_______________________________________________
Selinux mailing list
Selinux@xxxxxxxxxxxxx
To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
