[SSSD] Only create the SELinux login file if there are mappings on the server

Jakub Hrozek jhrozek at redhat.com
Tue Aug 7 08:01:54 UTC 2012


On Mon, Aug 06, 2012 at 09:31:07AM -0400, Simo Sorce wrote:
> On Mon, 2012-08-06 at 10:51 +0200, Jakub Hrozek wrote:
> > [PATCH 1/2] Do not try to remove the temp login file if already
> > renamed
> > 
> > write_selinux_string() would try to unlink the temporary file even
> > after
> > it was renamed. Failure to unlink the file would not be fatal, but
> > would
> > produce a confusing error message.
> > 
> > Also don't use "0" for the default fd number, that's reserved for
> > stdin.
> > Using -1 is safer.
> > 
> > [PATCH 2/2] Only create the SELinux login file if there are mappings
> > on the server
> > 
> > https://fedorahosted.org/sssd/ticket/1455
> > 
> > In case there are no rules on the IPA server, we must simply avoid
> > generating
> > the login file. That would make us fall back to the system-wide
> > default
> > defined in /etc/selinux/targeted/seusers.
> > 
> > The IPA default must be only used if there *are* rules on the server,
> > but none matches.
> 
> Minor nitpicks, but I agree with the patches purposes, so auto-ACK when
> nitpicks are addressed.
> 
> 
> > From 87ec8c6d2bfeed404672ddca114cc8d0e1497346 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek <jhrozek at redhat.com>
> > Date: Sun, 5 Aug 2012 22:03:11 +0200
> > Subject: [PATCH 1/2] Do not try to remove the temp login file if
> > already
> >  renamed
> >  
> >  done:
> > -    if (fd > 0) {
> > +    if (fd > -1) {
> 
> use fd != -1 here, it makes it clear only -1 is a special value.

Done.

> 
> 
> > From 90bd88e09e185dbcdaec17b9a8a619a0fdc9589a Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek <jhrozek at redhat.com>
> > Date: Sun, 5 Aug 2012 22:37:09 +0200
> > Subject: [PATCH 2/2] Only create the SELinux login file if there are
> > mappings
> >  on the server
> 
>  
> > -static errno_t write_selinux_string(const char *username, char
> > *string)
> > +static char *selogin_path(TALLOC_CTX *mem_ctx, const char *username)
> > +{
> > +    return talloc_asprintf(mem_ctx, "%s/logins/%s",
> > selinux_policy_root(),
> > +                           username);
> > +}
> 
> I would use a macro here, a whole function for a oneliner seem
> excessive.

OK, I assumed the function would be inlined by the compiler but wanted to
keep how the login file path is constructed at one place, so that in case
it changed in the future, there's only one place to change.

> > +static errno_t process_selinux_mappings(struct pam_auth_req *preq)
> >  {
> >      struct sysdb_ctx *sysdb;
> >      TALLOC_CTX *tmp_ctx;
> > @@ -549,13 +574,16 @@ static errno_t get_selinux_string(struct
> > pam_auth_req *preq)
> >      }
> >  
> >      if (ret == ENOENT) {

I also moved this ENOENT handler together with handling of other errors
to get rid of one level of indentation. I think the function is more
compact and readable now.

> > -        DEBUG(SSSDBG_TRACE_FUNC, ("No user maps found, using
> > default!"));
> > +        DEBUG(SSSDBG_TRACE_FUNC, ("No maps defined on the server
> > \n"));
> > +    } else {
> > +        /* If no maps match, we'll use the default SELinux user from
> > the
> > +           config */
> >          file_content = talloc_strdup(tmp_ctx, default_user);
> >          if (file_content == NULL) {
> >              ret = ENOMEM;
> >              goto done;
> 
> This error condition will leave the existing selinux users file in
> place, shouldn't you remove it in the error path instead of later in the
> body of the function ?

Probably yes, I didn't do it earlier because removing the file triggers
an allocation, which is likely to fail, too, if you're handling an
ENOMEM situation.


> 
> >          }
> > -    } else {
> > +
> >          /* Iterate through the order array and try to find SELinux
> > users
> >           * in fetched maps. The order array contains all SELinux
> > users
> >           * allowed in the domain in the same order they should appear
> > @@ -589,7 +617,9 @@ static errno_t get_selinux_string(struct
> > pam_auth_req *preq)
> >      }
> >  
> >      if (file_content) {
> > -        ret = write_selinux_string(pd->user, file_content);
> > +        ret = write_selinux_login_file(pd->user, file_content);
> > +    } else {
> > +        ret = remove_selinux_login_file(pd->user);
> >      }
> 
> See above comment, shoudln;t this be done after the 'done' label ?

Done in 'done' now :-)

>  
> >  done:
> > @@ -796,7 +826,7 @@ static void pam_reply(struct pam_auth_req *preq)
> >          pd->pam_status == PAM_SUCCESS) {
> >          /* Try to fetch data from sysdb
> >           * (auth already passed -> we should have them) */
> > -        ret = get_selinux_string(preq);
> > +        ret = process_selinux_mappings(preq);
> >          if (ret != EOK) {
> >              pd->pam_status = PAM_SYSTEM_ERR;
> >          }

New patches are attached.
-------------- next part --------------
>From 5944291ab5ff4898f814def92af51472ef30659e Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sun, 5 Aug 2012 22:03:11 +0200
Subject: [PATCH 1/2] Do not try to remove the temp login file if already
 renamed

write_selinux_string() would try to unlink the temporary file even after
it was renamed. Failure to unlink the file would not be fatal, but would
produce a confusing error message.

Also don't use "0" for the default fd number, that's reserved for stdin.
Using -1 is safer.
---
 src/responder/pam/pamsrv_cmd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index 92cd734079be76f6c40873c00278f75d5947d686..6df8213f7fea3eef3f05fa71b55ad8b54da93181 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -362,7 +362,7 @@ static errno_t write_selinux_string(const char *username, char *string)
     char *tmp_path = NULL;
     ssize_t written;
     int len;
-    int fd = 0;
+    int fd = -1;
     mode_t oldmask;
     TALLOC_CTX *tmp_ctx;
     char *full_string = NULL;
@@ -433,9 +433,10 @@ static errno_t write_selinux_string(const char *username, char *string)
     } else {
         ret = EOK;
     }
+    fd = -1;
 
 done:
-    if (fd > 0) {
+    if (fd != -1) {
         close(fd);
         if (unlink(tmp_path) < 0) {
             DEBUG(SSSDBG_MINOR_FAILURE, ("Could not remove file [%s]",
-- 
1.7.11.2

-------------- next part --------------
>From b7e6197356cbd434610ca4b726abb95825962a3c Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sun, 5 Aug 2012 22:37:09 +0200
Subject: [PATCH 2/2] Only create the SELinux login file if there are mappings
 on the server

https://fedorahosted.org/sssd/ticket/1455

In case there are no rules on the IPA server, we must simply avoid generating
the login file. That would make us fall back to the system-wide default
defined in /etc/selinux/targeted/seusers.

The IPA default must be only used if there *are* rules on the server,
but none matches.
---
 src/db/sysdb_selinux.c         |   7 +--
 src/responder/pam/pamsrv_cmd.c | 114 +++++++++++++++++++++++++----------------
 2 files changed, 71 insertions(+), 50 deletions(-)

diff --git a/src/db/sysdb_selinux.c b/src/db/sysdb_selinux.c
index eaf07b50a4435b11f937473e37c708edc6e2e0eb..976489503e0995b7e025146c23a7f4e7874c1643 100644
--- a/src/db/sysdb_selinux.c
+++ b/src/db/sysdb_selinux.c
@@ -364,7 +364,7 @@ errno_t sysdb_search_selinux_usermap_by_username(TALLOC_CTX *mem_ctx,
     struct ldb_message **msgs = NULL;
     struct sysdb_attrs *user;
     struct sysdb_attrs *tmp_attrs;
-    struct ldb_message **usermaps;
+    struct ldb_message **usermaps = NULL;
     struct sss_domain_info *domain;
     struct ldb_dn *basedn;
     size_t msgs_count = 0;
@@ -462,11 +462,6 @@ errno_t sysdb_search_selinux_usermap_by_username(TALLOC_CTX *mem_ctx,
         }
     }
 
-    if (usermaps[0] == NULL) {
-        ret = ENOENT;
-        goto done;
-    }
-
     *_usermaps = talloc_steal(mem_ctx, usermaps);
 
     ret = EOK;
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index 6df8213f7fea3eef3f05fa71b55ad8b54da93181..d1a5c421953721ad78e55bc98910104d1eb4e95d 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -355,8 +355,10 @@ fail:
 }
 
 #define ALL_SERVICES "*"
+#define selogin_path(mem_ctx, username) \
+    talloc_asprintf(mem_ctx, "%s/logins/%s", selinux_policy_root(), username)
 
-static errno_t write_selinux_string(const char *username, char *string)
+static errno_t write_selinux_login_file(const char *username, char *string)
 {
     char *path = NULL;
     char *tmp_path = NULL;
@@ -379,8 +381,7 @@ static errno_t write_selinux_string(const char *username, char *string)
         return ENOMEM;
     }
 
-    path = talloc_asprintf(tmp_ctx, "%s/logins/%s", selinux_policy_root(),
-                           username);
+    path = selogin_path(tmp_ctx, username);
     if (path == NULL) {
         ret = ENOMEM;
         goto done;
@@ -448,7 +449,27 @@ done:
     return ret;
 }
 
-static errno_t get_selinux_string(struct pam_auth_req *preq)
+static errno_t remove_selinux_login_file(const char *username)
+{
+    char *path;
+    errno_t ret;
+
+    path = selogin_path(NULL, username);
+    if (!path) return ENOMEM;
+
+    errno = 0;
+    ret = unlink(path);
+    if (ret < 0) {
+        ret = errno;
+        DEBUG(SSSDBG_OP_FAILURE, ("Could not remove login file %s [%d]: %s\n",
+              path, ret, strerror(ret)));
+    }
+
+    talloc_free(path);
+    return ret;
+}
+
+static errno_t process_selinux_mappings(struct pam_auth_req *preq)
 {
     struct sysdb_ctx *sysdb;
     TALLOC_CTX *tmp_ctx;
@@ -460,7 +481,7 @@ static errno_t get_selinux_string(struct pam_auth_req *preq)
     const char *tmp_str;
     char *order = NULL;
     char **order_array;
-    errno_t ret;
+    errno_t ret, err;
     int i, j;
     size_t order_count;
     size_t len = 0;
@@ -546,53 +567,58 @@ static errno_t get_selinux_string(struct pam_auth_req *preq)
                                                    &usermaps);
     if (ret != EOK && ret != ENOENT) {
         goto done;
+    } else if (ret == ENOENT) {
+        DEBUG(SSSDBG_TRACE_FUNC, ("No maps defined on the server\n"));
+        ret = EOK;
+        goto done;
     }
 
-    if (ret == ENOENT) {
-        DEBUG(SSSDBG_TRACE_FUNC, ("No user maps found, using default!"));
-        file_content = talloc_strdup(tmp_ctx, default_user);
-        if (file_content == NULL) {
-            ret = ENOMEM;
-            goto done;
-        }
-    } else {
-        /* Iterate through the order array and try to find SELinux users
-         * in fetched maps. The order array contains all SELinux users
-         * allowed in the domain in the same order they should appear
-         * in the SELinux config file. If any user from the order array
-         * is not in fetched user maps, it means it should not be allowed
-         * for the user who is just logging in.
-         *
-         * Right now we have empty content of the SELinux config file,
-         * we shall add only those SELinux users that are present both in
-         * the order array and user maps applicable to the user who is
-         * logging in.
-         */
-        for (i = 0; i < order_count; i++) {
-            for (j = 0; usermaps[j] != NULL; j++) {
-                tmp_str = sss_selinux_map_get_seuser(usermaps[j]);
+    /* If no maps match, we'll use the default SELinux user from the
+     * config */
+    file_content = talloc_strdup(tmp_ctx, default_user);
+    if (file_content == NULL) {
+        ret = ENOMEM;
+        goto done;
+    }
+
+    /* Iterate through the order array and try to find SELinux users
+     * in fetched maps. The order array contains all SELinux users
+     * allowed in the domain in the same order they should appear
+     * in the SELinux config file. If any user from the order array
+     * is not in fetched user maps, it means it should not be allowed
+     * for the user who is just logging in.
+     *
+     * Right now we have empty content of the SELinux config file,
+     * we shall add only those SELinux users that are present both in
+     * the order array and user maps applicable to the user who is
+     * logging in.
+     */
+    for (i = 0; i < order_count; i++) {
+        for (j = 0; usermaps[j] != NULL; j++) {
+            tmp_str = sss_selinux_map_get_seuser(usermaps[j]);
 
-                if (tmp_str && !strcasecmp(tmp_str, order_array[i])) {
-                    /* If file_content contained something, overwrite it.
-                     * This record has higher priority.
-                     */
-                    talloc_zfree(file_content);
-                    file_content = talloc_strdup(tmp_ctx, tmp_str);
-                    if (file_content == NULL) {
-                        ret = ENOMEM;
-                        goto done;
-                    }
-                    break;
+            if (tmp_str && !strcasecmp(tmp_str, order_array[i])) {
+                /* If file_content contained something, overwrite it.
+                 * This record has higher priority.
+                 */
+                talloc_zfree(file_content);
+                file_content = talloc_strdup(tmp_ctx, tmp_str);
+                if (file_content == NULL) {
+                    ret = ENOMEM;
+                    goto done;
                 }
+                break;
             }
         }
     }
 
-    if (file_content) {
-        ret = write_selinux_string(pd->user, file_content);
-    }
-
+    ret = write_selinux_login_file(pd->user, file_content);
 done:
+    if (!file_content) {
+        err = remove_selinux_login_file(pd->user);
+        /* Don't overwrite original error condition if there was one */
+        if (ret == EOK) ret = err;
+    }
     talloc_free(tmp_ctx);
     return ret;
 }
@@ -796,7 +822,7 @@ static void pam_reply(struct pam_auth_req *preq)
         pd->pam_status == PAM_SUCCESS) {
         /* Try to fetch data from sysdb
          * (auth already passed -> we should have them) */
-        ret = get_selinux_string(preq);
+        ret = process_selinux_mappings(preq);
         if (ret != EOK) {
             pd->pam_status = PAM_SYSTEM_ERR;
         }
-- 
1.7.11.2



More information about the sssd-devel mailing list