Message-ID: <20070827215954.GA5979@thorium2.jmh.mhn.de>
Date:	Mon, 27 Aug 2007 23:59:54 +0200
From:	Thomas Bleher <ThomasBleher@....de>
To:	Casey Schaufler <casey@...aufler-ca.com>
Cc:	torvalds@...l.org, akpm@...l.org,
	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Version2 Smack: Simplified Mandatory Access Control Kernel

* Casey Schaufler <casey@...aufler-ca.com> [2007-08-27 22:51]:
> 
> Smack is the Simplified Mandatory Access Control Kernel.
> 
> Smack implements mandatory access control (MAC) using labels
> attached to tasks and data containers, including files, SVIPC,
> and other tasks. Smack is a kernel based scheme that requires
> an absolute minimum of application support and a very small
> amount of configuration data.
> 
> Smack is implemented as a clean LSM. It requires no external
> code changes and the patch modifies only the Kconfig and Makefile
> in the security directory. Smack uses extended attributes and
> provides a set of general mount options, borrowing technics used
> elsewhere. Smack uses netlabel for CIPSO labeling. Smack provides
> a pseudo-filesystem smackfs that is used for manipulation of
> system Smack attributes.
> 
> The patch, patches for ls and sshd, a README, a startup script,
> and x86 binaries for ls and sshd are also available on
> 
>     http:/www.schaufler-ca.com

I like the general idea of this LSM.
One question about your security model, though:
If I understand your LSM correctly, MAC override is based on
capabilities, specifically on CAP_LINUX_IMMUTABLE. So any root process
which doesn't explicitly give up this capability is effectively
unconfined, right?

If so, this is a serious limitation that should be mentioned in the
docs. File capabilities should alleviate this problem, but they first
need to be configured correctly...

Some other comments below:

> --- linux-2.6.22-base/security/smack/Kconfig	1969-12-31 16:00:00.000000000 -0800
> +++ linux-2.6.22-smack/security/smack/Kconfig	2007-08-22 02:18:55.000000000 -0700
> @@ -0,0 +1,10 @@
> +config SECURITY_SMACK
> +	bool "Simplified Mandatory Access Control Kernel Support"
> +	depends on NETLABEL && SECURITY_NETWORK
> +	default n
> +	help
> +	  This selects the Simplified Mandatory Access Control Kernel.
> +	  Smack is useful for sensitivity, integrity, and a variety
> +	  of other mandatory security schemes.
> +	  If you are unsure how to answer this question, answer N.

I think a link to further documentation would be nice.
Maybe you could include the README from your site in Documentation/?

BTW: Some documentation missing there:
* the various mount options
* how to enable cipso (Do I have to enable it explicitly? Most people
  won't even know what cipso is)


> +/**
> + * smack_syslog - Smack approval on syslog
> + * @type: message type
> + *
> + * Require that the task has the floor label
> + *
> + * Returns 0 on success, error code otherwise.
> + */
> +static int smack_syslog(int type)
> +{
> +	int rc;
> +	smack_t *sp = current->security;
> +
> +	rc = cap_syslog(type);
> +	if (rc == 0)
> +		 if (*sp != SMK_FLOOR)
> +			rc = -EACCES;
> +
> +	return rc;
> +}

Can you explain why you limit syslog that way?

> +/**
> + * smackfs_follow_link - follow a smackfs symlink
              ^^^^^^^^^^^ should be put_link
> + * @dentry: unused
> + * @nd: name entry
> + * @ptr: unused
> + *
> + * free the buffer used in following the link.
> + */
> +static void smackfs_put_link(struct dentry *dentry, struct nameidata *nd, void *ptr)
> +{
> +	kfree(nd_get_link(nd));
> +}


> +       for (cp = data - 1; cp != NULL; cp = strchr(cp + 1, '\n')) {
> +               if (*++cp == '\0')
> +                       break;
> +               if (sscanf(cp, "%14s %30s\n", name, target) != 2) {
> +                       printk("%s:%d bad scan\n",
> +                               __func__, __LINE__);

Leftover debugging printk? Otherwise, a level would be nice.

> +                       break;
> +               }
> +               smk_add_symlink(name, target);
> +               printk("%s:%d add %s -> %s\n",
> +                       __func__, __LINE__, name, target);

Ditto.

> +       }


> +/**
> + * smackfs_follow_link - follow a smackfs symlink
> + * @dentry: name cache entry
> + * @nd: name entry
> + *
> + * A symlink on smackfs has unusual semantics.
> + *
> + * The Smack value of the task is appended to the link string.
> + * Thus, if a task labeled "Gentoo" does chdir("/smack/tmp")
> + * it will use "/moldy/Gentoo".
> + *
> + * The expected usage is the /tmp is a symlink to /smack/tmp
> + * which is itself a symlink to /moldy. /moldy should have a
> + * directory for each label in use to accomodate the value
> + * appended on the redirection.
> + *
> + * An interesting addition would be a file system that automatically
> + * creates directories as needed, at the appropriate label.
> + */

I'm not very versed in the VFS, so I don't think I understand the code
fully there, but shouldn't the above read /smack/links?


> +	static struct tree_descr smack_files[] = {
> +		[SMK_LOAD]	= {"load", &smk_load_ops, S_IRUGO|S_IWUSR},
> +		[SMK_LINKS]	= {"links", &smk_links_ops, S_IRUGO|S_IWUSR},

I really don't understand your symlink code (not because it's bad, I just
don't know this area), so this is just a question:
Wouldn't S_IRUGO be enough for the links entry?

> +		[SMK_CIPSO]	= {"cipso", &smk_cipso_ops, S_IRUGO|S_IWUSR},
> +		[SMK_DOI]	= {"doi", &smk_doi_ops, S_IRUGO|S_IWUSR},
> +		[SMK_DIRECT]	= {"direct", &smk_direct_ops, S_IRUGO|S_IWUSR},
> +		[SMK_AMBIENT]	= {"ambient", &smk_ambient_ops,S_IRUGO|S_IWUSR},
> +		[SMK_NLTYPE]	= {"nltype", &smk_nltype_ops, S_IRUGO|S_IWUSR},
> +		/* last one */ {""}
> +	};
> +
> +	/*
> +	 * There will be only one smackfs. Casey says so.
> +	 */
> +	smk_sb = sb;
> +
> +	rc = simple_fill_super(sb, SMACK_MAGIC, smack_files);
> +	if (rc != 0) {
> +		printk(KERN_ERR "%s failed %d while creating inodes\n",
> +			__func__, rc);
> +		return rc;
> +	}
> +
> +	root_inode = sb->s_root->d_inode;
> +	root_inode->i_security = new_inode_smack(&smack_known_floor.smk_known);
> +
> +	/*
> +	 * Create a directory for /smack/tmp
> +	 */
> +	smk_add_symlink("tmp", SMK_TMPPATH_ROOT);

Ah, so you dynamically add symlinks here. Can the user do this, too? If
so, how? A little documentation might be nice.

Thomas


Download attachment "signature.asc" of type "application/pgp-signature" (190 bytes)

Powered by blists - more mailing lists