On Wed, Feb 28, 2018 at 11:39 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> 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.
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?
>
>>
>> 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
>>>>>>>
>>>>>>>
>>
>>
>>
>
--
Respectfully,
William C Roberts
