[Rpm-maint] [PATCH v2 1/4] ima-plugin: Have executable configuration files signed

[Panu Matilainen]

On 09/22/2016 08:30 PM, Stefan Berger wrote:
> Some configuration files are executables and so they require the
> signature in the extended attribute. If they are not executable,
> they can be skipped.
>
> Examples for configuration files that are also executables are
> the grub files in /etc/grub.d.
>
> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
> ---
>  plugins/ima.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/plugins/ima.c b/plugins/ima.c
> index be15ecf..81ed194 100644
> --- a/plugins/ima.c
> +++ b/plugins/ima.c
> @@ -41,7 +41,8 @@ static rpmRC ima_psm_post(rpmPlugin plugin, rpmte te, int res)
>  	const char *fpath;
>  	const unsigned char * fsig = NULL;
>  	size_t len;
> -	int rc = 0;
> +	int rc = 0, n;
> +	struct stat statbuf;
>
>  	if (fi == NULL) {
>  	    rc = RPMERR_BAD_MAGIC;
> @@ -49,13 +50,21 @@ static rpmRC ima_psm_post(rpmPlugin plugin, rpmte te, int res)
>  	}
>
>  	while (rpmfiNext(fi) >= 0) {
> -	    /* Don't install signatures for (mutable) config files */
> -	    if (!(rpmfiFFlags(fi) & RPMFILE_CONFIG)) {
> -		fpath = rpmfiFN(fi);
> -		fsig = rpmfiFSignature(fi, &len);
> -		if (fsig && (check_zero_hdr(fsig, len) == 0)) {
> -		    lsetxattr(fpath, XATTR_NAME_IMA, fsig, len, 0);
> -		}
> +	    /* Don't install signatures for (mutable) files marked
> +	     * as config files unless they are also executable.
> +	     */
> +	    if (rpmfiFFlags(fi) & RPMFILE_CONFIG) {
> +	        n = rpmfiStat(fi, 0, &statbuf);
> +	        if (n != 0)
> +	            continue;
> +	        if ((statbuf.st_mode & 0111) == 0)
> +	            continue;

rpmfiStat() is not wrong, but it does far more than you need here and 
complicates things a little. You could just use rpmfiFMode() for the 
mode bits.

Also generally it's preferred to avoid magic numbers when it can be 
easily expressed with defined names, (S_IXUSR|S_IXGRP|S_IXOTH) is easier 
for the reader than 0111.

As for the change itself, I think it also points to
a) packaging bug in grub2-tools - except for those *_custom files these 
are *not* configuration files that you're supposed to touch
b) overall insanity of grub2 - inspect any of the files in /etc/grub.d, 
trying to keep in mind these are bootloader "configuration"...

All that aside, I wonder about this change: what will happen now if the 
user legitimately edits one of those executable %config files? The 
signature no longer match, does that mean it'll fail to execute?

	- Panu -