On Saturday, July 14 2007 5:47:38 pm Casey Schaufler wrote:
> Smack is the Simplified Mandatory Access Control Kernel.
One general comment I have, and this is more of a nit really, is that the kdoc
comment blocks at the top of functions are _really_ nice in my opinion. I
would encourage you to add them to some of the more important functions, if
not all of them.
Initial comments below ...
+/*
+ * An entry in the table of permitted label accesses.
+ */
+struct smk_list_entry {
+ struct smk_list_entry *smk_next;
+ struct smk_list_entry *smk_prev;
+ struct smack_rule smk_rule;
+};
You should probably use the kernel's list type/functions,
see "include/linux/list.h".
+#include "../../net/netlabel/netlabel_domainhash.h"
+#include <net/cipso_ipv4.h>
+
+ {snip}
+
+static void smk_cipso_doi(void)
+{
+ int rc;
+ struct cipso_v4_doi *doip;
+ struct netlbl_dom_map *ndmp;
+ struct netlbl_audit audit_info;
+
+ if (smk_cipso_doied != 0)
+ return;
+ smk_cipso_doied = 1;
+
+ doip = kmalloc(sizeof(struct cipso_v4_doi), GFP_KERNEL);
+ if (doip == NULL)
+ panic("smack: Failed to initialize cipso DOI.\n");
+ doip->map.std = NULL;
+
+ ndmp = kmalloc(sizeof(struct netlbl_dom_map), GFP_KERNEL);
+ if (ndmp == NULL)
+ panic("smack: Failed to initialize cipso ndmp.\n");
+
+ doip->doi = SMACK_CIPSO_DOI;
+ doip->type = CIPSO_V4_MAP_PASS;
+ doip->tags[0] = CIPSO_V4_TAG_RBITMAP;
+ for (rc = 1; rc < CIPSO_V4_TAG_MAXCNT; rc++)
+ doip->tags[rc] = CIPSO_V4_TAG_INVALID;
+
+ rc = cipso_v4_doi_add(doip);
+ printk("%s:%d add doi rc = %d\n", __FUNCTION__, __LINE__, rc);
+
+ /*
+ rc = cipso_v4_doi_domhsh_add(doip, SMACK_CIPSO_DOMAIN_NAME);
+ printk("%s:%d hash doi rc = %d\n", __FUNCTION__, __LINE__, rc);
+ */
+ ndmp->domain = SMACK_CIPSO_DOMAIN_NAME;
+ ndmp->type = NETLBL_NLTYPE_CIPSOV4;
+ ndmp->type_def.cipsov4 = doip;
+ rc = netlbl_domhsh_add(ndmp, &audit_info);
+ printk("%s:%d hash doi rc = %d\n", __FUNCTION__, __LINE__, rc);
+}
If SMACK gets an OK for upstream we need to find a better solution than the
above. The smk_cipso_doi() function pokes into too many NetLabel internals.
The two obvious options are to rely on userspace (netlabel_tools) to
configure NetLabel for SMACK, like SELinux, or to create a new NetLabel API
for allowing LSMs to manipulate the NetLabel domain mapping.
Also, any reason why you don't just use the NetLabel default domain mapping?
Unless I'm missing something SMACK doesn't make use of multiple NetLabel
domains.
+static struct option_names netlbl_choices[] = {
+ { NETLBL_NLTYPE_NONE,
+ "NLBL_NONE", "none" },
+ { NETLBL_NLTYPE_MGMT,
+ NETLBL_NLTYPE_MGMT_NAME, "mgmt" },
+ { NETLBL_NLTYPE_RIPSO,
+ NETLBL_NLTYPE_RIPSO_NAME, "ripso" },
+ { NETLBL_NLTYPE_CIPSOV4,
+ NETLBL_NLTYPE_CIPSOV4_NAME, "cipsov4" },
+ { NETLBL_NLTYPE_CIPSOV4,
+ NETLBL_NLTYPE_CIPSOV4_NAME, "cipso" },
+ { NETLBL_NLTYPE_CIPSOV6,
+ NETLBL_NLTYPE_CIPSOV6_NAME, "cipsov6" },
+ { NETLBL_NLTYPE_UNLABELED,
+ NETLBL_NLTYPE_UNLABELED_NAME, "unlabeled" },
+};
+#define NCHOICES (sizeof(netlbl_choices) / sizeof(struct option_names))
It looks like you are only using the netlbl_choices structure to track actual
labeled networking protocols, yes? If that's the case, of the above entries
you can drop "mgmt" (maybe "none" too) as it isn't a labeling protocol but a
management "module" for NetLabel.
+/*
+ * CIPSO domain information.
+ * Casey says that these may wind up being configurable
+ * at some point.
+ */
+#define SMACK_CIPSO_DOMAIN_NAME "SmackCIPSO"
+#define SMACK_CIPSO_DOI 3
+#define SMACK_CIPSO_DIRECT 250 /* Arbitrary and bad */
Yes, SMACK_CIPSO_DIRECT is bad. There has to be a cleaner solution than this,
think harder ;) I'm still trying to digest everything but what about
different DOIs, domains, etc?
+static smack_t smack_from_secattr(struct netlbl_lsm_secattr *sap)
+{
+ struct smk_cipso_entry *scp;
+ smack_t smack = 0LL;
+ int pcat;
+
+ if ((sap->flags & NETLBL_SECATTR_MLS_LVL) == 0) {
+ /*
+ * If there are flags but no level netlabel isn't
+ * behaving the way we expect it to.
+ *
+ * Without guidance regarding the smack value
+ * for the packet fall back on the network
+ * ambient value.
+ */
+ return smack_net_ambient;
+ }
+ /*
+ * Get the categories, if any
+ */
+ if ((sap->flags & NETLBL_SECATTR_MLS_CAT) != 0)
+ for (pcat = -1;;) {
+ pcat = netlbl_secattr_catmap_walk(sap->mls_cat, pcat+1);
+ if (pcat < 0)
+ break;
+ smack_catset_bit(pcat, &smack);
+ }
+ /*
+ * If it is CIPSO using smack direct mapping
+ * we are already done. WeeHee.
+ */
+ if (sap->mls_lvl == SMACK_CIPSO_DIRECT)
+ return smack;
+
+ /*
+ * Look it up in the supplied table if it is not a direct mapping.
+ */
+ for (scp = smack_cipso; scp != NULL; scp = scp->smk_next)
+ if (scp->smk_level == sap->mls_lvl && scp->smk_catset == smack)
+ return scp->smk_smack;
The NetLabel caching mechanism can help make this a lot less painful, i.e.
direct return of the correct "smack" instead of traversing an unordered
linked list each time.
+static ssize_t smk_write_cipso(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct smk_cipso_entry *scp;
+ smack_t mapsmack;
+ smack_t mapcatset;
+ int maplevel;
+ ssize_t rc = count;
+ char *data = NULL;
+ char *cp;
+ char *eolp;
+ char *linep;
+ int cat;
+ int i;
+
+ smk_cipso_doi();
+
+ if (!capable(CAP_MAC_OVERRIDE))
+ return -EPERM;
+ /*
+ * No partial writes.
+ */
+ if (*ppos != 0)
+ return -EINVAL;
+ /*
+ * Casey wonders about this number.
+ */
+ if ((count > 64 * 1024 * 1024) || (data = vmalloc(count + 1)) == NULL)
+ return -ENOMEM;
I wonder about that number too. I realize it is always going to be hard to
get rid of all those magic numbers, but at least put a nice comment there so
we know the reasoning behind the value (saying it's a guess is okay for me)
and ideally make it a constant/macro/#define somewhere.
This applies to a few other places in the code too.
+int smk_curacc(smack_t *obj_label, u32 mode)
+{
+ struct task_smack *tsp = current->security;
+ int rc;
+
+ rc = smk_access(&tsp->smk_task, obj_label, mode);
+ if (rc == 0)
+ return 0;
+
+ if (capable(CAP_MAC_OVERRIDE))
+ return 0;
+
+ return rc;
+}
I noticed a distinct lack of auditing in SMACK, the function above is a
perfect example. I know Linux's auditing mechanism is "interesting", but you
should probably make use of it for security related events.
+/*
+ * Should access checks be done on each read or write?
+ * UNICOS and SELinux say yes.
+ * Trusted Solaris, Trusted Irix, and just about everyone else says no.
+ */
+static int smack_file_permission(struct file *file, int mask)
+{
+#ifdef SMACKONREADWRITE
+ smack_t *fsp;
+ int rc;
+
+ if (mask == 0)
+ return 0;
+
+ fsp = (smack_t *)file->f_dentry->d_inode->i_security;
+
+ rc = smk_curacc(fsp, mask);
+ return rc;
+#else /* SMACKONREADWRITE */
+ return 0;
+#endif /* SMACKONREADWRITE */
+}
Pick one, even if it's wrong :)
+static int smack_task_to_sock(struct sock *sk, struct task_smack *tsp)
+{
+ struct netlbl_lsm_secattr secattr;
+ int rc;
+
+ netlbl_secattr_init(&secattr);
+ smack_to_secattr(tsp->smk_out, tsp->smk_ipscheme, &secattr);
+ if (secattr.flags != 0)
Please use the constants defined for the flag values in the NetLabel LSM
security attributes; it makes life easier if for some freak reason we have to
change them. For example, 0 == NETLBL_SECATTR_NONE.
This applies to a few other places in the code too.
I'm sure there will be more comments but these are the ones that jumped out at
me.
--
paul moore
linux security @ hp
-
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
