--- Paul Moore <[EMAIL PROTECTED]> wrote:
> 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.
An idea with merit. On the work queue it goes.
> 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".
A non-pressing but potentialy valuable improvement, I think.
> +#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.
A userspace scheme is a non-starter, it goes against the project
goals of minimal dependence on helper applications. I would be happy
to work with you to create an appropriate API.
> Also, any reason why you don't just use the NetLabel default domain mapping?
Uh, only that I couldn't figure out how to go about doing so. If it
simplifies (there's that word again) things I'm all for it. I would
be happy to have my ignorance dispelled.
> Unless I'm missing something SMACK doesn't make use of multiple NetLabel
> domains.
That is correct.
> +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?
That is correct.
> 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.
Done, in the next patch version.
> +/*
> + * 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 ;)
The cleanest solution is already in the code, where there's a 1:1
mapping of Smack labels to Cipso level/catset maintained via
/smack/cipso, and set administratively. A purist can always set
this up to be complete.
On the other hand, it has already been acknowledged that mapping one
set of CIPSO tags to another is necessary in some cases, for example
when one machine's bit 6 represents another's bits 2 and 3.
And on the gripping hand, it's my security policy and your transport
mechanism, and you've been as careful not to taint your mechanism
with policy enforcement as I have with not transporting packet
attributes (actually, you've been better given smk_cipso_doi()).
> I'm still trying to digest everything but what about
> different DOIs, domains, etc?
I hope to avoid excesses of translation, I would like the label
"Rubble" to come across the network and be dealt with locally on
it's own terms. The uninterpreted textual nature of Smack labels
is a key to maintaining simplicity.
> +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.
I got the impression that that might be the case, but as with the
default domain, the details of how I might do so did not seem obvious
to me. Again, I would welcome enlightenment.
> +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.
I will fix this when it's closer to the top of my list.
> 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.
Audit is #1 on the "Next Steps" slide.
> +/*
> + * 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 :)
Done.
> +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 think I got 'em all.
> I'm sure there will be more comments but these are the ones that jumped out
> at
> me.
Thank you for your helpful comments. Any suggestions on how to approach
the default domain and the caching would be welcome.
Casey Schaufler
[EMAIL PROTECTED]
-
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
