Re: [systemd-devel] [PATCH 2/2] main: added support for loading IMA custom policies

[Roberto Sassu <roberto.sassu@xxxxxxxxx>]

On 02/20/2012 06:12 PM, Lennart Poettering wrote:
> On Wed, 15.02.12 14:23, Roberto Sassu (roberto.sassu@xxxxxxxxx) wrote:
>
> > The new function ima_setup() loads an IMA custom policy from a file in the
> > default location '/etc/sysconfig/ima-policy', if present, and writes it to
> > the path 'ima/policy' in the security filesystem. This function is executed
> > at early stage in order to avoid that some file operations are not measured
> > by IMA and it is placed after the initialization of SELinux because IMA
> > needs the latter (or other security modules) to understand LSM-specific
> > rules.
>
> This must be a configure option. I am pretty sure most embedded people
> don't require this feature.
>
> The kernel side of things is merged upstream I presume? (We generally
> only want to support stuff in our code that is merged upstream itself)

Yes. IMA was in the mainline kernel since 2.6.30.


> > +#define IMA_SECFS_DIR SECURITYFS_MNTPOINT "/ima"
> > +#define IMA_SECFS_POLICY IMA_SECFS_DIR "/policy"
>
> Please use proper strings for this. (see my earlier mail)

Ok, i will replace the former with the hard-coded pathname.


> > +#define IMA_POLICY_PATH "/etc/sysconfig/ima-policy"
>
> This is a Fedoraism. Please introduce a proper configuration file for this.

Ok, i will answer about this in the next your email.


> > +
> > +int ima_setup(void) {
> > +       struct stat st;
> > +       ssize_t policy_size = 0, written = 0;
> > +       char *policy;
> > +       int policyfd = -1, imafd = -1;
> > +       int result = 0;
> > +
> > +#ifndef HAVE_SELINUX
> > +       /* Mount the securityfs filesystem */
> > +       mount_setup_early();
> > +#endif
> > +
> > +       if (stat(IMA_POLICY_PATH,&st) == -1)
> > +               return 0;
>
> We tend to do "<  0" instead of "== -1" checks for syscall
> failures. Might be good to use the same here, but this is not necessary
> for getting this merged.

Ok.


> > +
> > +       policyfd = open(IMA_POLICY_PATH, O_RDONLY);
>
> We tend to add O_CLOEXEC to all fds we open, just for being
> paranoid. Please do so here, too, to avoid surprise and avoid exceptions
> when people grep for all open() invocations looking for O_CLOEXEC.

No problem, i will do the change.


> > +       if (policyfd<  0) {
> > +               log_error("Failed to open the IMA custom policy file %s (%s), "
> > +                         "ignoring.", IMA_POLICY_PATH, strerror(errno));
> > +               return 0;
> > +       }
>
> Consider using %m instead of %s and strerror(errno).
>
> > +       imafd = open(IMA_SECFS_POLICY, O_WRONLY);
>
> Also O_CLOEXEC please.
>
> > +       if (imafd<  0) {
> > +               log_error("Failed to open the IMA kernel interface %s (%s), "
> > +                         "ignoring.", IMA_SECFS_POLICY, strerror(errno));
> > +               goto out;
> > +       }
> > +
> > +       policy = mmap(NULL, policy_size, PROT_READ, MAP_PRIVATE, policyfd, 0);
> > +       if (policy == NULL) {
>
> mmap() returns MAP_FAILED (i.e. (void) -1) on failure, not NULL. This
> check needs to be fixed.

Ok, i will replace NULL with MAP_FAILED.


> > +               log_error("mmap() failed (%s), freezing", strerror(errno));
> > +               result = -errno;
> > +               goto out;
> > +       }
> > +
> > +       while(written<  policy_size) {
> > +               ssize_t len = write(imafd, policy + written,
> > +                                   policy_size - written);
> > +               if (len<= 0) {
> > +                         log_error("Failed to load the IMA custom policy "
> > +                                   "file %s (%s), ignoring.", IMA_POLICY_PATH,
> > +                                   strerror(errno));
> > +                         goto out_mmap;
> > +               }
> > +               written += len;
> > +       }
>
> It might make sense to use loop_write() here instead, which does more or
> less this loop, and is defined in util.c anyway.

I briefly looked at the code and i'm not sure to use it, because i want
to add some extra information in the output message (for example the
line number of the rule in the policy file that was rejected by IMA).

Thanks

Roberto Sassu


> Otherwise looks good.
>
> Lennart

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html