[SSSD] [sssd] user's write permission on sssd.conf

Pavel Reichl preichl at redhat.com
Wed Apr 23 09:25:27 UTC 2014


On Tue, 2014-04-22 at 14:55 -0400, Simo Sorce wrote:
> On Tue, 2014-04-22 at 19:00 +0200, Pavel Reichl wrote:
> > Hello, 
> > 
> > I tested the patches and they seem to me to be working flawlessly (so
> > ACK to both).
> > 
> > I just noticed little nitpick:
> > It was proposed:
> > 
> > > What we want to check here is that group/other do not have access.
> > > The check should be:
> > > if ((stated_mode & 0077) != 0) { /* wrong perms */ }
> > 
> > but instead it is required for user to have permission to read and
> > permissions to write and execute are optional. But I guess it doesn't
> > really matter.
> 
> Thank you for testing, and yeah I decided to check at least for write
> permission, I think it is better to respect a lack of read permission
> which normally the root account would override.
> 
> > I have attached 2 patches fixing little issues of logging messages
> > which I encountered during testing.
> 
> See comments inline.
> 
> > 
> > 
> > 
> > 
> > 
> > 
> > differences
> > between files
> > attachment
> > (0001-Monitor-fix-message-wrong-perm.-mode-on-config-file.patch)
> > 
> > From b2e9350391a99085440e1b8ff816f0c72e72d14c Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <reichl.pavel at gmail.com>
> > Date: Tue, 22 Apr 2014 14:40:05 +0100
> > Subject: [PATCH 1/2] Monitor: fix message wrong perm. mode on config
> > file
> > 
> > Add missing new line in message.
> > Add supporded access mode.
> > ---
> >  src/monitor/monitor.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> > index
> > 85655de75138ca35a18c09c961814191fd528e55..a7e6a8485debd27ed0d24b787f3f583a4487a333 100644
> > --- a/src/monitor/monitor.c
> > +++ b/src/monitor/monitor.c
> > @@ -86,8 +86,8 @@
> >  
> >  /* Warning messages */
> >  #define CONF_FILE_PERM_ERROR_MSG "Cannot read config file %s. Please
> > check "\
> > -                                 "if permissions are 0600 and the
> > file is "\
> > -                                 "owned by root.root."
> > +                                 "if permissions are 0600 or 0400 and
> > the "\
> > +                                 "file is owned by root.root.\n"
> 
> Maybe we should change this to be something like:
> "Please check that the file is accessible only by the owner and owned by
> the root user."
> 
> Because technically all these will match: 0700, 0600, 0500, 0400.
> Listing them all seem excessive.

OK, 1st patch address this.

> 
>  
> > 
> > 
> > 
> > 
> > 
> > 
> > differences
> > between files
> > attachment
> > (0002-util-Fix-file-mode-in-debug-message.patch)
> > 
> > From 186424590d617dd28ff28c8ca7d5dbb509d914a3 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <reichl.pavel at gmail.com>
> > Date: Tue, 22 Apr 2014 17:43:17 +0100
> > Subject: [PATCH 2/2] util: Fix file mode in debug message
> > 
> > ---
> >  src/util/check_and_open.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/util/check_and_open.c b/src/util/check_and_open.c
> > index
> > 80ec00e1023a05c3cf736b98ef27e61695766069..a6d0e09470027382991fe67888d366e82d282cf6 100644
> > --- a/src/util/check_and_open.c
> > +++ b/src/util/check_and_open.c
> > @@ -106,7 +106,7 @@ static errno_t perform_checks(struct stat
> > *stat_buf,
> >      if ((st_mode & ALLPERMS) != (mode & ALLPERMS)) {
> >          DEBUG(SSSDBG_CRIT_FAILURE,
> >                "File has the wrong mode [%.7o], expected [%.7o].\n",
> > -                  (st_mode & ALLPERMS), (mode & ALLPERMS));
> > +              (stat_buf->st_mode & ALLPERMS), (mode & ALLPERMS));
> >          return EINVAL;
> >      }
> >  
> 
> I am pretty sure st_mode & ALLPERMS is intentional, althugh I see it is
> debatable. If you want to show the original untouched mode I think you
> will also need then to print the mask, to make it clear what bits we
> really care for in mode.
> 
> Otherwise a file with st_mode of 0777 which we test against a mode of
> 0600 with a mask of 0622 (ie we care for it not being writable and we do
> not care if read or execute bits are set) would become confusing.
> 
> With my version we print:
> File has the wrong mode [0622], expected [0600]
> Yours would print:
> File has the wrong mode [0777], expected [0600]
> 
> So mine is not precise, but it tells you exactly what bits are out of
> place.
> Yours tells you what the file permissions actually are, but also tells
> you that you should chmod 600, when in reality chmod go-w would be
> sufficient.
> > 
> I think mine would give a better clue, but I see possible confusion both
> ways ...
> 
> Simo.
> 

Oh I see now, I believe your solution is better. 
2nd attached patch just rewords the message to give a clue about bit
mask being used. 

File has the wrong (bit masked) mode [0622], expected [0600]
                   ^^^^^^^^^^^  
But I'm not sure if it is not even more confusing. :-)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Monitor-fix-message-wrong-perm.-mode-on-config-file.patch
Type: text/x-patch
Size: 1084 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140423/adb5a85b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-util-Fix-wrong-mode-debug-message.patch
Type: text/x-patch
Size: 1029 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140423/adb5a85b/attachment-0001.bin>


More information about the sssd-devel mailing list