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

[Lennart Poettering]

On Thu, 31.10.13 15:51, Vaclav Pavlin (vpavlin at redhat.com) wrote:

> From: Václav Pavlín <vpavlin at redhat.com>

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

> 
> ---
>  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.

> +        } 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?

> +                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.

Lennart

-- 
Lennart Poettering, Red Hat