| Message ID | 20170214131448.22837-1-vmojzis@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, 2017-02-14 at 14:14 +0100, Vit Mojzis wrote: > Use faccessat() with AT_EACCESS instead of accesss() in order to > check > permissions of effective user. access() calls checking existence of > a file (F_OK) were left untouched since they work correctly. > > This enables setuid programs to use libsemanage. Not sure we want setuid programs using libsemanage. Is there a use case for that? I wouldn't warrant it to be safe. AT_EACCESS is not implemented by the kernel, so this seems to rely on some libc magic to support? Is that done in a safe way? libsemanage usage of access() is not for security purposes; it is just to test for existence/location of various files and to confirm that the policy store is writable by the caller before beginning any real work. So arguments about access() being insecure are not relevant here. > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 > > Signed-off-by: Vit Mojzis <vmojzis@redhat.com> > --- > libsemanage/src/conf-parse.y | 7 ++++--- > libsemanage/src/semanage_store.c | 18 +++++++++--------- > 2 files changed, 13 insertions(+), 12 deletions(-) > > diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf- > parse.y > index b527e89..d72a0c2 100644 > --- a/libsemanage/src/conf-parse.y > +++ b/libsemanage/src/conf-parse.y > @@ -30,6 +30,7 @@ > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > +#include <fcntl.h> > > extern int semanage_lex(void); /* defined in conf- > scan.c */ > extern int semanage_lex_destroy(void); /* defined in conf- > scan.c */ > @@ -361,7 +362,7 @@ static int semanage_conf_init(semanage_conf_t * > conf) > return -1; > } > > - if (access("/sbin/load_policy", X_OK) == 0) { > + if (faccessat(AT_FDCWD, "/sbin/load_policy", X_OK, > AT_EACCESS) == 0) { > conf->load_policy->path = > strdup("/sbin/load_policy"); > } else { > conf->load_policy->path = > strdup("/usr/sbin/load_policy"); > @@ -375,7 +376,7 @@ static int semanage_conf_init(semanage_conf_t * > conf) > calloc(1, sizeof(*(current_conf->setfiles)))) == NULL) > { > return -1; > } > - if (access("/sbin/setfiles", X_OK) == 0) { > + if (faccessat(AT_FDCWD, "/sbin/setfiles", X_OK, AT_EACCESS) > == 0) { > conf->setfiles->path = strdup("/sbin/setfiles"); > } else { > conf->setfiles->path = strdup("/usr/sbin/setfiles"); > @@ -389,7 +390,7 @@ static int semanage_conf_init(semanage_conf_t * > conf) > calloc(1, sizeof(*(current_conf->sefcontext_compile)))) > == NULL) { > return -1; > } > - if (access("/sbin/sefcontext_compile", X_OK) == 0) { > + if (faccessat(AT_FDCWD, "/sbin/sefcontext_compile", X_OK, > AT_EACCESS) == 0) { > conf->sefcontext_compile->path = > strdup("/sbin/sefcontext_compile"); > } else { > conf->sefcontext_compile->path = > strdup("/usr/sbin/sefcontext_compile"); > diff --git a/libsemanage/src/semanage_store.c > b/libsemanage/src/semanage_store.c > index f468fab..805bd60 100644 > --- a/libsemanage/src/semanage_store.c > +++ b/libsemanage/src/semanage_store.c > @@ -517,7 +517,7 @@ char *semanage_conf_path(void) > snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), > selinux_path(), > SEMANAGE_CONF_FILE); > > - if (access(semanage_conf, R_OK) != 0) { > + if (faccessat(AT_FDCWD, semanage_conf, R_OK, AT_EACCESS) != > 0) { > snprintf(semanage_conf, len + 1, "%s%s", > selinux_path(), SEMANAGE_CONF_FILE); > } > > @@ -552,7 +552,7 @@ int semanage_create_store(semanage_handle_t * sh, > int create) > return -1; > } > } else { > - if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) > == -1) { > + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, > path, mode_mask, AT_EACCESS) == -1) { > ERR(sh, > "Could not access module store at %s, or > it is not a directory.", > path); > @@ -575,7 +575,7 @@ int semanage_create_store(semanage_handle_t * sh, > int create) > return -1; > } > } else { > - if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) > == -1) { > + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, > path, mode_mask, AT_EACCESS) == -1) { > ERR(sh, > "Could not access module store active > subdirectory at %s, or it is not a directory.", > path); > @@ -598,7 +598,7 @@ int semanage_create_store(semanage_handle_t * sh, > int create) > return -1; > } > } else { > - if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) > == -1) { > + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, > path, mode_mask, AT_EACCESS) == -1) { > ERR(sh, > "Could not access module store active > modules subdirectory at %s, or it is not a directory.", > path); > @@ -619,7 +619,7 @@ int semanage_create_store(semanage_handle_t * sh, > int create) > return -1; > } > } else { > - if (!S_ISREG(sb.st_mode) || access(path, R_OK | > W_OK) == -1) { > + if (!S_ISREG(sb.st_mode) || faccessat(AT_FDCWD, > path, R_OK | W_OK, AT_EACCESS) == -1) { > ERR(sh, "Could not access lock file at %s.", > path); > return -1; > } > @@ -639,7 +639,7 @@ int semanage_store_access_check(void) > > /* read access on active store */ > path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_TOPLEVEL); > - if (access(path, R_OK | X_OK) != 0) > + if (faccessat(AT_FDCWD, path, R_OK | X_OK, AT_EACCESS) != 0) > goto out; > > /* we can read the active store meaning it is managed > @@ -650,13 +650,13 @@ int semanage_store_access_check(void) > * write access necessary if the lock file does not exist > */ > path = semanage_files[SEMANAGE_READ_LOCK]; > - if (access(path, R_OK) != 0) { > + if (faccessat(AT_FDCWD, path, R_OK, AT_EACCESS) != 0) { > if (access(path, F_OK) == 0) { > goto out; > } > > path = semanage_files[SEMANAGE_ROOT]; > - if (access(path, R_OK | W_OK | X_OK) != 0) { > + if (faccessat(AT_FDCWD, path, R_OK | W_OK | X_OK, > AT_EACCESS) != 0) { > goto out; > } > } > @@ -666,7 +666,7 @@ int semanage_store_access_check(void) > > /* check the modules directory */ > path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_MODULES); > - if (access(path, R_OK | W_OK | X_OK) != 0) > + if (faccessat(AT_FDCWD, path, R_OK | W_OK | X_OK, > AT_EACCESS) != 0) > goto out; > > rc = SEMANAGE_CAN_WRITE;
On Tue, 2017-02-14 at 10:11 -0500, Stephen Smalley wrote: > On Tue, 2017-02-14 at 14:14 +0100, Vit Mojzis wrote: > > > > Use faccessat() with AT_EACCESS instead of accesss() in order to > > check > > permissions of effective user. access() calls checking existence of > > a file (F_OK) were left untouched since they work correctly. > > > > This enables setuid programs to use libsemanage. > > Not sure we want setuid programs using libsemanage. Is there a use > case for that? I wouldn't warrant it to be safe. > > AT_EACCESS is not implemented by the kernel, so this seems to rely on > some libc magic to support? Is that done in a safe way? Per the man page glibc notes, AT_EACCESS is implemented by the glibc wrapper function by using fstatat(2) and emulating the permission check in userspace. This however is not equivalent to access(2), since the latter will perform SELinux checks too. It is also seemingly glibc- specific and thus non-portable. > > libsemanage usage of access() is not for security purposes; it is > just > to test for existence/location of various files and to confirm that > the > policy store is writable by the caller before beginning any real > work. > So arguments about access() being insecure are not relevant here. > > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 > > > > Signed-off-by: Vit Mojzis <vmojzis@redhat.com> > > --- > > libsemanage/src/conf-parse.y | 7 ++++--- > > libsemanage/src/semanage_store.c | 18 +++++++++--------- > > 2 files changed, 13 insertions(+), 12 deletions(-) > > > > diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf- > > parse.y > > index b527e89..d72a0c2 100644 > > --- a/libsemanage/src/conf-parse.y > > +++ b/libsemanage/src/conf-parse.y > > @@ -30,6 +30,7 @@ > > #include <stdio.h> > > #include <stdlib.h> > > #include <string.h> > > +#include <fcntl.h> > > > > extern int semanage_lex(void); /* defined in conf- > > scan.c */ > > extern int semanage_lex_destroy(void); /* defined in conf- > > scan.c */ > > @@ -361,7 +362,7 @@ static int semanage_conf_init(semanage_conf_t * > > conf) > > return -1; > > } > > > > - if (access("/sbin/load_policy", X_OK) == 0) { > > + if (faccessat(AT_FDCWD, "/sbin/load_policy", X_OK, > > AT_EACCESS) == 0) { > > conf->load_policy->path = > > strdup("/sbin/load_policy"); > > } else { > > conf->load_policy->path = > > strdup("/usr/sbin/load_policy"); > > @@ -375,7 +376,7 @@ static int semanage_conf_init(semanage_conf_t * > > conf) > > calloc(1, sizeof(*(current_conf->setfiles)))) == > > NULL) > > { > > return -1; > > } > > - if (access("/sbin/setfiles", X_OK) == 0) { > > + if (faccessat(AT_FDCWD, "/sbin/setfiles", X_OK, > > AT_EACCESS) > > == 0) { > > conf->setfiles->path = strdup("/sbin/setfiles"); > > } else { > > conf->setfiles->path = > > strdup("/usr/sbin/setfiles"); > > @@ -389,7 +390,7 @@ static int semanage_conf_init(semanage_conf_t * > > conf) > > calloc(1, sizeof(*(current_conf- > > >sefcontext_compile)))) > > == NULL) { > > return -1; > > } > > - if (access("/sbin/sefcontext_compile", X_OK) == 0) { > > + if (faccessat(AT_FDCWD, "/sbin/sefcontext_compile", X_OK, > > AT_EACCESS) == 0) { > > conf->sefcontext_compile->path = > > strdup("/sbin/sefcontext_compile"); > > } else { > > conf->sefcontext_compile->path = > > strdup("/usr/sbin/sefcontext_compile"); > > diff --git a/libsemanage/src/semanage_store.c > > b/libsemanage/src/semanage_store.c > > index f468fab..805bd60 100644 > > --- a/libsemanage/src/semanage_store.c > > +++ b/libsemanage/src/semanage_store.c > > @@ -517,7 +517,7 @@ char *semanage_conf_path(void) > > snprintf(semanage_conf, len + 1, "%s%s%s", > > semanage_root(), > > selinux_path(), > > SEMANAGE_CONF_FILE); > > > > - if (access(semanage_conf, R_OK) != 0) { > > + if (faccessat(AT_FDCWD, semanage_conf, R_OK, AT_EACCESS) > > != > > 0) { > > snprintf(semanage_conf, len + 1, "%s%s", > > selinux_path(), SEMANAGE_CONF_FILE); > > } > > > > @@ -552,7 +552,7 @@ int semanage_create_store(semanage_handle_t * > > sh, > > int create) > > return -1; > > } > > } else { > > - if (!S_ISDIR(sb.st_mode) || access(path, > > mode_mask) > > == -1) { > > + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, > > path, mode_mask, AT_EACCESS) == -1) { > > ERR(sh, > > "Could not access module store at %s, > > or > > it is not a directory.", > > path); > > @@ -575,7 +575,7 @@ int semanage_create_store(semanage_handle_t * > > sh, > > int create) > > return -1; > > } > > } else { > > - if (!S_ISDIR(sb.st_mode) || access(path, > > mode_mask) > > == -1) { > > + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, > > path, mode_mask, AT_EACCESS) == -1) { > > ERR(sh, > > "Could not access module store active > > subdirectory at %s, or it is not a directory.", > > path); > > @@ -598,7 +598,7 @@ int semanage_create_store(semanage_handle_t * > > sh, > > int create) > > return -1; > > } > > } else { > > - if (!S_ISDIR(sb.st_mode) || access(path, > > mode_mask) > > == -1) { > > + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, > > path, mode_mask, AT_EACCESS) == -1) { > > ERR(sh, > > "Could not access module store active > > modules subdirectory at %s, or it is not a directory.", > > path); > > @@ -619,7 +619,7 @@ int semanage_create_store(semanage_handle_t * > > sh, > > int create) > > return -1; > > } > > } else { > > - if (!S_ISREG(sb.st_mode) || access(path, R_OK | > > W_OK) == -1) { > > + if (!S_ISREG(sb.st_mode) || faccessat(AT_FDCWD, > > path, R_OK | W_OK, AT_EACCESS) == -1) { > > ERR(sh, "Could not access lock file at > > %s.", > > path); > > return -1; > > } > > @@ -639,7 +639,7 @@ int semanage_store_access_check(void) > > > > /* read access on active store */ > > path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_TOPLEVEL); > > - if (access(path, R_OK | X_OK) != 0) > > + if (faccessat(AT_FDCWD, path, R_OK | X_OK, AT_EACCESS) != > > 0) > > goto out; > > > > /* we can read the active store meaning it is managed > > @@ -650,13 +650,13 @@ int semanage_store_access_check(void) > > * write access necessary if the lock file does not exist > > */ > > path = semanage_files[SEMANAGE_READ_LOCK]; > > - if (access(path, R_OK) != 0) { > > + if (faccessat(AT_FDCWD, path, R_OK, AT_EACCESS) != 0) { > > if (access(path, F_OK) == 0) { > > goto out; > > } > > > > path = semanage_files[SEMANAGE_ROOT]; > > - if (access(path, R_OK | W_OK | X_OK) != 0) { > > + if (faccessat(AT_FDCWD, path, R_OK | W_OK | X_OK, > > AT_EACCESS) != 0) { > > goto out; > > } > > } > > @@ -666,7 +666,7 @@ int semanage_store_access_check(void) > > > > /* check the modules directory */ > > path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_MODULES); > > - if (access(path, R_OK | W_OK | X_OK) != 0) > > + if (faccessat(AT_FDCWD, path, R_OK | W_OK | X_OK, > > AT_EACCESS) != 0) > > goto out; > > > > rc = SEMANAGE_CAN_WRITE; > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho > .nsa.gov.
On Tue, 2017-02-14 at 10:25 -0500, Stephen Smalley wrote: > On Tue, 2017-02-14 at 10:11 -0500, Stephen Smalley wrote: > > > > On Tue, 2017-02-14 at 14:14 +0100, Vit Mojzis wrote: > > > > > > > > > Use faccessat() with AT_EACCESS instead of accesss() in order to > > > check > > > permissions of effective user. access() calls checking existence > > > of > > > a file (F_OK) were left untouched since they work correctly. > > > > > > This enables setuid programs to use libsemanage. > > > > Not sure we want setuid programs using libsemanage. Is there a use > > case for that? I wouldn't warrant it to be safe. > > > > AT_EACCESS is not implemented by the kernel, so this seems to rely > > on > > some libc magic to support? Is that done in a safe way? > > Per the man page glibc notes, AT_EACCESS is implemented by the glibc > wrapper function by using fstatat(2) and emulating the permission > check > in userspace. This however is not equivalent to access(2), since the > latter will perform SELinux checks too. It is also seemingly glibc- > specific and thus non-portable. Looked at the glibc implementation and it seems very broken; it hardcodes legacy Unix permission checking without consideration of Linux capabilities, POSIX ACLs, or anything else including SELinux permissions. Someone really ought to consider removing that. musl implementation is perhaps more reliable but has to use a complex workaround (clones a child that call setuid(), setgid() and then faccessat() under the new uid/gid and reports the status to the parent via a pipe). > > > > > > > libsemanage usage of access() is not for security purposes; it is > > just > > to test for existence/location of various files and to confirm that > > the > > policy store is writable by the caller before beginning any real > > work. > > So arguments about access() being insecure are not relevant here. > > > > > > > > > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 > > > > > > Signed-off-by: Vit Mojzis <vmojzis@redhat.com> > > > --- > > > libsemanage/src/conf-parse.y | 7 ++++--- > > > libsemanage/src/semanage_store.c | 18 +++++++++--------- > > > 2 files changed, 13 insertions(+), 12 deletions(-) > > > > > > diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf- > > > parse.y > > > index b527e89..d72a0c2 100644 > > > --- a/libsemanage/src/conf-parse.y > > > +++ b/libsemanage/src/conf-parse.y > > > @@ -30,6 +30,7 @@ > > > #include <stdio.h> > > > #include <stdlib.h> > > > #include <string.h> > > > +#include <fcntl.h> > > > > > > extern int semanage_lex(void); /* defined in > > > conf- > > > scan.c */ > > > extern int semanage_lex_destroy(void); /* defined in > > > conf- > > > scan.c */ > > > @@ -361,7 +362,7 @@ static int semanage_conf_init(semanage_conf_t > > > * > > > conf) > > > return -1; > > > } > > > > > > - if (access("/sbin/load_policy", X_OK) == 0) { > > > + if (faccessat(AT_FDCWD, "/sbin/load_policy", X_OK, > > > AT_EACCESS) == 0) { > > > conf->load_policy->path = > > > strdup("/sbin/load_policy"); > > > } else { > > > conf->load_policy->path = > > > strdup("/usr/sbin/load_policy"); > > > @@ -375,7 +376,7 @@ static int semanage_conf_init(semanage_conf_t > > > * > > > conf) > > > calloc(1, sizeof(*(current_conf->setfiles)))) == > > > NULL) > > > { > > > return -1; > > > } > > > - if (access("/sbin/setfiles", X_OK) == 0) { > > > + if (faccessat(AT_FDCWD, "/sbin/setfiles", X_OK, > > > AT_EACCESS) > > > == 0) { > > > conf->setfiles->path = strdup("/sbin/setfiles"); > > > } else { > > > conf->setfiles->path = > > > strdup("/usr/sbin/setfiles"); > > > @@ -389,7 +390,7 @@ static int semanage_conf_init(semanage_conf_t > > > * > > > conf) > > > calloc(1, sizeof(*(current_conf- > > > > > > > > sefcontext_compile)))) > > > == NULL) { > > > return -1; > > > } > > > - if (access("/sbin/sefcontext_compile", X_OK) == 0) { > > > + if (faccessat(AT_FDCWD, "/sbin/sefcontext_compile", > > > X_OK, > > > AT_EACCESS) == 0) { > > > conf->sefcontext_compile->path = > > > strdup("/sbin/sefcontext_compile"); > > > } else { > > > conf->sefcontext_compile->path = > > > strdup("/usr/sbin/sefcontext_compile"); > > > diff --git a/libsemanage/src/semanage_store.c > > > b/libsemanage/src/semanage_store.c > > > index f468fab..805bd60 100644 > > > --- a/libsemanage/src/semanage_store.c > > > +++ b/libsemanage/src/semanage_store.c > > > @@ -517,7 +517,7 @@ char *semanage_conf_path(void) > > > snprintf(semanage_conf, len + 1, "%s%s%s", > > > semanage_root(), > > > selinux_path(), > > > SEMANAGE_CONF_FILE); > > > > > > - if (access(semanage_conf, R_OK) != 0) { > > > + if (faccessat(AT_FDCWD, semanage_conf, R_OK, AT_EACCESS) > > > != > > > 0) { > > > snprintf(semanage_conf, len + 1, "%s%s", > > > selinux_path(), SEMANAGE_CONF_FILE); > > > } > > > > > > @@ -552,7 +552,7 @@ int semanage_create_store(semanage_handle_t * > > > sh, > > > int create) > > > return -1; > > > } > > > } else { > > > - if (!S_ISDIR(sb.st_mode) || access(path, > > > mode_mask) > > > == -1) { > > > + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, > > > path, mode_mask, AT_EACCESS) == -1) { > > > ERR(sh, > > > "Could not access module store at > > > %s, > > > or > > > it is not a directory.", > > > path); > > > @@ -575,7 +575,7 @@ int semanage_create_store(semanage_handle_t * > > > sh, > > > int create) > > > return -1; > > > } > > > } else { > > > - if (!S_ISDIR(sb.st_mode) || access(path, > > > mode_mask) > > > == -1) { > > > + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, > > > path, mode_mask, AT_EACCESS) == -1) { > > > ERR(sh, > > > "Could not access module store > > > active > > > subdirectory at %s, or it is not a directory.", > > > path); > > > @@ -598,7 +598,7 @@ int semanage_create_store(semanage_handle_t * > > > sh, > > > int create) > > > return -1; > > > } > > > } else { > > > - if (!S_ISDIR(sb.st_mode) || access(path, > > > mode_mask) > > > == -1) { > > > + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, > > > path, mode_mask, AT_EACCESS) == -1) { > > > ERR(sh, > > > "Could not access module store > > > active > > > modules subdirectory at %s, or it is not a directory.", > > > path); > > > @@ -619,7 +619,7 @@ int semanage_create_store(semanage_handle_t * > > > sh, > > > int create) > > > return -1; > > > } > > > } else { > > > - if (!S_ISREG(sb.st_mode) || access(path, R_OK | > > > W_OK) == -1) { > > > + if (!S_ISREG(sb.st_mode) || faccessat(AT_FDCWD, > > > path, R_OK | W_OK, AT_EACCESS) == -1) { > > > ERR(sh, "Could not access lock file at > > > %s.", > > > path); > > > return -1; > > > } > > > @@ -639,7 +639,7 @@ int semanage_store_access_check(void) > > > > > > /* read access on active store */ > > > path = semanage_path(SEMANAGE_ACTIVE, > > > SEMANAGE_TOPLEVEL); > > > - if (access(path, R_OK | X_OK) != 0) > > > + if (faccessat(AT_FDCWD, path, R_OK | X_OK, AT_EACCESS) > > > != > > > 0) > > > goto out; > > > > > > /* we can read the active store meaning it is managed > > > @@ -650,13 +650,13 @@ int semanage_store_access_check(void) > > > * write access necessary if the lock file does not > > > exist > > > */ > > > path = semanage_files[SEMANAGE_READ_LOCK]; > > > - if (access(path, R_OK) != 0) { > > > + if (faccessat(AT_FDCWD, path, R_OK, AT_EACCESS) != 0) { > > > if (access(path, F_OK) == 0) { > > > goto out; > > > } > > > > > > path = semanage_files[SEMANAGE_ROOT]; > > > - if (access(path, R_OK | W_OK | X_OK) != 0) { > > > + if (faccessat(AT_FDCWD, path, R_OK | W_OK | > > > X_OK, > > > AT_EACCESS) != 0) { > > > goto out; > > > } > > > } > > > @@ -666,7 +666,7 @@ int semanage_store_access_check(void) > > > > > > /* check the modules directory */ > > > path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_MODULES); > > > - if (access(path, R_OK | W_OK | X_OK) != 0) > > > + if (faccessat(AT_FDCWD, path, R_OK | W_OK | X_OK, > > > AT_EACCESS) != 0) > > > goto out; > > > > > > rc = SEMANAGE_CAN_WRITE; > > _______________________________________________ > > Selinux mailing list > > Selinux@tycho.nsa.gov > > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > > To get help, send an email containing "help" to Selinux-request@tyc > > ho > > .nsa.gov. > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho > .nsa.gov.
On Tue, 2017-02-14 at 15:17 -0500, Stephen Smalley wrote: > On Tue, 2017-02-14 at 10:25 -0500, Stephen Smalley wrote: > > > > On Tue, 2017-02-14 at 10:11 -0500, Stephen Smalley wrote: > > > > > > > > > On Tue, 2017-02-14 at 14:14 +0100, Vit Mojzis wrote: > > > > > > > > > > > > > > > > Use faccessat() with AT_EACCESS instead of accesss() in order > > > > to > > > > check > > > > permissions of effective user. access() calls checking > > > > existence > > > > of > > > > a file (F_OK) were left untouched since they work correctly. > > > > > > > > This enables setuid programs to use libsemanage. > > > > > > Not sure we want setuid programs using libsemanage. Is there a > > > use > > > case for that? I wouldn't warrant it to be safe. > > > > > > AT_EACCESS is not implemented by the kernel, so this seems to > > > rely > > > on > > > some libc magic to support? Is that done in a safe way? > > > > Per the man page glibc notes, AT_EACCESS is implemented by the > > glibc > > wrapper function by using fstatat(2) and emulating the permission > > check > > in userspace. This however is not equivalent to access(2), since > > the > > latter will perform SELinux checks too. It is also seemingly > > glibc- > > specific and thus non-portable. > > Looked at the glibc implementation and it seems very broken; it > hardcodes legacy Unix permission checking without consideration of > Linux capabilities, POSIX ACLs, or anything else including SELinux > permissions. Someone really ought to consider removing that. > > musl implementation is perhaps more reliable but has to use a complex > workaround (clones a child that call setuid(), setgid() and then > faccessat() under the new uid/gid and reports the status to the > parent > via a pipe). This seems to be a known issue, https://bugzilla.redhat.com/show_bug.cgi?id=1333764
On 02/14/2017 04:11 PM, Stephen Smalley wrote: > On Tue, 2017-02-14 at 14:14 +0100, Vit Mojzis wrote: >> Use faccessat() with AT_EACCESS instead of accesss() in order to >> check >> permissions of effective user. access() calls checking existence of >> a file (F_OK) were left untouched since they work correctly. >> >> This enables setuid programs to use libsemanage. > > Not sure we want setuid programs using libsemanage. Is there a use > case for that? I wouldn't warrant it to be safe. The motivation is described in the response from the original reporter in the bug: --- I think the reason why I filed the bug back then was https://fedorahosted.org/sssd/ticket/2564 In general I don't think the bug is a big deal to us and if upstream is reluctant to this change, just close the bug. I just found it odd to check if a file exists before acting on it instead of just trying to work with the file and failing on errors..the current approach seems a bit racy to me. About the question in the thread that asks why do we use the selinux libraries in a setuid library..the reason is that in order to pass certain certifications, no code in SSSD that deals with network connections should run as root. Therefore, the SSSD itself runs as a nonprivileged user and for actions that require root privileges (like setting a selinux context for a user) we fork our a setgid helper that actually does the work. --- I think it's about situations when a process can create e.g. /var/lib/selinux/targeted/semanage.read.LOCK001 using fopen(), but libsemanage denies to work as it thinks it can't access the store. Petr > > AT_EACCESS is not implemented by the kernel, so this seems to rely on > some libc magic to support? Is that done in a safe way? > > libsemanage usage of access() is not for security purposes; it is just > to test for existence/location of various files and to confirm that the > policy store is writable by the caller before beginning any real work. > So arguments about access() being insecure are not relevant here. > >> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 >> >> Signed-off-by: Vit Mojzis <vmojzis@redhat.com> >> --- >> libsemanage/src/conf-parse.y | 7 ++++--- >> libsemanage/src/semanage_store.c | 18 +++++++++--------- >> 2 files changed, 13 insertions(+), 12 deletions(-) >> >> diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf- >> parse.y >> index b527e89..d72a0c2 100644 >> --- a/libsemanage/src/conf-parse.y >> +++ b/libsemanage/src/conf-parse.y >> @@ -30,6 +30,7 @@ >> #include <stdio.h> >> #include <stdlib.h> >> #include <string.h> >> +#include <fcntl.h> >> >> extern int semanage_lex(void); /* defined in conf- >> scan.c */ >> extern int semanage_lex_destroy(void); /* defined in conf- >> scan.c */ >> @@ -361,7 +362,7 @@ static int semanage_conf_init(semanage_conf_t * >> conf) >> return -1; >> } >> >> - if (access("/sbin/load_policy", X_OK) == 0) { >> + if (faccessat(AT_FDCWD, "/sbin/load_policy", X_OK, >> AT_EACCESS) == 0) { >> conf->load_policy->path = >> strdup("/sbin/load_policy"); >> } else { >> conf->load_policy->path = >> strdup("/usr/sbin/load_policy"); >> @@ -375,7 +376,7 @@ static int semanage_conf_init(semanage_conf_t * >> conf) >> calloc(1, sizeof(*(current_conf->setfiles)))) == NULL) >> { >> return -1; >> } >> - if (access("/sbin/setfiles", X_OK) == 0) { >> + if (faccessat(AT_FDCWD, "/sbin/setfiles", X_OK, AT_EACCESS) >> == 0) { >> conf->setfiles->path = strdup("/sbin/setfiles"); >> } else { >> conf->setfiles->path = strdup("/usr/sbin/setfiles"); >> @@ -389,7 +390,7 @@ static int semanage_conf_init(semanage_conf_t * >> conf) >> calloc(1, sizeof(*(current_conf->sefcontext_compile)))) >> == NULL) { >> return -1; >> } >> - if (access("/sbin/sefcontext_compile", X_OK) == 0) { >> + if (faccessat(AT_FDCWD, "/sbin/sefcontext_compile", X_OK, >> AT_EACCESS) == 0) { >> conf->sefcontext_compile->path = >> strdup("/sbin/sefcontext_compile"); >> } else { >> conf->sefcontext_compile->path = >> strdup("/usr/sbin/sefcontext_compile"); >> diff --git a/libsemanage/src/semanage_store.c >> b/libsemanage/src/semanage_store.c >> index f468fab..805bd60 100644 >> --- a/libsemanage/src/semanage_store.c >> +++ b/libsemanage/src/semanage_store.c >> @@ -517,7 +517,7 @@ char *semanage_conf_path(void) >> snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), >> selinux_path(), >> SEMANAGE_CONF_FILE); >> >> - if (access(semanage_conf, R_OK) != 0) { >> + if (faccessat(AT_FDCWD, semanage_conf, R_OK, AT_EACCESS) != >> 0) { >> snprintf(semanage_conf, len + 1, "%s%s", >> selinux_path(), SEMANAGE_CONF_FILE); >> } >> >> @@ -552,7 +552,7 @@ int semanage_create_store(semanage_handle_t * sh, >> int create) >> return -1; >> } >> } else { >> - if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) >> == -1) { >> + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, >> path, mode_mask, AT_EACCESS) == -1) { >> ERR(sh, >> "Could not access module store at %s, or >> it is not a directory.", >> path); >> @@ -575,7 +575,7 @@ int semanage_create_store(semanage_handle_t * sh, >> int create) >> return -1; >> } >> } else { >> - if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) >> == -1) { >> + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, >> path, mode_mask, AT_EACCESS) == -1) { >> ERR(sh, >> "Could not access module store active >> subdirectory at %s, or it is not a directory.", >> path); >> @@ -598,7 +598,7 @@ int semanage_create_store(semanage_handle_t * sh, >> int create) >> return -1; >> } >> } else { >> - if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) >> == -1) { >> + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, >> path, mode_mask, AT_EACCESS) == -1) { >> ERR(sh, >> "Could not access module store active >> modules subdirectory at %s, or it is not a directory.", >> path); >> @@ -619,7 +619,7 @@ int semanage_create_store(semanage_handle_t * sh, >> int create) >> return -1; >> } >> } else { >> - if (!S_ISREG(sb.st_mode) || access(path, R_OK | >> W_OK) == -1) { >> + if (!S_ISREG(sb.st_mode) || faccessat(AT_FDCWD, >> path, R_OK | W_OK, AT_EACCESS) == -1) { >> ERR(sh, "Could not access lock file at %s.", >> path); >> return -1; >> } >> @@ -639,7 +639,7 @@ int semanage_store_access_check(void) >> >> /* read access on active store */ >> path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_TOPLEVEL); >> - if (access(path, R_OK | X_OK) != 0) >> + if (faccessat(AT_FDCWD, path, R_OK | X_OK, AT_EACCESS) != 0) >> goto out; >> >> /* we can read the active store meaning it is managed >> @@ -650,13 +650,13 @@ int semanage_store_access_check(void) >> * write access necessary if the lock file does not exist >> */ >> path = semanage_files[SEMANAGE_READ_LOCK]; >> - if (access(path, R_OK) != 0) { >> + if (faccessat(AT_FDCWD, path, R_OK, AT_EACCESS) != 0) { >> if (access(path, F_OK) == 0) { >> goto out; >> } >> >> path = semanage_files[SEMANAGE_ROOT]; >> - if (access(path, R_OK | W_OK | X_OK) != 0) { >> + if (faccessat(AT_FDCWD, path, R_OK | W_OK | X_OK, >> AT_EACCESS) != 0) { >> goto out; >> } >> } >> @@ -666,7 +666,7 @@ int semanage_store_access_check(void) >> >> /* check the modules directory */ >> path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_MODULES); >> - if (access(path, R_OK | W_OK | X_OK) != 0) >> + if (faccessat(AT_FDCWD, path, R_OK | W_OK | X_OK, >> AT_EACCESS) != 0) >> goto out; >> >> rc = SEMANAGE_CAN_WRITE; > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. >
On Wed, 2017-02-22 at 16:47 +0100, Petr Lautrbach wrote: > On 02/14/2017 04:11 PM, Stephen Smalley wrote: > > > > On Tue, 2017-02-14 at 14:14 +0100, Vit Mojzis wrote: > > > > > > Use faccessat() with AT_EACCESS instead of accesss() in order to > > > check > > > permissions of effective user. access() calls checking existence > > > of > > > a file (F_OK) were left untouched since they work correctly. > > > > > > This enables setuid programs to use libsemanage. > > > > Not sure we want setuid programs using libsemanage. Is there a use > > case for that? I wouldn't warrant it to be safe. > > The motivation is described in the response from the original > reporter > in the bug: > > --- > I think the reason why I filed the bug back then was > https://fedorahosted.org/sssd/ticket/2564 > > In general I don't think the bug is a big deal to us and if upstream > is > reluctant to this change, just close the bug. I just found it odd to > check if a file exists before acting on it instead of just trying to > work with the file and failing on errors..the current approach seems > a > bit racy to me. > > About the question in the thread that asks why do we use the selinux > libraries in a setuid library..the reason is that in order to pass > certain certifications, no code in SSSD that deals with network > connections should run as root. Therefore, the SSSD itself runs as a > nonprivileged user and for actions that require root privileges (like > setting a selinux context for a user) we fork our a setgid helper > that > actually does the work. > --- > > I think it's about situations when a process can create e.g. > /var/lib/selinux/targeted/semanage.read.LOCK001 using fopen(), but > libsemanage denies to work as it thinks it can't access the store. So then I would suggest looking at removing the use of access() checks altogether, aside from trivial F_OK and X_OK tests that should be harmless.
access() uses real UID instead of effective UID which causes false negative checks in setuid programs. Following patches remove redundant access checks (where the access check was followed by open, write,etc. call and the return value is checked), and replace necessary "access(, F_OK)" checks by "stats()" (e.g. in case where existence of a file determines if hll module compilation is necessary, or represents some setting - such as preserve_tunables). RHBZ #1186431 libsemanage/src/direct_api.c | 79 ++++++++++++++++++++++++++++++++++++------------------------------------------- libsemanage/src/semanage_store.c | 17 ++++++++--------- 2 files changed, 44 insertions(+), 52 deletions(-)
diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf-parse.y index b527e89..d72a0c2 100644 --- a/libsemanage/src/conf-parse.y +++ b/libsemanage/src/conf-parse.y @@ -30,6 +30,7 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <fcntl.h> extern int semanage_lex(void); /* defined in conf-scan.c */ extern int semanage_lex_destroy(void); /* defined in conf-scan.c */ @@ -361,7 +362,7 @@ static int semanage_conf_init(semanage_conf_t * conf) return -1; } - if (access("/sbin/load_policy", X_OK) == 0) { + if (faccessat(AT_FDCWD, "/sbin/load_policy", X_OK, AT_EACCESS) == 0) { conf->load_policy->path = strdup("/sbin/load_policy"); } else { conf->load_policy->path = strdup("/usr/sbin/load_policy"); @@ -375,7 +376,7 @@ static int semanage_conf_init(semanage_conf_t * conf) calloc(1, sizeof(*(current_conf->setfiles)))) == NULL) { return -1; } - if (access("/sbin/setfiles", X_OK) == 0) { + if (faccessat(AT_FDCWD, "/sbin/setfiles", X_OK, AT_EACCESS) == 0) { conf->setfiles->path = strdup("/sbin/setfiles"); } else { conf->setfiles->path = strdup("/usr/sbin/setfiles"); @@ -389,7 +390,7 @@ static int semanage_conf_init(semanage_conf_t * conf) calloc(1, sizeof(*(current_conf->sefcontext_compile)))) == NULL) { return -1; } - if (access("/sbin/sefcontext_compile", X_OK) == 0) { + if (faccessat(AT_FDCWD, "/sbin/sefcontext_compile", X_OK, AT_EACCESS) == 0) { conf->sefcontext_compile->path = strdup("/sbin/sefcontext_compile"); } else { conf->sefcontext_compile->path = strdup("/usr/sbin/sefcontext_compile"); diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c index f468fab..805bd60 100644 --- a/libsemanage/src/semanage_store.c +++ b/libsemanage/src/semanage_store.c @@ -517,7 +517,7 @@ char *semanage_conf_path(void) snprintf(semanage_conf, len + 1, "%s%s%s", semanage_root(), selinux_path(), SEMANAGE_CONF_FILE); - if (access(semanage_conf, R_OK) != 0) { + if (faccessat(AT_FDCWD, semanage_conf, R_OK, AT_EACCESS) != 0) { snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE); } @@ -552,7 +552,7 @@ int semanage_create_store(semanage_handle_t * sh, int create) return -1; } } else { - if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) == -1) { + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, path, mode_mask, AT_EACCESS) == -1) { ERR(sh, "Could not access module store at %s, or it is not a directory.", path); @@ -575,7 +575,7 @@ int semanage_create_store(semanage_handle_t * sh, int create) return -1; } } else { - if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) == -1) { + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, path, mode_mask, AT_EACCESS) == -1) { ERR(sh, "Could not access module store active subdirectory at %s, or it is not a directory.", path); @@ -598,7 +598,7 @@ int semanage_create_store(semanage_handle_t * sh, int create) return -1; } } else { - if (!S_ISDIR(sb.st_mode) || access(path, mode_mask) == -1) { + if (!S_ISDIR(sb.st_mode) || faccessat(AT_FDCWD, path, mode_mask, AT_EACCESS) == -1) { ERR(sh, "Could not access module store active modules subdirectory at %s, or it is not a directory.", path); @@ -619,7 +619,7 @@ int semanage_create_store(semanage_handle_t * sh, int create) return -1; } } else { - if (!S_ISREG(sb.st_mode) || access(path, R_OK | W_OK) == -1) { + if (!S_ISREG(sb.st_mode) || faccessat(AT_FDCWD, path, R_OK | W_OK, AT_EACCESS) == -1) { ERR(sh, "Could not access lock file at %s.", path); return -1; } @@ -639,7 +639,7 @@ int semanage_store_access_check(void) /* read access on active store */ path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_TOPLEVEL); - if (access(path, R_OK | X_OK) != 0) + if (faccessat(AT_FDCWD, path, R_OK | X_OK, AT_EACCESS) != 0) goto out; /* we can read the active store meaning it is managed @@ -650,13 +650,13 @@ int semanage_store_access_check(void) * write access necessary if the lock file does not exist */ path = semanage_files[SEMANAGE_READ_LOCK]; - if (access(path, R_OK) != 0) { + if (faccessat(AT_FDCWD, path, R_OK, AT_EACCESS) != 0) { if (access(path, F_OK) == 0) { goto out; } path = semanage_files[SEMANAGE_ROOT]; - if (access(path, R_OK | W_OK | X_OK) != 0) { + if (faccessat(AT_FDCWD, path, R_OK | W_OK | X_OK, AT_EACCESS) != 0) { goto out; } } @@ -666,7 +666,7 @@ int semanage_store_access_check(void) /* check the modules directory */ path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_MODULES); - if (access(path, R_OK | W_OK | X_OK) != 0) + if (faccessat(AT_FDCWD, path, R_OK | W_OK | X_OK, AT_EACCESS) != 0) goto out; rc = SEMANAGE_CAN_WRITE;
Use faccessat() with AT_EACCESS instead of accesss() in order to check permissions of effective user. access() calls checking existence of a file (F_OK) were left untouched since they work correctly. This enables setuid programs to use libsemanage. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431 Signed-off-by: Vit Mojzis <vmojzis@redhat.com> --- libsemanage/src/conf-parse.y | 7 ++++--- libsemanage/src/semanage_store.c | 18 +++++++++--------- 2 files changed, 13 insertions(+), 12 deletions(-)