[systemd-devel] [PATCH v2 1/2] Smack - relabel directories and files created by systemd

[Lennart Poettering]

On Wed, 19.02.14 14:07, Łukasz Stelmach (l.stelmach at samsung.com) wrote:

> From: Casey Schaufler <casey at schaufler-ca.com>
> 
> Systemd creates directories in /dev. These directories will
> get the label of systemd, which is the label of the System
> domain, which is not accessable to everyone. Relabel the
> directories, files and symlinks created so that they can be
> generally used.
> 
> Signed-off-by: Casey Schaufler <casey.schaufler at intel.com>
> Signed-off-by: Łukasz Stelmach <l.stelmach at samsung.com>
> ---
>  src/shared/label.c |   60 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/src/shared/label.c b/src/shared/label.c
> index 4a26ba9..9a1916a 100644
> --- a/src/shared/label.c
> +++ b/src/shared/label.c
> @@ -41,6 +41,48 @@
>  static struct selabel_handle *label_hnd = NULL;
>  
>  #endif
> +#ifdef HAVE_SMACK
> +#include <sys/xattr.h>
> +#include <string.h>

No includes in the middle of files please for normal API stuff.

Also, these files are not smack-specific. In order to avoid superfluous
#ifdefs, and to avoid uplicate inclusions later on, please just add
these to the top of the file, and include string.h unconditionally, and
xattr.h only if HAVE_XATTR is defined...

> +#define FLOOR_LABEL	"_"
> +#define STAR_LABEL	"*"

hmm, could we rename these to SMACK_LABEL_FLOOR and SMACK_LABEL_STAR?
That way they have a namespaced common namespace.

> +
> +static void smack_relabel_in_dev(const char *path) {
> +        struct stat sb;
> +        const char *label;
> +        int r;
> +
> +        /*
> +         * Path must be in /dev and must exist
> +         */
> +        if (!path_equal(path, "/dev") &&
> +            !path_startswith(path, "/dev"))
> +                return;
> +
> +        r = lstat(path, &sb);
> +        if (r < 0)
> +                return;
> +
> +        /*
> +         * Label directories and character devices "*".
> +         * Label symlinks "_".
> +         * Don't change anything else.
> +         */
> +        if (S_ISDIR(sb.st_mode))
> +                label = STAR_LABEL;
> +        else if (S_ISLNK(sb.st_mode))
> +                label = FLOOR_LABEL;
> +        else if (S_ISCHR(sb.st_mode))
> +                label = STAR_LABEL;
> +        else
> +                return;
> +
> +        r = setxattr(path, "security.SMACK64", label, strlen(label), 0);
> +        if (r < 0)
> +                log_error("Smack relabeling \"%s\" %s", path, strerror(errno));
> +        return;

This return is unnecessary...

That said, I think it find it nicer if this call would actually return
an error, so that the caller decides whether it wants to ignore it, not
the function internally.

Also, please move the #ifdef HAVE_SMACK checks inside of this function
and make it a NOP on non-SMACK builds. That way we only have one #ifdef
check for this and not one for each invocation of the function. The
compiler should be smart away to suppress the function if it empty.

Otherwise looks good.

Lennart

-- 
Lennart Poettering, Red Hat