[PATCH] libsemanage: Perform access check using euid instead of uid

[Vit Mojzis]

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 <***@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;

[Stephen Smalley]

Post by Vit Mojzis
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.

Post by Vit Mojzis
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
 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;

[Stephen Smalley]

Post by Stephen Smalley

Post by Vit Mojzis
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.

Post by Stephen Smalley
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.

Post by Vit Mojzis
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
 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
.nsa.gov.

[Stephen Smalley]

Post by Stephen Smalley

Post by Stephen Smalley

Post by Vit Mojzis
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).

Post by Stephen Smalley

Post by Stephen Smalley
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.

Post by Vit Mojzis
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
 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
ho
.nsa.gov.

_______________________________________________
Selinux mailing list
.nsa.gov.

[Stephen Smalley]

Post by Stephen Smalley

Post by Stephen Smalley

Post by Stephen Smalley

Post by Vit Mojzis
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

[Petr Lautrbach]

Post by Vit Mojzis
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.

Post by Vit Mojzis
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
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

[Stephen Smalley]

Post by Petr Lautrbach

Post by Vit Mojzis
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
---
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.

[Vit Mojzis]

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(-)

[Vit Mojzis]

access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431

Signed-off-by: Vit Mojzis <***@redhat.com>
---
libsemanage/src/direct_api.c | 36 +++++++++++++++++++++---------------
1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 508277d..35ca1b0 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -272,7 +272,9 @@ int semanage_direct_connect(semanage_handle_t * sh)

/* set the disable dontaudit value */
path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ struct stat sb;
+ if (stat(path, &sb) == 0)
sepol_set_disable_dontaudit(sh->sepolh, 1);
else
sepol_set_disable_dontaudit(sh->sepolh, 0);
@@ -1101,8 +1103,9 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
goto cleanup;
}

+ struct stat sb;
if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ stat(cil_path, &sb) == 0) {
continue;
}

@@ -1165,7 +1168,8 @@ static int semanage_direct_commit(semanage_handle_t * sh)

/* Create or remove the disable_dontaudit flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ struct stat sb;
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
else
do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
@@ -1190,7 +1194,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)

/* Create or remove the preserve_tunables flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
else
do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
@@ -1231,37 +1235,37 @@ static int semanage_direct_commit(semanage_handle_t * sh)
*/
if (!do_rebuild) {
path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}
@@ -1395,7 +1399,7 @@ rebuild:
goto cleanup;

path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
@@ -1408,7 +1412,7 @@ rebuild:
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
@@ -1732,7 +1736,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}

