From: Petr Lautrbach <plautrba@redhat.com>
To: Vit Mojzis <vmojzis@redhat.com>
Cc: selinux@tycho.nsa.gov
Subject: Re: [PATCH] libsemanage: replace access() checks to make setuid programs work
Date: Sat, 17 Mar 2018 08:26:17 +0100 [thread overview]
Message-ID: <20180317072616.GA4209@workstation> (raw)
In-Reply-To: <20180309153944.7733-1-vmojzis@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 11775 bytes --]
On Fri, Mar 09, 2018 at 04:39:44PM +0100, Vit Mojzis wrote:
> access() uses real UID instead of effective UID which causes false
> negative checks in setuid programs.
> Replace access() calls (mostly tests for file existence) by stat().
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1186431
>
> Signed-off-by: Vit Mojzis <vmojzis@redhat.com>
> ---
> libsemanage/src/direct_api.c | 137 +++++++++++++++++++++++++--------------
> libsemanage/src/semanage_store.c | 11 +++-
> 2 files changed, 98 insertions(+), 50 deletions(-)
>
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index 92d7517d..439122df 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,16 @@ 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) {
> + /* The file does not exist */
> sepol_set_disable_dontaudit(sh->sepolh, 0);
> + } else {
> + ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> + goto err;
> + }
>
> return STATUS_SUCCESS;
>
> @@ -1139,6 +1146,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 +1163,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) {
> @@ -1196,6 +1208,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,
> @@ -1234,10 +1247,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) {
> + /* The file does not exist */
> do_rebuild |= (sepol_get_disable_dontaudit(sh->sepolh) == 1);
> + } else {
> + ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> + retval = -1;
> + goto cleanup;
> + }
> if (sepol_get_disable_dontaudit(sh->sepolh) == 1) {
> FILE *touch;
> touch = fopen(path, "w");
> @@ -1259,10 +1278,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) {
> + /* The file does not exist */
> do_rebuild |= (sepol_get_preserve_tunables(sh->sepolh) == 1);
> + } else {
> + ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> + retval = -1;
> + goto cleanup;
> + }
> +
> if (sepol_get_preserve_tunables(sh->sepolh) == 1) {
> FILE *touch;
> touch = fopen(path, "w");
> @@ -1299,40 +1325,25 @@ 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++) {
The size of the files needs to be divided by size of int, otherwise it causes segfault:
# semanage fcontext -m -t httpd_log_t "/var/opt/quest/vas/vasd(/
[1] 3063 segmentation fault (core dumped) semanage fcontext -m -t httpd_log_t "/var/opt/quest/vas/vasd(/.*)?"
backtrace:
#0 0x00007fffe7901f9b in semanage_path (store=store@entry=SEMANAGE_TMP, path_name=3918632424) at semanage_store.c:487
#1 0x00007fffe78f4be1 in semanage_direct_commit (sh=0x55555840da00) at direct_api.c:1336
#2 0x00007fffe78f9f71 in semanage_commit (sh=0x55555840da00) at handle.c:428
Something like this should fix it:
#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
for (i = 0; i < ARRAY_SIZE(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));
> + retval = -1;
> + 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;
> + }
> }
> }
>
> @@ -1465,7 +1476,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),
> @@ -1473,12 +1484,17 @@ rebuild:
> if (retval < 0)
> goto cleanup;
> pseusers->dtable->drop_cache(pseusers->dbase);
> - } else {
> + } else if (errno == ENOENT) {
> + /* The file does not exist */
> pseusers->dtable->clear(sh, pseusers->dbase);
> + } else {
> + ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> + retval = -1;
> + goto cleanup;
> }
>
> 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),
> @@ -1486,8 +1502,13 @@ rebuild:
> if (retval < 0)
> goto cleanup;
> pusers_extra->dtable->drop_cache(pusers_extra->dbase);
> - } else {
> + } else if (errno == ENOENT) {
> + /* The file does not exist */
> pusers_extra->dtable->clear(sh, pusers_extra->dbase);
> + } else {
> + ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> + retval = -1;
> + goto cleanup;
> }
> }
>
> @@ -1817,6 +1838,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(
> @@ -1829,8 +1851,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;
> }
> @@ -1859,7 +1881,13 @@ 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));
> + rc = -1;
> + goto cleanup;
> + }
> +
> rc = semanage_compile_module(sh, _modinfo);
> if (rc < 0) {
> goto cleanup;
> @@ -2004,6 +2032,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 {
> @@ -2330,6 +2364,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;
> @@ -2738,6 +2778,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);
> @@ -2838,7 +2879,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..14ad99c1 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;
> + struct stat sb;
>
> len = strlen(semanage_root()) + strlen(selinux_path()) + strlen(SEMANAGE_CONF_FILE);
> semanage_conf = calloc(len + 1, sizeof(char));
> @@ -522,7 +523,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 (stat(semanage_conf, &sb) != 0 && errno == ENOENT) {
> snprintf(semanage_conf, len + 1, "%s%s", selinux_path(), SEMANAGE_CONF_FILE);
> }
>
> @@ -1508,8 +1509,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
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2018-03-17 7:27 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-06 11:58 libsemanage: Perform access check using euid instead of uid v2 Vit Mojzis
2018-03-06 11:58 ` [PATCH 1/3] libsemanage: remove access() check to make setuid programs work Vit Mojzis
2018-03-06 11:58 ` [PATCH 2/3] " Vit Mojzis
2018-03-08 19:54 ` Stephen Smalley
2018-03-06 11:58 ` [PATCH 3/3] libsemanage: replace access() checks " Vit Mojzis
2018-03-07 14:59 ` Stephen Smalley
2018-03-08 20:24 ` Stephen Smalley
2018-03-09 13:14 ` Vit Mojzis
2018-03-09 13:28 ` Stephen Smalley
2018-03-09 13:36 ` Stephen Smalley
2018-03-09 15:21 ` [PATCH] " Vit Mojzis
2018-03-09 15:31 ` Stephen Smalley
2018-03-09 15:39 ` Vit Mojzis
2018-03-09 15:51 ` Stephen Smalley
2018-03-13 10:58 ` Petr Lautrbach
2018-03-17 7:26 ` Petr Lautrbach [this message]
2018-03-19 14:46 ` [PATCH] libsemanage/direct_api.c: Fix iterating over array Vit Mojzis
2018-03-19 15:19 ` William Roberts
2018-03-19 16:18 ` William Roberts
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180317072616.GA4209@workstation \
--to=plautrba@redhat.com \
--cc=selinux@tycho.nsa.gov \
--cc=vmojzis@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.