[systemd-devel] [PATCH] selinux: fix selinux check for transient units

Kay Sievers kay at vrfy.org
Mon Nov 4 08:11:28 PST 2013


On Mon, Nov 4, 2013 at 5:06 PM, Lennart Poettering
<lennart at poettering.net> wrote:

> Sorry, I don't understand what this patch is doing. Please explain in a
> commit message!

The file format should also be documented in the code itself, if not
done by selinx, then we need to add the link to the doc.

>> ---
>>  src/core/selinux-access.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  src/core/selinux-access.h | 15 +++++++++---
>>  2 files changed, 70 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c
>> index 0a3ee18..5908a79 100644
>> --- a/src/core/selinux-access.c
>> +++ b/src/core/selinux-access.c
>> @@ -333,6 +333,50 @@ static int get_calling_context(
>>          return 0;
>>  }
>>
>> +static int load_context(
>> +                const char *path,
>> +                const char *name,
>> +                char **context) {
>> +
>> +        int r = 0;
>> +        char l[LINE_MAX];
>> +        char *p;
>> +        FILE *fd;
>> +
>> +        fd = fopen(path, "r");
>
> Please always use "re" rather than "r". Not that it would matter much
> here, but Please always do.
>
>> +        if (!fd)
>> +                return -EEXIST;
>> +
>> +        if (!fgets(l, sizeof(l), fd)) {
>
> Hmm, isn't this a job for read_one_line_file()?
>
>> +                if (feof(fd)) {
>> +                        r = -EINVAL;
>> +                        goto finish;
>> +                }
>> +
>> +                log_error("Failed to read context file '%s': %m", path);
>> +                r = -errno;
>> +                goto finish;
>> +        }
>> +
>> +        p = strtok (l, "=");
>
> strtok() is evil, as it keeps internal state, please don't use it. Also
> one space too much...
>
>> +        if (p && streq(p, name)) {
>> +                p = strtok (NULL, "=");
>> +                if (!p) {
>> +                        r = -EINVAL;
>> +                        goto finish;
>> +                }
>> +
>> +                *context = strdup(p);
>
> Missing OOM check.
>
>> +        } else
>> +                r = -EINVAL;
>> +
>> +finish:
>> +        if (fd)
>> +                fclose(fd);
>
> Please use _cleanup_close_ for cases like this.

It should probably just use:
  parse_env_file()
it does all that.

>> +        } else if (class == CLASS_TRANSIENT) {
>> +                const char *context_path =
>> selinux_systemd_contexts_path();
>
> Where is selinux_systemd_contexts_path() define? I cant find it in this
> patch and neither in the sources? What am I missing?

http://people.fedoraproject.org/~dwalsh/SELinux/Patches/0008-Add-selinux_systemd_contexts_path.patch

>> +                if (_u->source_path || _u->fragment_path)              \
>> +                        _r = selinux_access_check(_c, _m, CLASS_SERVICE, _u->source_path ?: _u->fragment_path, (permission), &_error); \
>> +                else                                                    \
>> +                        _r = selinux_access_check(_c, _m, CLASS_TRANSIENT, _u->id, (permission), &_error); \
>
> You can recognize transient units via the transient bool on the Unit structure.

Kay


More information about the systemd-devel mailing list