On 02/28/2018 02:26 PM, William Roberts wrote:
> 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?
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.
>
> On Wed, Feb 28, 2018 at 11:07 AM, William Roberts
> <bill.c.roberts@xxxxxxxxx> wrote:
>> On Wed, Feb 28, 2018 at 10:43 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
>>> On 02/28/2018 01:24 PM, William Roberts wrote:
>>>> 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.
>>
>>
>>>>
>>>>
>>>> On Wed, Feb 28, 2018 at 9:50 AM, William Roberts
>>>> <bill.c.roberts@xxxxxxxxx> wrote:
>>>>> On Wed, Feb 28, 2018 at 2:15 AM, Vit Mojzis <vmojzis@xxxxxxxxxx> wrote:
>>>>>> 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 <vmojzis@xxxxxxxxxx>
>>>>>> ---
>>>>>> 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
>>>>>
>>>>>> 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;
>>>>>> }
>>>>>>
>>>>>> --
>>>>>> 2.14.3
>>>>>>
>>>>>>
>
>
>
