[Rpm-maint] [PATCH v3 2/2] ima-plugin: Move the IMA plugin to the fsm_file_prepare hook

Panu Matilainen pmatilai at laiskiainen.org
Wed Sep 28 08:43:12 UTC 2016


On 09/24/2016 12:11 AM, Stefan Berger wrote:
> Since newly installed files may be invoked by post install scriptlets,
> we need to have them signed before the scriptlets are executed.
> Therefore, we now move the IMA plugin to the fsm_file_prepare hook.

There are other reasons for the change too, such as to correctly handle 
skipped and shared files. Also the patch adds error handling for 
lsetxattr() which might deserve a note as it could trip up something 
that has previously gone unseen. Maybe these can be added while 
applying, seems a bit redundant to send another patch version just for that.

>
> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
> ---
>  plugins/ima.c | 43 +++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/plugins/ima.c b/plugins/ima.c
> index 76c7d3d..9264708 100644
> --- a/plugins/ima.c
> +++ b/plugins/ima.c
> @@ -1,9 +1,11 @@
> +#include <errno.h>
>  #include <sys/xattr.h>
>
>  #include <rpm/rpmfi.h>
>  #include <rpm/rpmte.h>
>  #include <rpm/rpmfiles.h>
>  #include <rpm/rpmtypes.h>
> +#include <rpm/rpmlog.h>
>  #include <rpmio/rpmstring.h>
>
>  #include "lib/rpmfs.h"
> @@ -35,38 +37,39 @@ static int check_zero_hdr(const unsigned char *fsig, size_t siglen)
>  	return (memcmp(fsig, &zero_hdr, sizeof(zero_hdr)) == 0);
>  }
>
> -static rpmRC ima_psm_post(rpmPlugin plugin, rpmte te, int res)
> +static rpmRC ima_fsm_file_prepare(rpmPlugin plugin, rpmfi fi,
> +                                  const char *path,
> +                                  const char *dest,
> +                                  mode_t file_mode, rpmFsmOp op)
>  {
> -	rpmfi fi = rpmteFI(te);
> -	const char *fpath;
>  	const unsigned char * fsig = NULL;
>  	size_t len;
> -	int rc = 0;
> +	int rc = RPMRC_OK;
> +	rpmFileAction action = XFO_ACTION(op);
>
> -	if (fi == NULL) {
> -	    rc = RPMERR_BAD_MAGIC;
> +	if (!fi || !path || XFA_SKIPPING(action))
>  	    goto exit;

Path cannot be NULL here. It'd be a grave bug someplace else if that 
were the case, and in that case it'd be better not to paper over it. So 
assert() would be closer to the mark. Probably not really worth another 
version of the patch alone though.

In fact (slowly remembering bits and pieces) this check could be also 
written as

	if (XFA_SKIPPING(action) || (op & FAF_UNOWNED))
	    goto exit;

...which is perhaps a little bit more self-documenting than checking for 
NULL fi, which is easily mistaken for "dont segfault on NULL" check when 
it actually *means* unowned path and is perfectly ok, we just dont want 
to sign it.

> -	}
>
> -	while (rpmfiNext(fi) >= 0) {
> -	    /* Don't install signatures for (mutable) files marked
> -	     * as config files unless they are also executable.
> -	     */
> -	    if (rpmfiFFlags(fi) & RPMFILE_CONFIG) {
> -	        if (!(rpmfiFMode(fi) & (S_IXUSR|S_IXGRP|S_IXOTH)))
> -	            continue;
> -	    }
> +	/* Don't install signatures for (mutable) files marked
> +	 * as config files unless they are also executable.
> +	 */
> +	if (rpmfiFFlags(fi) & RPMFILE_CONFIG) {
> +	    if (!(rpmfiFMode(fi) & (S_IXUSR|S_IXGRP|S_IXOTH)))
> +	        goto exit;
> +	}
>
> -	    fsig = rpmfiFSignature(fi, &len);
> -	    if (fsig && (check_zero_hdr(fsig, len) == 0)) {
> -	        fpath = rpmfiFN(fi);
> -	        lsetxattr(fpath, XATTR_NAME_IMA, fsig, len, 0);
> +	fsig = rpmfiFSignature(fi, &len);
> +	if (fsig && (check_zero_hdr(fsig, len) == 0)) {
> +	    if (lsetxattr(path, XATTR_NAME_IMA, fsig, len, 0) < 0) {
> +	        rpmlog(RPMLOG_ERR, "ima: could not apply signature on '%s': %s\n", path, strerror(errno));
> +	        rc = RPMRC_FAIL;
>  	    }
>  	}
> +
>  exit:
>  	return rc;
>  }
>
>  struct rpmPluginHooks_s ima_hooks = {
> -	.psm_post = ima_psm_post,
> +	.fsm_file_prepare = ima_fsm_file_prepare,
>  };
>

Anyway, those minor nits aside looks ok to me.

	- Panu -


More information about the Rpm-maint mailing list