[systemd-devel] [PATCH v3] journal: add logging of effective capabilities _CAP_EFFECTIVE

[Lennart Poettering]

On Mon, 15.07.13 10:44, Shawn Landden (shawnlandden at gmail.com) wrote:

> +int get_process_capeff(pid_t pid, char **capeff) {
> +        const char *p;
> +        _cleanup_free_ char *status = NULL;
> +        char *t = NULL;
> +        int r;
> +
> +        assert(capeff);
> +        assert(pid >= 0);
> +
> +        if (pid == 0)
> +                p = "/proc/self/status";
> +        else
> +                p = procfs_file_alloca(pid, "status");
> +
> +        r = read_full_file(p, &status, NULL);
> +        if (r < 0)
> +                return r;
> +
> +        t = strstr(status, "CapEff:\t");
> +        if (!t)
> +                return -ENOENT;

Hmm, I am a bit concerned about broad checks like this, I'd always
prefer if we could at least look for a newline before and after the
relevant line. In this case this should be simple to fix, by looking for
"\nCapEff:\t". Otherwise, an evil user might be able to change the name
of its process to "CappEff\t" and our code would be throughly confused
and parse the wrong line from the file...

\n is used as record separator by the kernel here, so let's rely on
this. If the process name includes a \n, then it's the job of the kernel
to escape it.

> +
> +        *capeff = strndup(t + strlen("CapEff:\t"), 16);
> +        if (!*capeff)
> +                return -ENOMEM;

The number of capabilities is probably going to grow over time. I am a
bit concerned of always just reading 16 bytes here... Better: read until
the next newline.

Hmm, and if we take the strings 1:1 then there is the problem that if
the number of caps is increased, then they search keys might need to
change over time too, which sucks for compatibility reasons. Hence I'd
recommend simply dropping all initial "0" chars (except for the entirely
empty caps set where i'd leave one "0" in). That way there's exactly one
representation for each capability set, instead of many, if you follow
what I mean.

Example:

On a current kernel a process might have the CapEff field set to
"0000003fffffffff". If the kernel caps set size is one day increased to
twice the number of bits we'd read "00000000000000000000003fffffffff"
instead. Both are the exact same sets though. Hence if you look for the
value made on a new kernel you couldn't find the entries of the old one
anymore, and vice versa. Hence my recommendation to store this as
"3fffffffff" instead, which avoids this ambuigity. Hope that makes
sense?

Otherwise looks fine.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.