- if (access(module_path, F_OK) != 0) {
+ struct stat sb;
+ if (stat(module_path, &sb) != 0) {
ERR(sh, "Module does not exist: %s", module_path);
rc = -1;
goto cleanup;
@@ -1762,7 +1767,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}

- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
rc = semanage_compile_module(sh, _modinfo);
if (rc < 0) {
goto cleanup;
@@ -2737,7 +2742,8 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
goto cleanup;
}

- if (access(path, F_OK) == 0) {
+ struct stat sb;
+ if (stat(path, &sb) == 0) {
ret = unlink(path);
if (ret != 0) {
ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));

[Stephen Smalley]

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
 libsemanage/src/direct_api.c | 36 +++++++++++++++++++++-------------
--
 1 file changed, 21 insertions(+), 15 deletions(-)
diff --git a/libsemanage/src/direct_api.c
b/libsemanage/src/direct_api.c
index 508277d..35ca1b0 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -272,7 +272,9 @@ int semanage_direct_connect(semanage_handle_t * sh)
 
  /* set the disable dontaudit value */
  path = semanage_path(SEMANAGE_ACTIVE,
SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ struct stat sb;
+ if (stat(path, &sb) == 0)

Need to check errno as well to ensure that it is just ENOENT when
stat() returns non-zero; anything else is an error.

stat() can produce a SELinux getattr denial, whereas access(F_OK)
doesn't perform any SELinux check beyond directory search access.

Post by Vit Mojzis
  sepol_set_disable_dontaudit(sh->sepolh, 1);
  else
  sepol_set_disable_dontaudit(sh->sepolh, 0);
@@ -1101,8 +1103,9 @@ static int
semanage_compile_hll_modules(semanage_handle_t *sh,
  goto cleanup;
  }
 
+ struct stat sb;
  if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ stat(cil_path, &sb) == 0) {
  continue;
  }
 
@@ -1165,7 +1168,8 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
 
  /* Create or remove the disable_dontaudit flag file. */
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ struct stat sb;
+ if (stat(path, &sb) == 0)
  do_rebuild |= !(sepol_get_disable_dontaudit(sh-

Post by Vit Mojzis
sepolh) == 1);

  else
  do_rebuild |= (sepol_get_disable_dontaudit(sh-

Post by Vit Mojzis
sepolh) == 1);

@@ -1190,7 +1194,7 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
 
  /* Create or remove the preserve_tunables flag file. */
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
  do_rebuild |= !(sepol_get_preserve_tunables(sh-

Post by Vit Mojzis
sepolh) == 1);

  else
  do_rebuild |= (sepol_get_preserve_tunables(sh-

Post by Vit Mojzis
sepolh) == 1);

@@ -1231,37 +1235,37 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
   */
  if (!do_rebuild) {
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
  goto cleanup;
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
  retval = semanage_copy_file(path,
      semanage_path(SE
MANAGE_TMP,
    SE
MANAGE_STORE_SEUSERS),
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
  retval = semanage_copy_file(path,
      semanage_path(SE
MANAGE_TMP,
    SE
MANAGE_USERS_EXTRA),
@@ -1732,7 +1736,8 @@ static int
semanage_direct_extract(semanage_handle_t * sh,
  goto cleanup;
  }
 
- if (access(module_path, F_OK) != 0) {
+ struct stat sb;
+ if (stat(module_path, &sb) != 0) {
  ERR(sh, "Module does not exist: %s", module_path);
  rc = -1;
  goto cleanup;
@@ -1762,7 +1767,7 @@ static int
semanage_direct_extract(semanage_handle_t * sh,
  goto cleanup;
  }
 
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") &&
access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") &&
stat(input_file, &sb) != 0) {
  rc = semanage_compile_module(sh, _modinfo);
  if (rc < 0) {
  goto cleanup;
@@ -2737,7 +2742,8 @@ static int
semanage_direct_install_info(semanage_handle_t *sh,
  goto cleanup;
  }
 
- if (access(path, F_OK) == 0) {
+ struct stat sb;
+ if (stat(path, &sb) == 0) {
  ret = unlink(path);
  if (ret != 0) {
  ERR(sh, "Error while removing cached
CIL file %s: %s", path, strerror(errno));

[Vit Mojzis]

Post by Stephen Smalley

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 36 +++++++++++++++++++++-------------
--
1 file changed, 21 insertions(+), 15 deletions(-)
diff --git a/libsemanage/src/direct_api.c
b/libsemanage/src/direct_api.c
index 508277d..35ca1b0 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -272,7 +272,9 @@ int semanage_direct_connect(semanage_handle_t * sh)
/* set the disable dontaudit value */
path = semanage_path(SEMANAGE_ACTIVE,
SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ struct stat sb;
+ if (stat(path, &sb) == 0)

Need to check errno as well to ensure that it is just ENOENT when
stat() returns non-zero; anything else is an error.

Thank you for the feedback. I was trying to keep behavioral changes to a
minimum. In case "access" call failed (it can encounter all the errors
that "stat" can except for EOVERFLOW) the code assumed that the file
doesn't exist and this patch preserves said behavior.
But if you think think that the calling function should fail instead, I
can rework the patch.

Post by Stephen Smalley
stat() can produce a SELinux getattr denial, whereas access(F_OK)
doesn't perform any SELinux check beyond directory search access.

That is a fair point and I don't have a solution for it. However in my
understanding the calling process should have the "getattr" access
permission (and it should even be able to create the file in most cases).

Post by Stephen Smalley

Post by Vit Mojzis
sepol_set_disable_dontaudit(sh->sepolh, 1);
else
sepol_set_disable_dontaudit(sh->sepolh, 0);
@@ -1101,8 +1103,9 @@ static int
semanage_compile_hll_modules(semanage_handle_t *sh,
goto cleanup;
}
+ struct stat sb;
if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ stat(cil_path, &sb) == 0) {
continue;
}
@@ -1165,7 +1168,8 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the disable_dontaudit flag file. */
path = semanage_path(SEMANAGE_TMP,
SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ struct stat sb;
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_disable_dontaudit(sh-

Post by Vit Mojzis
sepolh) == 1);

else
do_rebuild |= (sepol_get_disable_dontaudit(sh-

Post by Vit Mojzis
sepolh) == 1);

@@ -1190,7 +1194,7 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the preserve_tunables flag file. */
path = semanage_path(SEMANAGE_TMP,
SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_preserve_tunables(sh-

Post by Vit Mojzis
sepolh) == 1);

else
do_rebuild |= (sepol_get_preserve_tunables(sh-

Post by Vit Mojzis
sepolh) == 1);

@@ -1231,37 +1235,37 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
*/
if (!do_rebuild) {
path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}
path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}
path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}
path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}
path = semanage_path(SEMANAGE_TMP,
SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}
path = semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}
goto cleanup;
path = semanage_path(SEMANAGE_TMP,
SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SE
MANAGE_TMP,
SE
MANAGE_STORE_SEUSERS),
}
path = semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SE
MANAGE_TMP,
SE
MANAGE_USERS_EXTRA),
@@ -1732,7 +1736,8 @@ static int
semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (access(module_path, F_OK) != 0) {
+ struct stat sb;
+ if (stat(module_path, &sb) != 0) {
ERR(sh, "Module does not exist: %s", module_path);
rc = -1;
goto cleanup;
@@ -1762,7 +1767,7 @@ static int
semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") &&
access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") &&
stat(input_file, &sb) != 0) {
rc = semanage_compile_module(sh, _modinfo);
if (rc < 0) {
goto cleanup;
@@ -2737,7 +2742,8 @@ static int
semanage_direct_install_info(semanage_handle_t *sh,
goto cleanup;
}
- if (access(path, F_OK) == 0) {
+ struct stat sb;
+ if (stat(path, &sb) == 0) {
ret = unlink(path);
if (ret != 0) {
ERR(sh, "Error while removing cached
CIL file %s: %s", path, strerror(errno));

[Stephen Smalley]

Post by Vit Mojzis

Post by Stephen Smalley

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
  libsemanage/src/direct_api.c | 36 +++++++++++++++++++++------
-------
--
  1 file changed, 21 insertions(+), 15 deletions(-)
diff --git a/libsemanage/src/direct_api.c
b/libsemanage/src/direct_api.c
index 508277d..35ca1b0 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -272,7 +272,9 @@ int semanage_direct_connect(semanage_handle_t
*
sh)
  
   /* set the disable dontaudit value */
   path = semanage_path(SEMANAGE_ACTIVE,
SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ struct stat sb;
+ if (stat(path, &sb) == 0)

Need to check errno as well to ensure that it is just ENOENT when
stat() returns non-zero; anything else is an error.

Thank you for the feedback. I was trying to keep behavioral changes
to a 
minimum. In case "access" call failed (it can encounter all the
errors 
that "stat" can except for EOVERFLOW) the code assumed that the file 
doesn't exist and this patch preserves said behavior.
But if you think think that the calling function should fail instead,
I 
can rework the patch.

Except that use of stat() does introduce one additional potential
failure case, i.e. SELinux getattr permission denial on the file.
Unlikely, of course, but possible. And since you removed the
semanage_access_check() in the earlier patch, we are no longer checking
that we can even read/search the active directory prior to making this
call. So our first permission denial (on either the active directory or
the file itself) might occur here, and we would wrongly interpret it as
the file not existing after your change.

It is also more correct to explicitly test errno for ENOENT regardless,
so we might as well fix that while we are making these changes.

Post by Vit Mojzis

Post by Stephen Smalley
stat() can produce a SELinux getattr denial, whereas access(F_OK)
doesn't perform any SELinux check beyond directory search access.

That is a fair point and I don't have a solution for it. However in
my 
understanding the calling process should have the "getattr" access 
permission (and it should even be able to create the file in most cases).

Yes, I'm not concerned about needing getattr permission itself; I just
want to ensure that we correctly handle it when it is denied.

Post by Vit Mojzis

Post by Stephen Smalley

Post by Vit Mojzis
   sepol_set_disable_dontaudit(sh->sepolh, 1);
   else
   sepol_set_disable_dontaudit(sh->sepolh, 0);
@@ -1101,8 +1103,9 @@ static int
semanage_compile_hll_modules(semanage_handle_t *sh,
   goto cleanup;
   }
  
+ struct stat sb;
   if (semanage_get_ignore_module_cache(sh) == 0
&&
- access(cil_path, F_OK) == 0) {
+ stat(cil_path, &sb) == 0) {
   continue;
   }
  
@@ -1165,7 +1168,8 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
  
   /* Create or remove the disable_dontaudit flag file. */
   path = semanage_path(SEMANAGE_TMP,
SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ struct stat sb;
+ if (stat(path, &sb) == 0)
   do_rebuild |= !(sepol_get_disable_dontaudit(sh-

Post by Vit Mojzis
sepolh) == 1);

   else
   do_rebuild |= (sepol_get_disable_dontaudit(sh-

Post by Vit Mojzis
sepolh) == 1);

@@ -1190,7 +1194,7 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
  
   /* Create or remove the preserve_tunables flag file. */
   path = semanage_path(SEMANAGE_TMP,
SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
   do_rebuild |= !(sepol_get_preserve_tunables(sh-

Post by Vit Mojzis
sepolh) == 1);

   else
   do_rebuild |= (sepol_get_preserve_tunables(sh-

Post by Vit Mojzis
sepolh) == 1);

@@ -1231,37 +1235,37 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
    */
   if (!do_rebuild) {
   path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
   do_rebuild = 1;
   goto rebuild;
   }
  
   path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
   do_rebuild = 1;
   goto rebuild;
   }
  
   path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
   do_rebuild = 1;
   goto rebuild;
   }
  
   path = semanage_path(SEMANAGE_TMP,
SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
   do_rebuild = 1;
   goto rebuild;
   }
  
   path = semanage_path(SEMANAGE_TMP,
SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
   do_rebuild = 1;
   goto rebuild;
   }
  
   path = semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
   do_rebuild = 1;
   goto rebuild;
   }
   goto cleanup;
  
   path = semanage_path(SEMANAGE_TMP,
SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
   retval = semanage_copy_file(path,
       semanage_pa
th(SE
MANAGE_TMP,
  
  SE
MANAGE_STORE_SEUSERS),
   }
  
   path = semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
   retval = semanage_copy_file(path,
       semanage_pa
th(SE
MANAGE_TMP,
  
  SE
MANAGE_USERS_EXTRA),
@@ -1732,7 +1736,8 @@ static int
semanage_direct_extract(semanage_handle_t * sh,
   goto cleanup;
   }
  
- if (access(module_path, F_OK) != 0) {
+ struct stat sb;
+ if (stat(module_path, &sb) != 0) {
   ERR(sh, "Module does not exist: %s",
module_path);
   rc = -1;
   goto cleanup;
@@ -1762,7 +1767,7 @@ static int
semanage_direct_extract(semanage_handle_t * sh,
   goto cleanup;
   }
  
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext,
"cil") &&
access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext,
"cil") &&
stat(input_file, &sb) != 0) {
   rc = semanage_compile_module(sh, _modinfo);
   if (rc < 0) {
   goto cleanup;
@@ -2737,7 +2742,8 @@ static int
semanage_direct_install_info(semanage_handle_t *sh,
   goto cleanup;
   }
  
- if (access(path, F_OK) == 0) {
+ struct stat sb;
+ if (stat(path, &sb) == 0) {
   ret = unlink(path);
   if (ret != 0) {
   ERR(sh, "Error while removing
cached
CIL file %s: %s", path, strerror(errno));

[Vit Mojzis]

access() uses real UID instead of effective UID which causes false
negative checks in setuid programs. Remove redundant access() checks

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 7 -------
libsemanage/src/semanage_store.c | 17 ++++++++---------
2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 65842df..b761b68 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -148,9 +148,6 @@ int semanage_direct_connect(semanage_handle_t * sh)
if (semanage_create_store(sh, 1))
goto err;

- if (semanage_access_check(sh) < SEMANAGE_CAN_READ)
- goto err;
-
sh->u.direct.translock_file_fd = -1;
sh->u.direct.activelock_file_fd = -1;

@@ -373,10 +370,6 @@ static int semanage_direct_disconnect(semanage_handle_t * sh)

static int semanage_direct_begintrans(semanage_handle_t * sh)
{
-
- if (semanage_access_check(sh) != SEMANAGE_CAN_WRITE) {
- return -1;
- }
if (semanage_get_trans_lock(sh) < 0) {
return -1;
}
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 6158d08..ac84349 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -537,7 +537,6 @@ char *semanage_conf_path(void)
int semanage_create_store(semanage_handle_t * sh, int create)
{
struct stat sb;
- int mode_mask = R_OK | W_OK | X_OK;
const char *path = semanage_files[SEMANAGE_ROOT];
int fd;

@@ -556,9 +555,9 @@ 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)) {
ERR(sh,
- "Could not access module store at %s, or it is not a directory.",
+ "Module store at %s is not a directory.",
path);
return -1;
}
@@ -579,9 +578,9 @@ 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)) {
ERR(sh,
- "Could not access module store active subdirectory at %s, or it is not a directory.",
+ "Module store active subdirectory at %s is not a directory.",
path);
return -1;
}
@@ -602,9 +601,9 @@ 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)) {
ERR(sh,
- "Could not access module store active modules subdirectory at %s, or it is not a directory.",
+ "Module store active modules subdirectory at %s is not a directory.",
path);
return -1;
}
@@ -623,8 +622,8 @@ 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) {
- ERR(sh, "Could not access lock file at %s.", path);
+ if (!S_ISREG(sb.st_mode)) {
+ ERR(sh, "Object at %s is not a lock file.", path);
return -1;
}
}

[Vit Mojzis]

F_OK access checks only work properly as long as all directories along
the path are accessible to real user running the program.
Replace F_OK access checks by testing return value of open, write, etc.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 36 +++++++++++++++---------------------
1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index b761b68..ed11a7c 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -1538,33 +1538,27 @@ rebuild:
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}

/* run genhomedircon if its enabled, this should be the last operation

[Stephen Smalley]

Post by Vit Mojzis
F_OK access checks only work properly as long as all directories along
the path are accessible to real user running the program.
Replace F_OK access checks by testing return value of open, write, etc.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
 libsemanage/src/direct_api.c | 36 +++++++++++++++-------------------
--
 1 file changed, 15 insertions(+), 21 deletions(-)
diff --git a/libsemanage/src/direct_api.c
b/libsemanage/src/direct_api.c
index b761b68..ed11a7c 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
  }
 
  path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL);
- if (access(path, F_OK) == 0) {
- retval =
semanage_copy_file(semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_FC_LOCAL),
- semanage_fin
al_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
- sh->conf-

Post by Vit Mojzis
file_mode);

- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_FC_LOCAL),
+ semanage_final_path(
SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
+ sh->conf-

Post by Vit Mojzis
file_mode);

+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
  }
  path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) == 0) {
- retval =
semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
- semanage_fin
al_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
- sh->conf-

Post by Vit Mojzis
file_mode);

- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_FC),
+ semanage_final_path(
SEMANAGE_FINAL_TMP, SEMANAGE_FC),
+ sh->conf-

Post by Vit Mojzis
file_mode);

+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
  }
 
  path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) == 0) {
- retval =
semanage_copy_file(semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
- semanage_fin
al_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
- sh->conf-

Post by Vit Mojzis
file_mode);

- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
+ semanage_final_path(
SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
+ sh->conf-

Post by Vit Mojzis
file_mode);

+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
  }
 
  /* run genhomedircon if its enabled, this should be the last
operation

Isn't the assignment to path in each of the sequences above rendered
moot by your removal of the call to access()? So you could either
remove the assignment to path each time, or alternatively retain it but
pass path as the first argument to semanage_copy_file() rather than
calling semanage_path() again.

[Vit Mojzis]

access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431

Signed-off-by: Vit Mojzis <***@redhat.com>
---
libsemanage/src/direct_api.c | 50 +++++++++++++++++++++++++++++---------------
1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index ed11a7c..3d1c354 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -296,10 +296,19 @@ int semanage_direct_connect(semanage_handle_t * sh)

/* set the disable dontaudit value */
path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ struct stat sb;
+ if (stat(path, &sb) == 0)
sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
- sepol_set_disable_dontaudit(sh->sepolh, 0);
+ else{
+ if (errno == ENOENT){
+ sepol_set_disable_dontaudit(sh->sepolh, 0);
+ }
+ else{
+ ERR(sh, "Unable to access disable_dontaudit file at %s: %s\n",path , strerror(errno));
+ goto err;
+ }
+ }

return STATUS_SUCCESS;

@@ -1114,6 +1123,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
int status = 0;
int i;
char cil_path[PATH_MAX];
+ struct stat sb;

assert(sh);
assert(modinfos);
@@ -1130,9 +1140,12 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
}

if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0) {
continue;
}
+ if (status != 0 && status != ENOENT) {
+ goto cleanup; //an error in the "stat" call
+ }

status = semanage_compile_module(sh, &modinfos[i]);
if (status < 0) {
@@ -1200,7 +1213,8 @@ static int semanage_direct_commit(semanage_handle_t * sh)

/* Create or remove the disable_dontaudit flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ struct stat sb;
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
else
do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
@@ -1225,7 +1239,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)

/* Create or remove the preserve_tunables flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
else
do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
@@ -1266,37 +1280,37 @@ static int semanage_direct_commit(semanage_handle_t * sh)
*/
if (!do_rebuild) {
path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
do_rebuild = 1;
goto rebuild;
}
@@ -1431,7 +1445,7 @@ rebuild:
goto cleanup;

path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
@@ -1444,7 +1458,7 @@ rebuild:
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
@@ -1785,7 +1799,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}

- if (access(module_path, F_OK) != 0) {
+ struct stat sb;
+ if (stat(module_path, &sb) != 0) {
ERR(sh, "Module does not exist: %s", module_path);
rc = -1;
goto cleanup;
@@ -1815,7 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}

- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
rc = semanage_compile_module(sh, _modinfo);
if (rc < 0) {
goto cleanup;
@@ -2790,7 +2805,8 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
goto cleanup;
}

- if (access(path, F_OK) == 0) {
+ struct stat sb;
+ if (stat(path, &sb) == 0) {
ret = unlink(path);
if (ret != 0) {
ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));

[Stephen Smalley]

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
 libsemanage/src/direct_api.c | 50 +++++++++++++++++++++++++++++-----
----------
 1 file changed, 33 insertions(+), 17 deletions(-)
diff --git a/libsemanage/src/direct_api.c
b/libsemanage/src/direct_api.c
index ed11a7c..3d1c354 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -296,10 +296,19 @@ int semanage_direct_connect(semanage_handle_t * sh)
 
  /* set the disable dontaudit value */
  path = semanage_path(SEMANAGE_ACTIVE,
SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ struct stat sb;

Please add a blank line after declarations before code, and move this
up to the beginning of the function.

Post by Vit Mojzis
+ if (stat(path, &sb) == 0)
  sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
- sepol_set_disable_dontaudit(sh->sepolh, 0);
+ else{

Please include a space between else and {.

Post by Vit Mojzis
+ if (errno == ENOENT){

Please include a space between ) and {.

Post by Vit Mojzis
+ sepol_set_disable_dontaudit(sh->sepolh, 0);
+ }
+ else{

This should be } else { on a single line, with space between else and
{.

Post by Vit Mojzis
+ ERR(sh, "Unable to access disable_dontaudit
file at %s: %s\n",path , strerror(errno));
+ goto err;
+ }
+ }
 
  return STATUS_SUCCESS;
 
@@ -1114,6 +1123,7 @@ static int
semanage_compile_hll_modules(semanage_handle_t *sh,
  int status = 0;
  int i;
  char cil_path[PATH_MAX];
+ struct stat sb;
 
  assert(sh);
  assert(modinfos);
@@ -1130,9 +1140,12 @@ static int
semanage_compile_hll_modules(semanage_handle_t *sh,
  }
 
  if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0)
{
  continue;
  }
+ if (status != 0 && status != ENOENT) {

errno != ENOENT

Also, we should ERR() here since otherwise we won't get any diagnostic
output (in the other error cases, the called function handles this, but
here we have a direct failure in this function).

Post by Vit Mojzis
+ goto cleanup; //an error in the "stat" call
+ }
 
  status = semanage_compile_module(sh, &modinfos[i]);
  if (status < 0) {
@@ -1200,7 +1213,8 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
 
  /* Create or remove the disable_dontaudit flag file. */
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ struct stat sb;

Empty line between declarations and code, move decls to beginning of
function.

Post by Vit Mojzis
+ if (stat(path, &sb) == 0)
  do_rebuild |= !(sepol_get_disable_dontaudit(sh-

Post by Vit Mojzis
sepolh) == 1);

  else
  do_rebuild |= (sepol_get_disable_dontaudit(sh-

Post by Vit Mojzis
sepolh) == 1);

No errno check here or below?

Post by Vit Mojzis
@@ -1225,7 +1239,7 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
 
  /* Create or remove the preserve_tunables flag file. */
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
  do_rebuild |= !(sepol_get_preserve_tunables(sh-

Post by Vit Mojzis
sepolh) == 1);

  else
  do_rebuild |= (sepol_get_preserve_tunables(sh-

Post by Vit Mojzis
sepolh) == 1);

@@ -1266,37 +1280,37 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
   */
  if (!do_rebuild) {
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
  goto cleanup;
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
  retval = semanage_copy_file(path,
      semanage_path(SE
MANAGE_TMP,
    SE
MANAGE_STORE_SEUSERS),
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
  retval = semanage_copy_file(path,
      semanage_path(SE
MANAGE_TMP,
    SE
MANAGE_USERS_EXTRA),
@@ -1785,7 +1799,8 @@ static int
semanage_direct_extract(semanage_handle_t * sh,
  goto cleanup;
  }
 
- if (access(module_path, F_OK) != 0) {
+ struct stat sb;
+ if (stat(module_path, &sb) != 0) {
  ERR(sh, "Module does not exist: %s", module_path);
  rc = -1;
  goto cleanup;
@@ -1815,7 +1830,7 @@ static int
semanage_direct_extract(semanage_handle_t * sh,
  goto cleanup;
  }
 
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") &&
access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") &&
stat(input_file, &sb) != 0) {
  rc = semanage_compile_module(sh, _modinfo);
  if (rc < 0) {
  goto cleanup;
@@ -2790,7 +2805,8 @@ static int
semanage_direct_install_info(semanage_handle_t *sh,
  goto cleanup;
  }
 
- if (access(path, F_OK) == 0) {
+ struct stat sb;
+ if (stat(path, &sb) == 0) {
  ret = unlink(path);
  if (ret != 0) {
  ERR(sh, "Error while removing cached
CIL file %s: %s", path, strerror(errno));

[Vit Mojzis]

access() uses real UID instead of effective UID which causes false
negative checks in setuid programs. Remove redundant access() checks

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431

Signed-off-by: Vit Mojzis <***@redhat.com>
---
libsemanage/src/direct_api.c | 7 -------
libsemanage/src/semanage_store.c | 17 ++++++++---------
2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 88873c43..b7899d68 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -148,9 +148,6 @@ int semanage_direct_connect(semanage_handle_t * sh)
if (semanage_create_store(sh, 1))
goto err;

- if (semanage_access_check(sh) < SEMANAGE_CAN_READ)
- goto err;
-
sh->u.direct.translock_file_fd = -1;
sh->u.direct.activelock_file_fd = -1;

@@ -398,10 +395,6 @@ static int semanage_direct_disconnect(semanage_handle_t *sh)

static int semanage_direct_begintrans(semanage_handle_t * sh)
{
-
- if (semanage_access_check(sh) != SEMANAGE_CAN_WRITE) {
- return -1;
- }
if (semanage_get_trans_lock(sh) < 0) {
return -1;
}
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 936e6495..4bd1d651 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -538,7 +538,6 @@ char *semanage_conf_path(void)
int semanage_create_store(semanage_handle_t * sh, int create)
{
struct stat sb;
- int mode_mask = R_OK | W_OK | X_OK;
const char *path = semanage_files[SEMANAGE_ROOT];
int fd;

@@ -557,9 +556,9 @@ 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)) {
ERR(sh,
- "Could not access module store at %s, or it is not a directory.",
+ "Module store at %s is not a directory.",
path);
return -1;
}
@@ -580,9 +579,9 @@ 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)) {
ERR(sh,
- "Could not access module store active subdirectory at %s, or it is not a directory.",
+ "Module store active subdirectory at %s is not a directory.",
path);
return -1;
}
@@ -603,9 +602,9 @@ 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)) {
ERR(sh,
- "Could not access module store active modules subdirectory at %s, or it is not a directory.",
+ "Module store active modules subdirectory at %s is not a directory.",
path);
return -1;
}
@@ -624,8 +623,8 @@ 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) {
- ERR(sh, "Could not access lock file at %s.", path);
+ if (!S_ISREG(sb.st_mode)) {
+ ERR(sh, "Object at %s is not a lock file.", path);
return -1;
}
}

[Vit Mojzis]

F_OK access checks only work properly as long as all directories along
the path are accessible to real user running the program.
Replace F_OK access checks by testing return value of open, write, etc.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431

Signed-off-by: Vit Mojzis <***@redhat.com>
---
libsemanage/src/direct_api.c | 39 +++++++++++++++------------------------
1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index b7899d68..f4d57cf8 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -1563,34 +1563,25 @@ rebuild:
goto cleanup;
}

- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}

- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}

- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}

/* run genhomedircon if its enabled, this should be the last operation

[William Roberts]

Post by Vit Mojzis
F_OK access checks only work properly as long as all directories along
the path are accessible to real user running the program.
Replace F_OK access checks by testing return value of open, write, etc.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 39 +++++++++++++++------------------------
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index b7899d68..f4d57cf8 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
goto cleanup;
}
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}
/* run genhomedircon if its enabled, this should be the last operation
--
2.14.3

tentative ack. I need to run tests on this one, but it looks fine. I
suspect this should work looking at semanage_copy_file() internals.

[Stephen Smalley]

Post by Vit Mojzis
F_OK access checks only work properly as long as all directories along
the path are accessible to real user running the program.
Replace F_OK access checks by testing return value of open, write, etc.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 39 +++++++++++++++------------------------
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index b7899d68..f4d57cf8 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
goto cleanup;
}
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}

I think we need to clear retval here to ensure that we don't later
misinterpret a negative retval as an error. Otherwise, if
disable_genhomedircon && !do_install, we'll fall through to the end of
the function without ever setting retval again, so the mere absence of a
seusers file will cause a fatal error from semanage_direct_commit.

Post by Vit Mojzis
/* run genhomedircon if its enabled, this should be the last operation

[William Roberts]

Post by Stephen Smalley

Post by Vit Mojzis
F_OK access checks only work properly as long as all directories along
the path are accessible to real user running the program.
Replace F_OK access checks by testing return value of open, write, etc.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 39 +++++++++++++++------------------------
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index b7899d68..f4d57cf8 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
goto cleanup;
}
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}

I think we need to clear retval here to ensure that we don't later
misinterpret a negative retval as an error. Otherwise, if
disable_genhomedircon && !do_install, we'll fall through to the end of
the function without ever setting retval again, so the mere absence of a
seusers file will cause a fatal error from semanage_direct_commit.

That's spot on. That is precisely what happens. In the case retval is
-1 and errno IS
ENOENT, retval needs to be set to 0.

Maybe just make a static helper so this way you don't have to even
worry about errno here:

static int copy_file(...) {
int rc = semanage_copy_file(...);
return !(rc < 0 && errno != ENOENT);
}

It would make that main code simpler to read and then retval should be
0 on ENOENT case.

Post by Stephen Smalley

Post by Vit Mojzis
/* run genhomedircon if its enabled, this should be the last operation

[Vit Mojzis]

access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
And access(,R_OK) by fopen(,"r")

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431

Signed-off-by: Vit Mojzis <***@redhat.com>
---
libsemanage/src/direct_api.c | 132 +++++++++++++++++++++++++--------------
libsemanage/src/semanage_store.c | 14 ++++-
2 files changed, 98 insertions(+), 48 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index f4d57cf8..d42a51cd 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
int semanage_direct_connect(semanage_handle_t * sh)
{
const char *path;
+ struct stat sb;

if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
@@ -302,10 +303,17 @@ int semanage_direct_connect(semanage_handle_t * sh)

/* set the disable dontaudit value */
path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ if (stat(path, &sb) == 0)
sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto err;
+ }
+
sepol_set_disable_dontaudit(sh->sepolh, 0);
+ }

return STATUS_SUCCESS;

@@ -1139,6 +1147,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
int status = 0;
int i;
char cil_path[PATH_MAX];
+ struct stat sb;

assert(sh);
assert(modinfos);
@@ -1155,9 +1164,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
}

if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0) {
continue;
}
+ if (status != 0 && errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+ goto cleanup; //an error in the "stat" call
+ }

status = semanage_compile_module(sh, &modinfos[i]);
if (status < 0) {
@@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
struct cil_db *cildb = NULL;
semanage_module_info_t *modinfos = NULL;
mode_t mask = umask(0077);
+ struct stat sb;

int do_rebuild, do_write_kernel, do_install;
int fcontexts_modified, ports_modified, seusers_modified,
@@ -1226,10 +1240,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)

/* Create or remove the disable_dontaudit flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+ }
if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1251,10 +1271,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)

/* Create or remove the preserve_tunables flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
+ }
+
if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
* a rebuild.
*/
if (!do_rebuild) {
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
+ int files[] = {SEMANAGE_STORE_KERNEL,
+ SEMANAGE_STORE_FC,
+ SEMANAGE_STORE_SEUSERS,
+ SEMANAGE_LINKED,
+ SEMANAGE_SEUSERS_LINKED,
+ SEMANAGE_USERS_EXTRA_LINKED};
+
+ for (i = 0; i < (int) sizeof(files); i++) {
+ path = semanage_path(SEMANAGE_TMP, files[i]);
+ if (stat(path, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }

- path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
+ do_rebuild = 1;
+ goto rebuild;
+ }
}
}

@@ -1457,7 +1468,7 @@ rebuild:
goto cleanup;

path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
@@ -1466,11 +1477,16 @@ rebuild:
goto cleanup;
pseusers->dtable->drop_cache(pseusers->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pseusers->dtable->clear(sh, pseusers->dbase);
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
@@ -1479,6 +1495,11 @@ rebuild:
goto cleanup;
pusers_extra->dtable->drop_cache(pusers_extra->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pusers_extra->dtable->clear(sh, pusers_extra->dbase);
}
}
@@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
ssize_t _data_len;
char *_data;
int compressed;
+ struct stat sb;

/* get path of module */
rc = semanage_module_get_path(
@@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}

- if (access(module_path, F_OK) != 0) {
- ERR(sh, "Module does not exist: %s", module_path);
+ if (stat(module_path, &sb) != 0) {
+ ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
rc = -1;
goto cleanup;
}
@@ -1851,7 +1873,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}

- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
+ goto cleanup;
+ }
+
rc = semanage_compile_module(sh, _modinfo);
if (rc < 0) {
goto cleanup;
@@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
}

if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
*enabled = 1;
}
else {
@@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,

/* set enabled/disabled status */
if (stat(fn, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
if (ret != 0) {
status = -1;
@@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
int status = 0;
int ret = 0;
int type;
+ struct stat sb;

char path[PATH_MAX];
mode_t mask = umask(0077);
@@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
goto cleanup;
}

- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
ret = unlink(path);
if (ret != 0) {
ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 4bd1d651..83c188dc 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -514,6 +514,7 @@ char *semanage_conf_path(void)
{
char *semanage_conf = NULL;
int len;
+ FILE *fp = NULL;

len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
semanage_conf = calloc(len + 1, sizeof(char));
@@ -522,7 +523,10 @@ 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) {
+ //the following replaces access(semanage_conf, R_OK) check
+ if ((fp = fopen(semanage_conf, "r")) != NULL) {
+ fclose(fp);
+ } else {
snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
}

@@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
static int sefcontext_compile(semanage_handle_t * sh, const char *path) {

int r;
+ struct stat sb;
+
+ if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ return -1;
+ }

- if (access(path, F_OK) != 0) {
return 0;
}

[William Roberts]

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
And access(,R_OK) by fopen(,"r")
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 132 +++++++++++++++++++++++++--------------
libsemanage/src/semanage_store.c | 14 ++++-
2 files changed, 98 insertions(+), 48 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index f4d57cf8..d42a51cd 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
int semanage_direct_connect(semanage_handle_t * sh)
{
const char *path;
+ struct stat sb;
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
@@ -302,10 +303,17 @@ int semanage_direct_connect(semanage_handle_t * sh)
/* set the disable dontaudit value */
path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ if (stat(path, &sb) == 0)
sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto err;
+ }
+
sepol_set_disable_dontaudit(sh->sepolh, 0);
+ }
return STATUS_SUCCESS;
@@ -1139,6 +1147,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
int status = 0;
int i;
char cil_path[PATH_MAX];
+ struct stat sb;
assert(sh);
assert(modinfos);
@@ -1155,9 +1164,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
}
if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0) {
continue;
}
+ if (status != 0 && errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+ goto cleanup; //an error in the "stat" call
+ }
status = semanage_compile_module(sh, &modinfos[i]);
if (status < 0) {
@@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
struct cil_db *cildb = NULL;
semanage_module_info_t *modinfos = NULL;
mode_t mask = umask(0077);
+ struct stat sb;
int do_rebuild, do_write_kernel, do_install;
int fcontexts_modified, ports_modified, seusers_modified,
@@ -1226,10 +1240,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the disable_dontaudit flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+

I am not a huge fan of this if under else block of if (errno !=ENOENT).

maybe this is a bit cleaner:

if (stat() == 0)
//exists
else if (errno == ENOENT)
// doesn't exist
else
//fail

Post by Vit Mojzis
do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+ }
if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1251,10 +1271,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the preserve_tunables flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
+ }
+
if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
* a rebuild.
*/
if (!do_rebuild) {
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
+ int files[] = {SEMANAGE_STORE_KERNEL,
+ SEMANAGE_STORE_FC,
+ SEMANAGE_STORE_SEUSERS,
+ SEMANAGE_LINKED,
+ SEMANAGE_SEUSERS_LINKED,
+ SEMANAGE_USERS_EXTRA_LINKED};
+
+ for (i = 0; i < (int) sizeof(files); i++) {
+ path = semanage_path(SEMANAGE_TMP, files[i]);
+ if (stat(path, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
+ do_rebuild = 1;
+ goto rebuild;
+ }
}
}
goto cleanup;
path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
goto cleanup;
pseusers->dtable->drop_cache(pseusers->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pseusers->dtable->clear(sh, pseusers->dbase);
}
path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
goto cleanup;
pusers_extra->dtable->drop_cache(pusers_extra->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pusers_extra->dtable->clear(sh, pusers_extra->dbase);
}
}
@@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
ssize_t _data_len;
char *_data;
int compressed;
+ struct stat sb;
/* get path of module */
rc = semanage_module_get_path(
@@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (access(module_path, F_OK) != 0) {
- ERR(sh, "Module does not exist: %s", module_path);
+ if (stat(module_path, &sb) != 0) {
+ ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
rc = -1;
goto cleanup;
}
@@ -1851,7 +1873,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
+ goto cleanup;
+ }
+
rc = semanage_compile_module(sh, _modinfo);
if (rc < 0) {
goto cleanup;
@@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
}
if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
*enabled = 1;
}
else {
@@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
/* set enabled/disabled status */
if (stat(fn, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
if (ret != 0) {
status = -1;
@@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
int status = 0;
int ret = 0;
int type;
+ struct stat sb;
char path[PATH_MAX];
mode_t mask = umask(0077);
@@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
goto cleanup;
}
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
ret = unlink(path);
if (ret != 0) {
ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 4bd1d651..83c188dc 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -514,6 +514,7 @@ char *semanage_conf_path(void)
{
char *semanage_conf = NULL;
int len;
+ FILE *fp = NULL;
len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
semanage_conf = calloc(len + 1, sizeof(char));
@@ -522,7 +523,10 @@ 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) {
+ //the following replaces access(semanage_conf, R_OK) check
+ if ((fp = fopen(semanage_conf, "r")) != NULL) {
+ fclose(fp);
+ } else {
snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
}
@@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
int r;
+ struct stat sb;
+
+ if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ return -1;
+ }
- if (access(path, F_OK) != 0) {
return 0;
}
--
2.14.3

[William Roberts]

Where is patch 2/2, I have yet to see it?

Did something get screwy and is it: [PATCH] libsemanage: Improve
warning for installing disabled module


On Wed, Feb 28, 2018 at 9:50 AM, William Roberts

Post by William Roberts

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
And access(,R_OK) by fopen(,"r")
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 132 +++++++++++++++++++++++++--------------
libsemanage/src/semanage_store.c | 14 ++++-
2 files changed, 98 insertions(+), 48 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index f4d57cf8..d42a51cd 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
int semanage_direct_connect(semanage_handle_t * sh)
{
const char *path;
+ struct stat sb;
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
@@ -302,10 +303,17 @@ int semanage_direct_connect(semanage_handle_t * sh)
/* set the disable dontaudit value */
path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ if (stat(path, &sb) == 0)
sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto err;
+ }
+
sepol_set_disable_dontaudit(sh->sepolh, 0);
+ }
return STATUS_SUCCESS;
@@ -1139,6 +1147,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
int status = 0;
int i;
char cil_path[PATH_MAX];
+ struct stat sb;
assert(sh);
assert(modinfos);
@@ -1155,9 +1164,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
}
if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0) {
continue;
}
+ if (status != 0 && errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+ goto cleanup; //an error in the "stat" call
+ }
status = semanage_compile_module(sh, &modinfos[i]);
if (status < 0) {
@@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
struct cil_db *cildb = NULL;
semanage_module_info_t *modinfos = NULL;
mode_t mask = umask(0077);
+ struct stat sb;
int do_rebuild, do_write_kernel, do_install;
int fcontexts_modified, ports_modified, seusers_modified,
@@ -1226,10 +1240,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the disable_dontaudit flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+

I am not a huge fan of this if under else block of if (errno !=ENOENT).
if (stat() == 0)
//exists
else if (errno == ENOENT)
// doesn't exist
else
//fail

Post by Vit Mojzis
do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+ }
if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1251,10 +1271,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the preserve_tunables flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
+ }
+
if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
* a rebuild.
*/
if (!do_rebuild) {
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
+ int files[] = {SEMANAGE_STORE_KERNEL,
+ SEMANAGE_STORE_FC,
+ SEMANAGE_STORE_SEUSERS,
+ SEMANAGE_LINKED,
+ SEMANAGE_SEUSERS_LINKED,
+ SEMANAGE_USERS_EXTRA_LINKED};
+
+ for (i = 0; i < (int) sizeof(files); i++) {
+ path = semanage_path(SEMANAGE_TMP, files[i]);
+ if (stat(path, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
+ do_rebuild = 1;
+ goto rebuild;
+ }
}
}
goto cleanup;
path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
goto cleanup;
pseusers->dtable->drop_cache(pseusers->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pseusers->dtable->clear(sh, pseusers->dbase);
}
path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
goto cleanup;
pusers_extra->dtable->drop_cache(pusers_extra->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pusers_extra->dtable->clear(sh, pusers_extra->dbase);
}
}
@@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
ssize_t _data_len;
char *_data;
int compressed;
+ struct stat sb;
/* get path of module */
rc = semanage_module_get_path(
@@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (access(module_path, F_OK) != 0) {
- ERR(sh, "Module does not exist: %s", module_path);
+ if (stat(module_path, &sb) != 0) {
+ ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
rc = -1;
goto cleanup;
}
@@ -1851,7 +1873,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
+ goto cleanup;
+ }
+
rc = semanage_compile_module(sh, _modinfo);
if (rc < 0) {
goto cleanup;
@@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
}
if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
*enabled = 1;
}
else {
@@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
/* set enabled/disabled status */
if (stat(fn, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
if (ret != 0) {
status = -1;
@@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
int status = 0;
int ret = 0;
int type;
+ struct stat sb;
char path[PATH_MAX];
mode_t mask = umask(0077);
@@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
goto cleanup;
}
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
ret = unlink(path);
if (ret != 0) {
ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 4bd1d651..83c188dc 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -514,6 +514,7 @@ char *semanage_conf_path(void)
{
char *semanage_conf = NULL;
int len;
+ FILE *fp = NULL;
len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
semanage_conf = calloc(len + 1, sizeof(char));
@@ -522,7 +523,10 @@ 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) {
+ //the following replaces access(semanage_conf, R_OK) check
+ if ((fp = fopen(semanage_conf, "r")) != NULL) {
+ fclose(fp);
+ } else {
snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
}
@@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
int r;
+ struct stat sb;
+
+ if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ return -1;
+ }
- if (access(path, F_OK) != 0) {
return 0;
}
--
2.14.3

--
Respectfully,
William C Roberts

[Stephen Smalley]

Post by William Roberts
Where is patch 2/2, I have yet to see it?
Did something get screwy and is it: [PATCH] libsemanage: Improve
warning for installing disabled module

No, 2/3 was a separate patch and had the 2/3 in the subject line.
I received all three from the list, both locally and on my gmail.

Post by William Roberts
On Wed, Feb 28, 2018 at 9:50 AM, William Roberts

Post by William Roberts

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
And access(,R_OK) by fopen(,"r")
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 132 +++++++++++++++++++++++++--------------
libsemanage/src/semanage_store.c | 14 ++++-
2 files changed, 98 insertions(+), 48 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index f4d57cf8..d42a51cd 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
int semanage_direct_connect(semanage_handle_t * sh)
{
const char *path;
+ struct stat sb;
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
@@ -302,10 +303,17 @@ int semanage_direct_connect(semanage_handle_t * sh)
/* set the disable dontaudit value */
path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ if (stat(path, &sb) == 0)
sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto err;
+ }
+
sepol_set_disable_dontaudit(sh->sepolh, 0);
+ }
return STATUS_SUCCESS;
@@ -1139,6 +1147,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
int status = 0;
int i;
char cil_path[PATH_MAX];
+ struct stat sb;
assert(sh);
assert(modinfos);
@@ -1155,9 +1164,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
}
if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0) {
continue;
}
+ if (status != 0 && errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+ goto cleanup; //an error in the "stat" call
+ }
status = semanage_compile_module(sh, &modinfos[i]);
if (status < 0) {
@@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
struct cil_db *cildb = NULL;
semanage_module_info_t *modinfos = NULL;
mode_t mask = umask(0077);
+ struct stat sb;
int do_rebuild, do_write_kernel, do_install;
int fcontexts_modified, ports_modified, seusers_modified,
@@ -1226,10 +1240,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the disable_dontaudit flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+

I am not a huge fan of this if under else block of if (errno !=ENOENT).
if (stat() == 0)
//exists
else if (errno == ENOENT)
// doesn't exist
else
//fail

Post by Vit Mojzis
do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+ }
if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1251,10 +1271,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the preserve_tunables flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
+ }
+
if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
* a rebuild.
*/
if (!do_rebuild) {
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
+ int files[] = {SEMANAGE_STORE_KERNEL,
+ SEMANAGE_STORE_FC,
+ SEMANAGE_STORE_SEUSERS,
+ SEMANAGE_LINKED,
+ SEMANAGE_SEUSERS_LINKED,
+ SEMANAGE_USERS_EXTRA_LINKED};
+
+ for (i = 0; i < (int) sizeof(files); i++) {
+ path = semanage_path(SEMANAGE_TMP, files[i]);
+ if (stat(path, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
+ do_rebuild = 1;
+ goto rebuild;
+ }
}
}
goto cleanup;
path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
goto cleanup;
pseusers->dtable->drop_cache(pseusers->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pseusers->dtable->clear(sh, pseusers->dbase);
}
path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
goto cleanup;
pusers_extra->dtable->drop_cache(pusers_extra->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pusers_extra->dtable->clear(sh, pusers_extra->dbase);
}
}
@@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
ssize_t _data_len;
char *_data;
int compressed;
+ struct stat sb;
/* get path of module */
rc = semanage_module_get_path(
@@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (access(module_path, F_OK) != 0) {
- ERR(sh, "Module does not exist: %s", module_path);
+ if (stat(module_path, &sb) != 0) {
+ ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
rc = -1;
goto cleanup;
}
@@ -1851,7 +1873,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
+ goto cleanup;
+ }
+
rc = semanage_compile_module(sh, _modinfo);
if (rc < 0) {
goto cleanup;
@@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
}
if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
*enabled = 1;
}
else {
@@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
/* set enabled/disabled status */
if (stat(fn, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
if (ret != 0) {
status = -1;
@@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
int status = 0;
int ret = 0;
int type;
+ struct stat sb;
char path[PATH_MAX];
mode_t mask = umask(0077);
@@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
goto cleanup;
}
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
ret = unlink(path);
if (ret != 0) {
ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 4bd1d651..83c188dc 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -514,6 +514,7 @@ char *semanage_conf_path(void)
{
char *semanage_conf = NULL;
int len;
+ FILE *fp = NULL;
len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
semanage_conf = calloc(len + 1, sizeof(char));
@@ -522,7 +523,10 @@ 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) {
+ //the following replaces access(semanage_conf, R_OK) check
+ if ((fp = fopen(semanage_conf, "r")) != NULL) {
+ fclose(fp);
+ } else {
snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
}
@@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
int r;
+ struct stat sb;
+
+ if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ return -1;
+ }
- if (access(path, F_OK) != 0) {
return 0;
}
--
2.14.3

--
Respectfully,
William C Roberts

[William Roberts]

Post by Stephen Smalley

Post by William Roberts
Where is patch 2/2, I have yet to see it?
Did something get screwy and is it: [PATCH] libsemanage: Improve
warning for installing disabled module

No, 2/3 was a separate patch and had the 2/3 in the subject line.
I received all three from the list, both locally and on my gmail.

Thanks gmail for folding the patch 1/3 and 2/3 into the same "conversation" or
whatever it does.

Post by Stephen Smalley

Post by William Roberts
On Wed, Feb 28, 2018 at 9:50 AM, William Roberts

Post by William Roberts

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
And access(,R_OK) by fopen(,"r")
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 132 +++++++++++++++++++++++++--------------
libsemanage/src/semanage_store.c | 14 ++++-
2 files changed, 98 insertions(+), 48 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index f4d57cf8..d42a51cd 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
int semanage_direct_connect(semanage_handle_t * sh)
{
const char *path;
+ struct stat sb;
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
@@ -302,10 +303,17 @@ int semanage_direct_connect(semanage_handle_t * sh)
/* set the disable dontaudit value */
path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ if (stat(path, &sb) == 0)
sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto err;
+ }
+
sepol_set_disable_dontaudit(sh->sepolh, 0);
+ }
return STATUS_SUCCESS;
@@ -1139,6 +1147,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
int status = 0;
int i;
char cil_path[PATH_MAX];
+ struct stat sb;
assert(sh);
assert(modinfos);
@@ -1155,9 +1164,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
}
if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0) {
continue;
}
+ if (status != 0 && errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+ goto cleanup; //an error in the "stat" call
+ }
status = semanage_compile_module(sh, &modinfos[i]);
if (status < 0) {
@@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
struct cil_db *cildb = NULL;
semanage_module_info_t *modinfos = NULL;
mode_t mask = umask(0077);
+ struct stat sb;
int do_rebuild, do_write_kernel, do_install;
int fcontexts_modified, ports_modified, seusers_modified,
@@ -1226,10 +1240,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the disable_dontaudit flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+

I am not a huge fan of this if under else block of if (errno !=ENOENT).
if (stat() == 0)
//exists
else if (errno == ENOENT)
// doesn't exist
else
//fail

Post by Vit Mojzis
do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+ }
if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1251,10 +1271,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the preserve_tunables flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
+ }
+
if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
* a rebuild.
*/
if (!do_rebuild) {
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
+ int files[] = {SEMANAGE_STORE_KERNEL,
+ SEMANAGE_STORE_FC,
+ SEMANAGE_STORE_SEUSERS,
+ SEMANAGE_LINKED,
+ SEMANAGE_SEUSERS_LINKED,
+ SEMANAGE_USERS_EXTRA_LINKED};
+
+ for (i = 0; i < (int) sizeof(files); i++) {
+ path = semanage_path(SEMANAGE_TMP, files[i]);
+ if (stat(path, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
+ do_rebuild = 1;
+ goto rebuild;
+ }
}
}
goto cleanup;
path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
goto cleanup;
pseusers->dtable->drop_cache(pseusers->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pseusers->dtable->clear(sh, pseusers->dbase);
}
path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
goto cleanup;
pusers_extra->dtable->drop_cache(pusers_extra->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pusers_extra->dtable->clear(sh, pusers_extra->dbase);
}
}
@@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
ssize_t _data_len;
char *_data;
int compressed;
+ struct stat sb;
/* get path of module */
rc = semanage_module_get_path(
@@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (access(module_path, F_OK) != 0) {
- ERR(sh, "Module does not exist: %s", module_path);
+ if (stat(module_path, &sb) != 0) {
+ ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
rc = -1;
goto cleanup;
}
@@ -1851,7 +1873,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
+ goto cleanup;
+ }
+
rc = semanage_compile_module(sh, _modinfo);
if (rc < 0) {
goto cleanup;
@@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
}
if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
*enabled = 1;
}
else {
@@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
/* set enabled/disabled status */
if (stat(fn, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
if (ret != 0) {
status = -1;
@@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
int status = 0;
int ret = 0;
int type;
+ struct stat sb;
char path[PATH_MAX];
mode_t mask = umask(0077);
@@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
goto cleanup;
}
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
ret = unlink(path);
if (ret != 0) {
ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 4bd1d651..83c188dc 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -514,6 +514,7 @@ char *semanage_conf_path(void)
{
char *semanage_conf = NULL;
int len;
+ FILE *fp = NULL;
len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
semanage_conf = calloc(len + 1, sizeof(char));
@@ -522,7 +523,10 @@ 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) {
+ //the following replaces access(semanage_conf, R_OK) check
+ if ((fp = fopen(semanage_conf, "r")) != NULL) {
+ fclose(fp);
+ } else {
snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
}
@@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
int r;
+ struct stat sb;
+
+ if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ return -1;
+ }
- if (access(path, F_OK) != 0) {
return 0;
}
--
2.14.3

[William Roberts]

So peeking through the code base, I see:

int semanage_direct_is_managed(semanage_handle_t * sh)
{
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;

if (semanage_access_check(sh) < 0)
return 0;

return 1;

err:
ERR(sh, "could not check whether policy is managed");
return STATUS_ERR;
}

Which semanage_access_check eventually gets down to a raw access check.

Which is only ever used in test_fcontext

semanage_access_check is also the only consumer of semanage_direct_access_check

which is the semanage_store_access_check is only consumed by the
former and test case and
the same could be said for semanage_store_access_check

I think this is a good time to roll in patch 4 and drop everything
relying on semanage_store_access_check.

Thoughts?

On Wed, Feb 28, 2018 at 11:07 AM, William Roberts

Post by William Roberts

Post by Stephen Smalley

Post by William Roberts
Where is patch 2/2, I have yet to see it?
Did something get screwy and is it: [PATCH] libsemanage: Improve
warning for installing disabled module

No, 2/3 was a separate patch and had the 2/3 in the subject line.
I received all three from the list, both locally and on my gmail.

Thanks gmail for folding the patch 1/3 and 2/3 into the same "conversation" or
whatever it does.

Post by Stephen Smalley

Post by William Roberts
On Wed, Feb 28, 2018 at 9:50 AM, William Roberts

Post by William Roberts

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
And access(,R_OK) by fopen(,"r")
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 132 +++++++++++++++++++++++++--------------
libsemanage/src/semanage_store.c | 14 ++++-
2 files changed, 98 insertions(+), 48 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index f4d57cf8..d42a51cd 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
int semanage_direct_connect(semanage_handle_t * sh)
{
const char *path;
+ struct stat sb;
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
@@ -302,10 +303,17 @@ int semanage_direct_connect(semanage_handle_t * sh)
/* set the disable dontaudit value */
path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ if (stat(path, &sb) == 0)
sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto err;
+ }
+
sepol_set_disable_dontaudit(sh->sepolh, 0);
+ }
return STATUS_SUCCESS;
@@ -1139,6 +1147,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
int status = 0;
int i;
char cil_path[PATH_MAX];
+ struct stat sb;
assert(sh);
assert(modinfos);
@@ -1155,9 +1164,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
}
if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0) {
continue;
}
+ if (status != 0 && errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+ goto cleanup; //an error in the "stat" call
+ }
status = semanage_compile_module(sh, &modinfos[i]);
if (status < 0) {
@@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
struct cil_db *cildb = NULL;
semanage_module_info_t *modinfos = NULL;
mode_t mask = umask(0077);
+ struct stat sb;
int do_rebuild, do_write_kernel, do_install;
int fcontexts_modified, ports_modified, seusers_modified,
@@ -1226,10 +1240,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the disable_dontaudit flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+

I am not a huge fan of this if under else block of if (errno !=ENOENT).
if (stat() == 0)
//exists
else if (errno == ENOENT)
// doesn't exist
else
//fail

Post by Vit Mojzis
do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+ }
if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1251,10 +1271,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the preserve_tunables flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
+ }
+
if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
* a rebuild.
*/
if (!do_rebuild) {
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
+ int files[] = {SEMANAGE_STORE_KERNEL,
+ SEMANAGE_STORE_FC,
+ SEMANAGE_STORE_SEUSERS,
+ SEMANAGE_LINKED,
+ SEMANAGE_SEUSERS_LINKED,
+ SEMANAGE_USERS_EXTRA_LINKED};
+
+ for (i = 0; i < (int) sizeof(files); i++) {
+ path = semanage_path(SEMANAGE_TMP, files[i]);
+ if (stat(path, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
+ do_rebuild = 1;
+ goto rebuild;
+ }
}
}
goto cleanup;
path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
goto cleanup;
pseusers->dtable->drop_cache(pseusers->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pseusers->dtable->clear(sh, pseusers->dbase);
}
path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
goto cleanup;
pusers_extra->dtable->drop_cache(pusers_extra->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pusers_extra->dtable->clear(sh, pusers_extra->dbase);
}
}
@@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
ssize_t _data_len;
char *_data;
int compressed;
+ struct stat sb;
/* get path of module */
rc = semanage_module_get_path(
@@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (access(module_path, F_OK) != 0) {
- ERR(sh, "Module does not exist: %s", module_path);
+ if (stat(module_path, &sb) != 0) {
+ ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
rc = -1;
goto cleanup;
}
@@ -1851,7 +1873,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
+ goto cleanup;
+ }
+
rc = semanage_compile_module(sh, _modinfo);
if (rc < 0) {
goto cleanup;
@@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
}
if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
*enabled = 1;
}
else {
@@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
/* set enabled/disabled status */
if (stat(fn, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
if (ret != 0) {
status = -1;
@@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
int status = 0;
int ret = 0;
int type;
+ struct stat sb;
char path[PATH_MAX];
mode_t mask = umask(0077);
@@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
goto cleanup;
}
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
ret = unlink(path);
if (ret != 0) {
ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 4bd1d651..83c188dc 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -514,6 +514,7 @@ char *semanage_conf_path(void)
{
char *semanage_conf = NULL;
int len;
+ FILE *fp = NULL;
len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
semanage_conf = calloc(len + 1, sizeof(char));
@@ -522,7 +523,10 @@ 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) {
+ //the following replaces access(semanage_conf, R_OK) check
+ if ((fp = fopen(semanage_conf, "r")) != NULL) {
+ fclose(fp);
+ } else {
snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
}
@@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
int r;
+ struct stat sb;
+
+ if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ return -1;
+ }
- if (access(path, F_OK) != 0) {
return 0;
}
--
2.14.3

[Stephen Smalley]

Post by Vit Mojzis
int semanage_direct_is_managed(semanage_handle_t * sh)
{
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
if (semanage_access_check(sh) < 0)
return 0;
return 1;
ERR(sh, "could not check whether policy is managed");
return STATUS_ERR;
}
Which semanage_access_check eventually gets down to a raw access check.
Which is only ever used in test_fcontext
semanage_access_check is also the only consumer of semanage_direct_access_check
which is the semanage_store_access_check is only consumed by the
former and test case and
the same could be said for semanage_store_access_check
I think this is a good time to roll in patch 4 and drop everything
relying on semanage_store_access_check.
Thoughts?

semanage_access_check() is part of the shared library ABI. Can't be
removed. Used by seobject.py via the python bindings. At most, we can
kill all internal users but the ABI has to remain.

Post by Vit Mojzis
On Wed, Feb 28, 2018 at 11:07 AM, William Roberts

Post by William Roberts

Post by Stephen Smalley

Post by William Roberts
Where is patch 2/2, I have yet to see it?
Did something get screwy and is it: [PATCH] libsemanage: Improve
warning for installing disabled module

No, 2/3 was a separate patch and had the 2/3 in the subject line.
I received all three from the list, both locally and on my gmail.

Thanks gmail for folding the patch 1/3 and 2/3 into the same "conversation" or
whatever it does.

Post by Stephen Smalley

Post by William Roberts
On Wed, Feb 28, 2018 at 9:50 AM, William Roberts

Post by William Roberts

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
And access(,R_OK) by fopen(,"r")
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 132 +++++++++++++++++++++++++--------------
libsemanage/src/semanage_store.c | 14 ++++-
2 files changed, 98 insertions(+), 48 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index f4d57cf8..d42a51cd 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
int semanage_direct_connect(semanage_handle_t * sh)
{
const char *path;
+ struct stat sb;
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
@@ -302,10 +303,17 @@ int semanage_direct_connect(semanage_handle_t * sh)
/* set the disable dontaudit value */
path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ if (stat(path, &sb) == 0)
sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto err;
+ }
+
sepol_set_disable_dontaudit(sh->sepolh, 0);
+ }
return STATUS_SUCCESS;
@@ -1139,6 +1147,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
int status = 0;
int i;
char cil_path[PATH_MAX];
+ struct stat sb;
assert(sh);
assert(modinfos);
@@ -1155,9 +1164,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
}
if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0) {
continue;
}
+ if (status != 0 && errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+ goto cleanup; //an error in the "stat" call
+ }
status = semanage_compile_module(sh, &modinfos[i]);
if (status < 0) {
@@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
struct cil_db *cildb = NULL;
semanage_module_info_t *modinfos = NULL;
mode_t mask = umask(0077);
+ struct stat sb;
int do_rebuild, do_write_kernel, do_install;
int fcontexts_modified, ports_modified, seusers_modified,
@@ -1226,10 +1240,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the disable_dontaudit flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+

I am not a huge fan of this if under else block of if (errno !=ENOENT).
if (stat() == 0)
//exists
else if (errno == ENOENT)
// doesn't exist
else
//fail

Post by Vit Mojzis
do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+ }
if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1251,10 +1271,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the preserve_tunables flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
+ }
+
if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
* a rebuild.
*/
if (!do_rebuild) {
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
+ int files[] = {SEMANAGE_STORE_KERNEL,
+ SEMANAGE_STORE_FC,
+ SEMANAGE_STORE_SEUSERS,
+ SEMANAGE_LINKED,
+ SEMANAGE_SEUSERS_LINKED,
+ SEMANAGE_USERS_EXTRA_LINKED};
+
+ for (i = 0; i < (int) sizeof(files); i++) {
+ path = semanage_path(SEMANAGE_TMP, files[i]);
+ if (stat(path, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
+ do_rebuild = 1;
+ goto rebuild;
+ }
}
}
goto cleanup;
path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
goto cleanup;
pseusers->dtable->drop_cache(pseusers->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pseusers->dtable->clear(sh, pseusers->dbase);
}
path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
goto cleanup;
pusers_extra->dtable->drop_cache(pusers_extra->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pusers_extra->dtable->clear(sh, pusers_extra->dbase);
}
}
@@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
ssize_t _data_len;
char *_data;
int compressed;
+ struct stat sb;
/* get path of module */
rc = semanage_module_get_path(
@@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (access(module_path, F_OK) != 0) {
- ERR(sh, "Module does not exist: %s", module_path);
+ if (stat(module_path, &sb) != 0) {
+ ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
rc = -1;
goto cleanup;
}
@@ -1851,7 +1873,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
+ goto cleanup;
+ }
+
rc = semanage_compile_module(sh, _modinfo);
if (rc < 0) {
goto cleanup;
@@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
}
if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
*enabled = 1;
}
else {
@@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
/* set enabled/disabled status */
if (stat(fn, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
if (ret != 0) {
status = -1;
@@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
int status = 0;
int ret = 0;
int type;
+ struct stat sb;
char path[PATH_MAX];
mode_t mask = umask(0077);
@@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
goto cleanup;
}
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
ret = unlink(path);
if (ret != 0) {
ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 4bd1d651..83c188dc 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -514,6 +514,7 @@ char *semanage_conf_path(void)
{
char *semanage_conf = NULL;
int len;
+ FILE *fp = NULL;
len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
semanage_conf = calloc(len + 1, sizeof(char));
@@ -522,7 +523,10 @@ 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) {
+ //the following replaces access(semanage_conf, R_OK) check
+ if ((fp = fopen(semanage_conf, "r")) != NULL) {
+ fclose(fp);
+ } else {
snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
}
@@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
int r;
+ struct stat sb;
+
+ if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ return -1;
+ }
- if (access(path, F_OK) != 0) {
return 0;
}
--
2.14.3

[William Roberts]

Post by Stephen Smalley

Post by Vit Mojzis
int semanage_direct_is_managed(semanage_handle_t * sh)
{
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
if (semanage_access_check(sh) < 0)
return 0;
return 1;
ERR(sh, "could not check whether policy is managed");
return STATUS_ERR;
}
Which semanage_access_check eventually gets down to a raw access check.
Which is only ever used in test_fcontext
semanage_access_check is also the only consumer of semanage_direct_access_check
which is the semanage_store_access_check is only consumed by the
former and test case and
the same could be said for semanage_store_access_check
I think this is a good time to roll in patch 4 and drop everything
relying on semanage_store_access_check.
Thoughts?

semanage_access_check() is part of the shared library ABI. Can't be
removed. Used by seobject.py via the python bindings. At most, we can
kill all internal users but the ABI has to remain.

Ahh yes, duh.

Outside of just killing off internal users of it, should we modify it
to not use access?
So it at least works under setuid?

Post by Stephen Smalley

Post by Vit Mojzis
On Wed, Feb 28, 2018 at 11:07 AM, William Roberts

Post by William Roberts

Post by Stephen Smalley

Post by William Roberts
Where is patch 2/2, I have yet to see it?
Did something get screwy and is it: [PATCH] libsemanage: Improve
warning for installing disabled module

No, 2/3 was a separate patch and had the 2/3 in the subject line.
I received all three from the list, both locally and on my gmail.

Thanks gmail for folding the patch 1/3 and 2/3 into the same "conversation" or
whatever it does.

Post by Stephen Smalley

Post by William Roberts
On Wed, Feb 28, 2018 at 9:50 AM, William Roberts

Post by William Roberts

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
And access(,R_OK) by fopen(,"r")
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 132 +++++++++++++++++++++++++--------------
libsemanage/src/semanage_store.c | 14 ++++-
2 files changed, 98 insertions(+), 48 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index f4d57cf8..d42a51cd 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
int semanage_direct_connect(semanage_handle_t * sh)
{
const char *path;
+ struct stat sb;
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
@@ -302,10 +303,17 @@ int semanage_direct_connect(semanage_handle_t * sh)
/* set the disable dontaudit value */
path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ if (stat(path, &sb) == 0)
sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto err;
+ }
+
sepol_set_disable_dontaudit(sh->sepolh, 0);
+ }
return STATUS_SUCCESS;
@@ -1139,6 +1147,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
int status = 0;
int i;
char cil_path[PATH_MAX];
+ struct stat sb;
assert(sh);
assert(modinfos);
@@ -1155,9 +1164,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
}
if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0) {
continue;
}
+ if (status != 0 && errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+ goto cleanup; //an error in the "stat" call
+ }
status = semanage_compile_module(sh, &modinfos[i]);
if (status < 0) {
@@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
struct cil_db *cildb = NULL;
semanage_module_info_t *modinfos = NULL;
mode_t mask = umask(0077);
+ struct stat sb;
int do_rebuild, do_write_kernel, do_install;
int fcontexts_modified, ports_modified, seusers_modified,
@@ -1226,10 +1240,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the disable_dontaudit flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+

I am not a huge fan of this if under else block of if (errno !=ENOENT).
if (stat() == 0)
//exists
else if (errno == ENOENT)
// doesn't exist
else
//fail

Post by Vit Mojzis
do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+ }
if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1251,10 +1271,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the preserve_tunables flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
+ }
+
if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
* a rebuild.
*/
if (!do_rebuild) {
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
+ int files[] = {SEMANAGE_STORE_KERNEL,
+ SEMANAGE_STORE_FC,
+ SEMANAGE_STORE_SEUSERS,
+ SEMANAGE_LINKED,
+ SEMANAGE_SEUSERS_LINKED,
+ SEMANAGE_USERS_EXTRA_LINKED};
+
+ for (i = 0; i < (int) sizeof(files); i++) {
+ path = semanage_path(SEMANAGE_TMP, files[i]);
+ if (stat(path, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
+ do_rebuild = 1;
+ goto rebuild;
+ }
}
}
goto cleanup;
path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
goto cleanup;
pseusers->dtable->drop_cache(pseusers->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pseusers->dtable->clear(sh, pseusers->dbase);
}
path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
goto cleanup;
pusers_extra->dtable->drop_cache(pusers_extra->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pusers_extra->dtable->clear(sh, pusers_extra->dbase);
}
}
@@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
ssize_t _data_len;
char *_data;
int compressed;
+ struct stat sb;
/* get path of module */
rc = semanage_module_get_path(
@@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (access(module_path, F_OK) != 0) {
- ERR(sh, "Module does not exist: %s", module_path);
+ if (stat(module_path, &sb) != 0) {
+ ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
rc = -1;
goto cleanup;
}
@@ -1851,7 +1873,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
+ goto cleanup;
+ }
+
rc = semanage_compile_module(sh, _modinfo);
if (rc < 0) {
goto cleanup;
@@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
}
if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
*enabled = 1;
}
else {
@@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
/* set enabled/disabled status */
if (stat(fn, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
if (ret != 0) {
status = -1;
@@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
int status = 0;
int ret = 0;
int type;
+ struct stat sb;
char path[PATH_MAX];
mode_t mask = umask(0077);
@@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
goto cleanup;
}
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
ret = unlink(path);
if (ret != 0) {
ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 4bd1d651..83c188dc 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -514,6 +514,7 @@ char *semanage_conf_path(void)
{
char *semanage_conf = NULL;
int len;
+ FILE *fp = NULL;
len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
semanage_conf = calloc(len + 1, sizeof(char));
@@ -522,7 +523,10 @@ 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) {
+ //the following replaces access(semanage_conf, R_OK) check
+ if ((fp = fopen(semanage_conf, "r")) != NULL) {
+ fclose(fp);
+ } else {
snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
}
@@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
int r;
+ struct stat sb;
+
+ if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ return -1;
+ }
- if (access(path, F_OK) != 0) {
return 0;
}
--
2.14.3

[Stephen Smalley]

Post by William Roberts

Post by Stephen Smalley

Post by Vit Mojzis
int semanage_direct_is_managed(semanage_handle_t * sh)
{
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
if (semanage_access_check(sh) < 0)
return 0;
return 1;
ERR(sh, "could not check whether policy is managed");
return STATUS_ERR;
}
Which semanage_access_check eventually gets down to a raw access check.
Which is only ever used in test_fcontext
semanage_access_check is also the only consumer of semanage_direct_access_check
which is the semanage_store_access_check is only consumed by the
former and test case and
the same could be said for semanage_store_access_check
I think this is a good time to roll in patch 4 and drop everything
relying on semanage_store_access_check.
Thoughts?

semanage_access_check() is part of the shared library ABI. Can't be
removed. Used by seobject.py via the python bindings. At most, we can
kill all internal users but the ABI has to remain.

Ahh yes, duh.
Outside of just killing off internal users of it, should we modify it
to not use access?
So it at least works under setuid?

I don't think it is worthwhile, and it isn't clear how one would test
the directory writability case. Plus semanage_access_check() is only
advisory and the caller is free to ignore the result or not call it in
the first place. It is really only intended to allow a program to save
itself some work if it isn't going to be able to access the policy store
at all, not as a security feature. Also, as I've said before, I
wouldn't warrant libsemanage to be safe if called from a setuid program;
no one has ever audited it for such to my knowledge.

[Stephen Smalley]

Post by Stephen Smalley

Post by William Roberts

Post by Stephen Smalley

Post by Vit Mojzis
int semanage_direct_is_managed(semanage_handle_t * sh)
{
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
if (semanage_access_check(sh) < 0)
return 0;
return 1;
ERR(sh, "could not check whether policy is managed");
return STATUS_ERR;
}
Which semanage_access_check eventually gets down to a raw access check.
Which is only ever used in test_fcontext
semanage_access_check is also the only consumer of semanage_direct_access_check
which is the semanage_store_access_check is only consumed by the
former and test case and
the same could be said for semanage_store_access_check
I think this is a good time to roll in patch 4 and drop everything
relying on semanage_store_access_check.
Thoughts?

semanage_access_check() is part of the shared library ABI. Can't be
removed. Used by seobject.py via the python bindings. At most, we can
kill all internal users but the ABI has to remain.

Ahh yes, duh.
Outside of just killing off internal users of it, should we modify it
to not use access?
So it at least works under setuid?

I don't think it is worthwhile, and it isn't clear how one would test
the directory writability case. Plus semanage_access_check() is only
advisory and the caller is free to ignore the result or not call it in
the first place. It is really only intended to allow a program to save
itself some work if it isn't going to be able to access the policy store
at all, not as a security feature. Also, as I've said before, I
wouldn't warrant libsemanage to be safe if called from a setuid program;
no one has ever audited it for such to my knowledge.

By the way, please test that these patches do not end up yielding silent
failures or confusing error messages when users run semanage or semodule
commands with insufficient permissions, e.g. semanage login -l or
semodule -l as non-root. That's what semanage_access_check() was for,
to check up front and provide a useful error message before doing any
other work.

[Stephen Smalley]

Post by Stephen Smalley

Post by Stephen Smalley

Post by William Roberts

Post by Stephen Smalley

Post by Vit Mojzis
int semanage_direct_is_managed(semanage_handle_t * sh)
{
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
if (semanage_access_check(sh) < 0)
return 0;
return 1;
ERR(sh, "could not check whether policy is managed");
return STATUS_ERR;
}
Which semanage_access_check eventually gets down to a raw access check.
Which is only ever used in test_fcontext
semanage_access_check is also the only consumer of semanage_direct_access_check
which is the semanage_store_access_check is only consumed by the
former and test case and
the same could be said for semanage_store_access_check
I think this is a good time to roll in patch 4 and drop everything
relying on semanage_store_access_check.
Thoughts?

semanage_access_check() is part of the shared library ABI. Can't be
removed. Used by seobject.py via the python bindings. At most, we can
kill all internal users but the ABI has to remain.

Ahh yes, duh.
Outside of just killing off internal users of it, should we modify it
to not use access?
So it at least works under setuid?

I don't think it is worthwhile, and it isn't clear how one would test
the directory writability case. Plus semanage_access_check() is only
advisory and the caller is free to ignore the result or not call it in
the first place. It is really only intended to allow a program to save
itself some work if it isn't going to be able to access the policy store
at all, not as a security feature. Also, as I've said before, I
wouldn't warrant libsemanage to be safe if called from a setuid program;
no one has ever audited it for such to my knowledge.

By the way, please test that these patches do not end up yielding silent
failures or confusing error messages when users run semanage or semodule
commands with insufficient permissions, e.g. semanage login -l or
semodule -l as non-root. That's what semanage_access_check() was for,
to check up front and provide a useful error message before doing any
other work.

And for comparison, this is what we get today before these patches:
$ semanage login -l
ValueError: SELinux policy is not managed or store cannot be accessed.

$ semodule -l
libsemanage.semanage_create_store: Could not access module store at
/var/lib/selinux/targeted, or it is not a directory. (Permission denied).
libsemanage.semanage_direct_connect: could not establish direct
connection (Permission denied).
semodule: Could not connect to policy handler

The messages don't have to be the same, but we need to make sure we have
some useful diagnostic that points to the underlying problem.

[Stephen Smalley]

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
And access(,R_OK) by fopen(,"r")
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 132 +++++++++++++++++++++++++--------------
libsemanage/src/semanage_store.c | 14 ++++-
2 files changed, 98 insertions(+), 48 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index f4d57cf8..d42a51cd 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -140,6 +140,7 @@ int semanage_direct_is_managed(semanage_handle_t * sh)
int semanage_direct_connect(semanage_handle_t * sh)
{
const char *path;
+ struct stat sb;
if (semanage_check_init(sh, sh->conf->store_root_path))
goto err;
@@ -302,10 +303,17 @@ int semanage_direct_connect(semanage_handle_t * sh)
/* set the disable dontaudit value */
path = semanage_path(SEMANAGE_ACTIVE, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ if (stat(path, &sb) == 0)
sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto err;
+ }
+
sepol_set_disable_dontaudit(sh->sepolh, 0);
+ }
return STATUS_SUCCESS;
@@ -1139,6 +1147,7 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
int status = 0;
int i;
char cil_path[PATH_MAX];
+ struct stat sb;
assert(sh);
assert(modinfos);
@@ -1155,9 +1164,13 @@ static int semanage_compile_hll_modules(semanage_handle_t *sh,
}
if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0) {
continue;
}
+ if (status != 0 && errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", cil_path, strerror(errno));
+ goto cleanup; //an error in the "stat" call
+ }
status = semanage_compile_module(sh, &modinfos[i]);
if (status < 0) {
@@ -1188,6 +1201,7 @@ static int semanage_direct_commit(semanage_handle_t * sh)
struct cil_db *cildb = NULL;
semanage_module_info_t *modinfos = NULL;
mode_t mask = umask(0077);
+ struct stat sb;
int do_rebuild, do_write_kernel, do_install;
int fcontexts_modified, ports_modified, seusers_modified,
@@ -1226,10 +1240,16 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the disable_dontaudit flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_disable_dontaudit(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
+ }
if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1251,10 +1271,17 @@ static int semanage_direct_commit(semanage_handle_t * sh)
/* Create or remove the preserve_tunables flag file. */
path = semanage_path(SEMANAGE_TMP, SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
do_rebuild |= !(sepol_get_preserve_tunables(sh->sepolh) == 1);
- else
+ else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
+ }
+
if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
FILE *touch;
touch = fopen(path, "w");
@@ -1291,40 +1318,24 @@ static int semanage_direct_commit(semanage_handle_t * sh)
* a rebuild.
*/
if (!do_rebuild) {
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
-
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
- }
+ int files[] = {SEMANAGE_STORE_KERNEL,
+ SEMANAGE_STORE_FC,
+ SEMANAGE_STORE_SEUSERS,
+ SEMANAGE_LINKED,
+ SEMANAGE_SEUSERS_LINKED,
+ SEMANAGE_USERS_EXTRA_LINKED};
+
+ for (i = 0; i < (int) sizeof(files); i++) {
+ path = semanage_path(SEMANAGE_TMP, files[i]);
+ if (stat(path, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
- path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
- do_rebuild = 1;
- goto rebuild;
+ do_rebuild = 1;
+ goto rebuild;
+ }
}
}
goto cleanup;
path = semanage_path(SEMANAGE_TMP, SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
goto cleanup;
pseusers->dtable->drop_cache(pseusers->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pseusers->dtable->clear(sh, pseusers->dbase);
}
path = semanage_path(SEMANAGE_TMP, SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
retval = semanage_copy_file(path,
semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA),
goto cleanup;
pusers_extra->dtable->drop_cache(pusers_extra->dbase);
} else {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ goto cleanup;
+ }
+
pusers_extra->dtable->clear(sh, pusers_extra->dbase);
}
}
@@ -1809,6 +1830,7 @@ static int semanage_direct_extract(semanage_handle_t * sh,
ssize_t _data_len;
char *_data;
int compressed;
+ struct stat sb;
/* get path of module */
rc = semanage_module_get_path(
@@ -1821,8 +1843,8 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (access(module_path, F_OK) != 0) {
- ERR(sh, "Module does not exist: %s", module_path);
+ if (stat(module_path, &sb) != 0) {
+ ERR(sh, "Unable to access %s: %s\n", module_path, strerror(errno));
rc = -1;
goto cleanup;
}
@@ -1851,7 +1873,12 @@ static int semanage_direct_extract(semanage_handle_t * sh,
goto cleanup;
}
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") && stat(input_file, &sb) != 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", input_file, strerror(errno));
+ goto cleanup;
+ }
+
rc = semanage_compile_module(sh, _modinfo);
if (rc < 0) {
goto cleanup;
@@ -1996,6 +2023,12 @@ static int semanage_direct_get_enabled(semanage_handle_t *sh,
}
if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
*enabled = 1;
}
else {
@@ -2322,6 +2355,12 @@ static int semanage_direct_get_module_info(semanage_handle_t *sh,
/* set enabled/disabled status */
if (stat(fn, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", fn, strerror(errno));
+ status = -1;
+ goto cleanup;
+ }
+
ret = semanage_module_info_set_enabled(sh, *modinfo, 1);
if (ret != 0) {
status = -1;
@@ -2730,6 +2769,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
int status = 0;
int ret = 0;
int type;
+ struct stat sb;
char path[PATH_MAX];
mode_t mask = umask(0077);
@@ -2830,7 +2870,7 @@ static int semanage_direct_install_info(semanage_handle_t *sh,
goto cleanup;
}
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
ret = unlink(path);
if (ret != 0) {
ERR(sh, "Error while removing cached CIL file %s: %s", path, strerror(errno));
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 4bd1d651..83c188dc 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -514,6 +514,7 @@ char *semanage_conf_path(void)
{
char *semanage_conf = NULL;
int len;
+ FILE *fp = NULL;
len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
semanage_conf = calloc(len + 1, sizeof(char));
@@ -522,7 +523,10 @@ 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) {
+ //the following replaces access(semanage_conf, R_OK) check
+ if ((fp = fopen(semanage_conf, "r")) != NULL) {

Arguably the test should have just been F_OK (i.e. does it exist) and
you can just make it a stat() instead.

Post by Vit Mojzis
+ fclose(fp);
+ } else {
snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
}
@@ -1508,8 +1512,14 @@ int semanage_split_fc(semanage_handle_t * sh)
static int sefcontext_compile(semanage_handle_t * sh, const char *path) {
int r;
+ struct stat sb;
+
+ if (stat(path, &sb) < 0) {
+ if (errno != ENOENT) {
+ ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
+ return -1;
+ }
- if (access(path, F_OK) != 0) {
return 0;
}

[William Roberts]

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs. Remove redundant access() checks
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 7 -------
libsemanage/src/semanage_store.c | 17 ++++++++---------
2 files changed, 8 insertions(+), 16 deletions(-)
diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index 88873c43..b7899d68 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -148,9 +148,6 @@ int semanage_direct_connect(semanage_handle_t * sh)
if (semanage_create_store(sh, 1))
goto err;
- if (semanage_access_check(sh) < SEMANAGE_CAN_READ)
- goto err;
-
sh->u.direct.translock_file_fd = -1;
sh->u.direct.activelock_file_fd = -1;
@@ -398,10 +395,6 @@ static int semanage_direct_disconnect(semanage_handle_t *sh)
static int semanage_direct_begintrans(semanage_handle_t * sh)
{
-
- if (semanage_access_check(sh) != SEMANAGE_CAN_WRITE) {
- return -1;
- }
if (semanage_get_trans_lock(sh) < 0) {
return -1;
}
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 936e6495..4bd1d651 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -538,7 +538,6 @@ char *semanage_conf_path(void)
int semanage_create_store(semanage_handle_t * sh, int create)
{
struct stat sb;
- int mode_mask = R_OK | W_OK | X_OK;
const char *path = semanage_files[SEMANAGE_ROOT];
int fd;
@@ -557,9 +556,9 @@ 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)) {
ERR(sh,
- "Could not access module store at %s, or it is not a directory.",
+ "Module store at %s is not a directory.",
path);
return -1;
}
@@ -580,9 +579,9 @@ 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)) {
ERR(sh,
- "Could not access module store active subdirectory at %s, or it is not a directory.",
+ "Module store active subdirectory at %s is not a directory.",
path);
return -1;
}
@@ -603,9 +602,9 @@ 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)) {
ERR(sh,
- "Could not access module store active modules subdirectory at %s, or it is not a directory.",
+ "Module store active modules subdirectory at %s is not a directory.",
path);
return -1;
}
@@ -624,8 +623,8 @@ 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) {
- ERR(sh, "Could not access lock file at %s.", path);
+ if (!S_ISREG(sb.st_mode)) {
+ ERR(sh, "Object at %s is not a lock file.", path);
return -1;
}
}
--
2.14.3

Tenative ack on testing. This routine semanage_create_store() has some
crazy indenting,
a lot of that could be organized to be way-less horizontal.

[Stephen Smalley]

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431

Also, even with all three patches applied, there are still a number of
residual access() calls within semanage_store.c. Is that intentional?

Post by Vit Mojzis
---
 libsemanage/src/direct_api.c | 50 +++++++++++++++++++++++++++++-----
----------
 1 file changed, 33 insertions(+), 17 deletions(-)
diff --git a/libsemanage/src/direct_api.c
b/libsemanage/src/direct_api.c
index ed11a7c..3d1c354 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -296,10 +296,19 @@ int semanage_direct_connect(semanage_handle_t * sh)
 
  /* set the disable dontaudit value */
  path = semanage_path(SEMANAGE_ACTIVE,
SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ struct stat sb;
+ if (stat(path, &sb) == 0)
  sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
- sepol_set_disable_dontaudit(sh->sepolh, 0);
+ else{
+ if (errno == ENOENT){
+ sepol_set_disable_dontaudit(sh->sepolh, 0);
+ }
+ else{
+ ERR(sh, "Unable to access disable_dontaudit
file at %s: %s\n",path , strerror(errno));
+ goto err;
+ }
+ }
 
  return STATUS_SUCCESS;
 
@@ -1114,6 +1123,7 @@ static int
semanage_compile_hll_modules(semanage_handle_t *sh,
  int status = 0;
  int i;
  char cil_path[PATH_MAX];
+ struct stat sb;
 
  assert(sh);
  assert(modinfos);
@@ -1130,9 +1140,12 @@ static int
semanage_compile_hll_modules(semanage_handle_t *sh,
  }
 
  if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0)
{
  continue;
  }
+ if (status != 0 && status != ENOENT) {
+ goto cleanup; //an error in the "stat" call
+ }
 
  status = semanage_compile_module(sh, &modinfos[i]);
  if (status < 0) {
@@ -1200,7 +1213,8 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
 
  /* Create or remove the disable_dontaudit flag file. */
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ struct stat sb;
+ if (stat(path, &sb) == 0)
  do_rebuild |= !(sepol_get_disable_dontaudit(sh-

Post by Vit Mojzis
sepolh) == 1);

  else
  do_rebuild |= (sepol_get_disable_dontaudit(sh-

Post by Vit Mojzis
sepolh) == 1);

@@ -1225,7 +1239,7 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
 
  /* Create or remove the preserve_tunables flag file. */
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
  do_rebuild |= !(sepol_get_preserve_tunables(sh-

Post by Vit Mojzis
sepolh) == 1);

  else
  do_rebuild |= (sepol_get_preserve_tunables(sh-

Post by Vit Mojzis
sepolh) == 1);

@@ -1266,37 +1280,37 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
   */
  if (!do_rebuild) {
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
  goto cleanup;
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
  retval = semanage_copy_file(path,
      semanage_path(SE
MANAGE_TMP,
    SE
MANAGE_STORE_SEUSERS),
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
  retval = semanage_copy_file(path,
      semanage_path(SE
MANAGE_TMP,
    SE
MANAGE_USERS_EXTRA),
@@ -1785,7 +1799,8 @@ static int
semanage_direct_extract(semanage_handle_t * sh,
  goto cleanup;
  }
 
- if (access(module_path, F_OK) != 0) {
+ struct stat sb;
+ if (stat(module_path, &sb) != 0) {
  ERR(sh, "Module does not exist: %s", module_path);
  rc = -1;
  goto cleanup;
@@ -1815,7 +1830,7 @@ static int
semanage_direct_extract(semanage_handle_t * sh,
  goto cleanup;
  }
 
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") &&
access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") &&
stat(input_file, &sb) != 0) {
  rc = semanage_compile_module(sh, _modinfo);
  if (rc < 0) {
  goto cleanup;
@@ -2790,7 +2805,8 @@ static int
semanage_direct_install_info(semanage_handle_t *sh,
  goto cleanup;
  }
 
- if (access(path, F_OK) == 0) {
+ struct stat sb;
+ if (stat(path, &sb) == 0) {
  ret = unlink(path);
  if (ret != 0) {
  ERR(sh, "Error while removing cached
CIL file %s: %s", path, strerror(errno));

[Stephen Smalley]

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs.
Replace access(,F_OK) (i.e. tests for file existence) by stat().
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431

Did you decide to not pursue this further? I didn't see any follow-up
since my last set of comments in June.

Post by Vit Mojzis
---
 libsemanage/src/direct_api.c | 50 +++++++++++++++++++++++++++++-----
----------
 1 file changed, 33 insertions(+), 17 deletions(-)
diff --git a/libsemanage/src/direct_api.c
b/libsemanage/src/direct_api.c
index ed11a7c..3d1c354 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -296,10 +296,19 @@ int semanage_direct_connect(semanage_handle_t * sh)
 
  /* set the disable dontaudit value */
  path = semanage_path(SEMANAGE_ACTIVE,
SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+
+ struct stat sb;
+ if (stat(path, &sb) == 0)
  sepol_set_disable_dontaudit(sh->sepolh, 1);
- else
- sepol_set_disable_dontaudit(sh->sepolh, 0);
+ else{
+ if (errno == ENOENT){
+ sepol_set_disable_dontaudit(sh->sepolh, 0);
+ }
+ else{
+ ERR(sh, "Unable to access disable_dontaudit
file at %s: %s\n",path , strerror(errno));
+ goto err;
+ }
+ }
 
  return STATUS_SUCCESS;
 
@@ -1114,6 +1123,7 @@ static int
semanage_compile_hll_modules(semanage_handle_t *sh,
  int status = 0;
  int i;
  char cil_path[PATH_MAX];
+ struct stat sb;
 
  assert(sh);
  assert(modinfos);
@@ -1130,9 +1140,12 @@ static int
semanage_compile_hll_modules(semanage_handle_t *sh,
  }
 
  if (semanage_get_ignore_module_cache(sh) == 0 &&
- access(cil_path, F_OK) == 0) {
+ (status = stat(cil_path, &sb)) == 0)
{
  continue;
  }
+ if (status != 0 && status != ENOENT) {
+ goto cleanup; //an error in the "stat" call
+ }
 
  status = semanage_compile_module(sh, &modinfos[i]);
  if (status < 0) {
@@ -1200,7 +1213,8 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
 
  /* Create or remove the disable_dontaudit flag file. */
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_DISABLE_DONTAUDIT);
- if (access(path, F_OK) == 0)
+ struct stat sb;
+ if (stat(path, &sb) == 0)
  do_rebuild |= !(sepol_get_disable_dontaudit(sh-

Post by Vit Mojzis
sepolh) == 1);

  else
  do_rebuild |= (sepol_get_disable_dontaudit(sh-

Post by Vit Mojzis
sepolh) == 1);

@@ -1225,7 +1239,7 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
 
  /* Create or remove the preserve_tunables flag file. */
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_PRESERVE_TUNABLES);
- if (access(path, F_OK) == 0)
+ if (stat(path, &sb) == 0)
  do_rebuild |= !(sepol_get_preserve_tunables(sh-

Post by Vit Mojzis
sepolh) == 1);

  else
  do_rebuild |= (sepol_get_preserve_tunables(sh-

Post by Vit Mojzis
sepolh) == 1);

@@ -1266,37 +1280,37 @@ static int
semanage_direct_commit(semanage_handle_t * sh)
   */
  if (!do_rebuild) {
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_KERNEL);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_FC);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP, SEMANAGE_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) != 0) {
+ if (stat(path, &sb) != 0) {
  do_rebuild = 1;
  goto rebuild;
  }
  goto cleanup;
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_SEUSERS_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
  retval = semanage_copy_file(path,
      semanage_path(SE
MANAGE_TMP,
    SE
MANAGE_STORE_SEUSERS),
  }
 
  path = semanage_path(SEMANAGE_TMP,
SEMANAGE_USERS_EXTRA_LINKED);
- if (access(path, F_OK) == 0) {
+ if (stat(path, &sb) == 0) {
  retval = semanage_copy_file(path,
      semanage_path(SE
MANAGE_TMP,
    SE
MANAGE_USERS_EXTRA),
@@ -1785,7 +1799,8 @@ static int
semanage_direct_extract(semanage_handle_t * sh,
  goto cleanup;
  }
 
- if (access(module_path, F_OK) != 0) {
+ struct stat sb;
+ if (stat(module_path, &sb) != 0) {
  ERR(sh, "Module does not exist: %s", module_path);
  rc = -1;
  goto cleanup;
@@ -1815,7 +1830,7 @@ static int
semanage_direct_extract(semanage_handle_t * sh,
  goto cleanup;
  }
 
- if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") &&
access(input_file, F_OK) != 0) {
+ if (extract_cil == 1 && strcmp(_modinfo->lang_ext, "cil") &&
stat(input_file, &sb) != 0) {
  rc = semanage_compile_module(sh, _modinfo);
  if (rc < 0) {
  goto cleanup;
@@ -2790,7 +2805,8 @@ static int
semanage_direct_install_info(semanage_handle_t *sh,
  goto cleanup;
  }
 
- if (access(path, F_OK) == 0) {
+ struct stat sb;
+ if (stat(path, &sb) == 0) {
  ret = unlink(path);
  if (ret != 0) {
  ERR(sh, "Error while removing cached
CIL file %s: %s", path, strerror(errno));

[Vit Mojzis]

access() uses real UID instead of effective UID which causes false
negative checks in setuid programs (except for F_OK which works
properly). fopen() return values are always checked, which makes
access() checks redundant.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 3 ---
libsemanage/src/semanage_store.c | 17 ++++++++---------
2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index f4b0416..dc46f8f 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -146,9 +146,6 @@ int semanage_direct_connect(semanage_handle_t * sh)
if (semanage_create_store(sh, 1))
goto err;

- if (semanage_access_check(sh) < SEMANAGE_CAN_READ)
- goto err;
-
sh->u.direct.translock_file_fd = -1;
sh->u.direct.activelock_file_fd = -1;

diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 6b75002..0a10032 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -535,7 +535,6 @@ char *semanage_conf_path(void)
int semanage_create_store(semanage_handle_t * sh, int create)
{
struct stat sb;
- int mode_mask = R_OK | W_OK | X_OK;
const char *path = semanage_files[SEMANAGE_ROOT];
int fd;

@@ -554,9 +553,9 @@ 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)) {
ERR(sh,
- "Could not access module store at %s, or it is not a directory.",
+ "Module store at %s is not a directory.",
path);
return -1;
}
@@ -577,9 +576,9 @@ 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)) {
ERR(sh,
- "Could not access module store active subdirectory at %s, or it is not a directory.",
+ "Module store active subdirectory at %s is not a directory.",
path);
return -1;
}
@@ -600,9 +599,9 @@ 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)) {
ERR(sh,
- "Could not access module store active modules subdirectory at %s, or it is not a directory.",
+ "Module store active modules subdirectory at %s is not a directory.",
path);
return -1;
}
@@ -621,8 +620,8 @@ 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) {
- ERR(sh, "Could not access lock file at %s.", path);
+ if (!S_ISREG(sb.st_mode)) {
+ ERR(sh, "Lock file at %s missing.", path);
return -1;
}
}

[Stephen Smalley]

Post by Vit Mojzis
access() uses real UID instead of effective UID which causes false
negative checks in setuid programs (except for F_OK which works
properly). fopen() return values are always checked, which makes
access() checks redundant.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
 libsemanage/src/direct_api.c     |  3 ---
 libsemanage/src/semanage_store.c | 17 ++++++++---------
 2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/libsemanage/src/direct_api.c
b/libsemanage/src/direct_api.c
index f4b0416..dc46f8f 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -146,9 +146,6 @@ int semanage_direct_connect(semanage_handle_t * sh)
  if (semanage_create_store(sh, 1))
  goto err;
 
- if (semanage_access_check(sh) < SEMANAGE_CAN_READ)
- goto err;
-
  sh->u.direct.translock_file_fd = -1;
  sh->u.direct.activelock_file_fd = -1;
 
diff --git a/libsemanage/src/semanage_store.c
b/libsemanage/src/semanage_store.c
index 6b75002..0a10032 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -535,7 +535,6 @@ char *semanage_conf_path(void)
 int semanage_create_store(semanage_handle_t * sh, int create)
 {
  struct stat sb;
- int mode_mask = R_OK | W_OK | X_OK;
  const char *path = semanage_files[SEMANAGE_ROOT];
  int fd;
 
@@ -554,9 +553,9 @@ 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)) {
  ERR(sh,
-     "Could not access module store at %s, or
it is not a directory.",
+     "Module store at %s is not a
directory.",
      path);
  return -1;
  }
@@ -577,9 +576,9 @@ 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)) {
  ERR(sh,
-     "Could not access module store active
subdirectory at %s, or it is not a directory.",
+     "Module store active subdirectory at %s
is not a directory.",
      path);
  return -1;
  }
@@ -600,9 +599,9 @@ 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)) {
  ERR(sh,
-     "Could not access module store active
modules subdirectory at %s, or it is not a directory.",
+     "Module store active modules
subdirectory at %s is not a directory.",
      path);
  return -1;
  }
@@ -621,8 +620,8 @@ 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) {
- ERR(sh, "Could not access lock file at %s.",
path);
+ if (!S_ISREG(sb.st_mode)) {
+ ERR(sh, "Lock file at %s missing.", path);

It can't be missing or the stat() would have failed.
Lock file is not a regular file?
Not sure how useful these tests are though...

Post by Vit Mojzis
  return -1;
  }
  }

[Vit Mojzis]

F_OK access checks only work properly as long as all directories along
the path are accessible to real user running the program.
Replace F_OK access checks by testing return value of open, write, etc.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
libsemanage/src/direct_api.c | 40 +++++++++++++++-------------------------
1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index dc46f8f..508277d 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -342,10 +342,6 @@ static int semanage_direct_disconnect(semanage_handle_t * sh)

static int semanage_direct_begintrans(semanage_handle_t * sh)
{
-
- if (semanage_access_check(sh) != SEMANAGE_CAN_WRITE) {
- return -1;
- }
if (semanage_get_trans_lock(sh) < 0) {
return -1;
}
@@ -1491,33 +1487,27 @@ rebuild:
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}

path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) == 0) {
- retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
- semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
- sh->conf->file_mode);
- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS),
+ semanage_final_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
+ sh->conf->file_mode);
+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
}

/* run genhomedircon if its enabled, this should be the last operation

[Stephen Smalley]

Post by Vit Mojzis
F_OK access checks only work properly as long as all directories along
the path are accessible to real user running the program.
Replace F_OK access checks by testing return value of open, write, etc.

The patch description talks about F_OK tests, but...

Post by Vit Mojzis
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
---
 libsemanage/src/direct_api.c | 40 +++++++++++++++-------------------
------
 1 file changed, 15 insertions(+), 25 deletions(-)
diff --git a/libsemanage/src/direct_api.c
b/libsemanage/src/direct_api.c
index dc46f8f..508277d 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -342,10 +342,6 @@ static int
semanage_direct_disconnect(semanage_handle_t * sh)
 
 static int semanage_direct_begintrans(semanage_handle_t * sh)
 {
-
- if (semanage_access_check(sh) != SEMANAGE_CAN_WRITE) {
- return -1;
- }

The patch has an unrelated change (possibly belonging in the first
patch?).

Post by Vit Mojzis
  if (semanage_get_trans_lock(sh) < 0) {
  return -1;
  }
  }
 
  path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC_LOCAL);
- if (access(path, F_OK) == 0) {
- retval =
semanage_copy_file(semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_FC_LOCAL),
- semanage_fin
al_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
- sh->conf-

Post by Vit Mojzis
file_mode);

- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_FC_LOCAL),
+ semanage_final_path(
SEMANAGE_FINAL_TMP, SEMANAGE_FC_LOCAL),
+ sh->conf-

Post by Vit Mojzis
file_mode);

+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
  }
 
  path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC);
- if (access(path, F_OK) == 0) {
- retval =
semanage_copy_file(semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_FC),
- semanage_fin
al_path(SEMANAGE_FINAL_TMP, SEMANAGE_FC),
- sh->conf-

Post by Vit Mojzis
file_mode);

- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_FC),
+ semanage_final_path(
SEMANAGE_FINAL_TMP, SEMANAGE_FC),
+ sh->conf-

Post by Vit Mojzis
file_mode);

+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
  }
 
  path = semanage_path(SEMANAGE_TMP, SEMANAGE_STORE_SEUSERS);
- if (access(path, F_OK) == 0) {
- retval =
semanage_copy_file(semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
- semanage_fin
al_path(SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
- sh->conf-

Post by Vit Mojzis
file_mode);

- if (retval < 0) {
- goto cleanup;
- }
+ retval = semanage_copy_file(semanage_path(SEMANAGE_TMP,
SEMANAGE_STORE_SEUSERS),
+ semanage_final_path(
SEMANAGE_FINAL_TMP, SEMANAGE_SEUSERS),
+ sh->conf-

Post by Vit Mojzis
file_mode);

+ if (retval < 0 && errno != ENOENT) {
+ goto cleanup;
  }
 
  /* run genhomedircon if its enabled, this should be the last
operation