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

Simo Sorce simo at redhat.com
Tue Apr 22 18:55:05 UTC 2014


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.

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

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list