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

[Thierry Vignaud]

On 23 September 2016 at 08:44, Panu Matilainen  wrote:
> 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.

That actually depends on the reader :-)
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

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

[Stefan Berger]

Panu Matilainen <pmati...@laiskiainen.org> wrote on 09/23/2016 02:44:48 
AM:

> From: Panu Matilainen <pmati...@laiskiainen.org>
> To: Stefan Berger <stef...@linux.vnet.ibm.com>, rpm-maint@lists.rpm.org
> Cc: Stefan Berger/Watson/IBM@IBMUS, fionnuala.gun...@gmail.com
> Date: 09/23/2016 02:45 AM
> Subject: Re: [Rpm-maint] [PATCH v2 1/4] ima-plugin: Have executable 
> configuration files signed
> 
> 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 <stef...@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, );
> > -  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, );
> > +   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.

Ok, I'll fix that.

> 
> 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?

The change is necessary. There are two ways to run these scripts:

bash 00_header  or  ./00_header

The latter needs the signature, while the former does (currently) not need 
it. Following tests with grub the scripts seem to be executed using the 
latter method and fail if they don't have a signature.

Once IMA Appraisal is active, you would have to regard the system as being 
in lock-down mode. If you make changes to these files, you'd have to sign 
them, otherwise they will fail executing.

   Stefan

> 
>- Panu -
> 


___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [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 
---
 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, );
-   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, );
+   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 -
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

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

[Stefan Berger]

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 
---
 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, );
-   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, );
+   if (n != 0)
+   continue;
+   if ((statbuf.st_mode & 0111) == 0)
+   continue;
+   }
+
+   fsig = rpmfiFSignature(fi, );
+   if (fsig && (check_zero_hdr(fsig, len) == 0)) {
+   fpath = rpmfiFN(fi);
+   lsetxattr(fpath, XATTR_NAME_IMA, fsig, len, 0);
}
}
 exit:
-- 
2.5.5

___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint