Casey Schaufler <ca...@schaufler-ca.com> wrote:
> On 9/19/2018 4:14 PM, Christian Göttsche wrote:
> > Add the ability to set the security context of packets within the nf_tables 
> > framework.
> > Add a nft_object for holding security contexts in the kernel and 
> > manipulating packets on the wire.
> > The contexts are kept as strings and are evaluated to security identifiers 
> > at runtime (packet arrival),
> > so that the nft_objects do not need to be refreshed after security changes.
> > The maximum security context length is set to 256.
> >
> > Based on v4.18.6
> >
> > Signed-off-by: Christian Göttsche <cgzo...@googlemail.com>
> 
> I've only had a cursory look at your patch, but how is it
> different from what's in xt_SECMARK.c ?

this change is supposed to make secmark labeling accessible from
nftables.

The advantage is that its now possible to use
maps to assign secmarks from a single rule instead of using
several rules:

nft add rule meta secmark set tcp dport map { 22 : tag-ssh, 80 :
        tag-http }

and so on.

> > +   for (i = 0; i < ARRAY_SIZE(nft_basic_objects); i++) {
> > +           err = nft_register_obj(nft_basic_objects[i]);
> > +           if (err)
> > +                   goto err;
> > +   }
> >  
> > -   for (i = 0; i < ARRAY_SIZE(nft_basic_types); i++) {
> > -           err = nft_register_expr(nft_basic_types[i]);
> > +   for (j = 0; j < ARRAY_SIZE(nft_basic_types); j++) {
> > +           err = nft_register_expr(nft_basic_types[j]);
> >             if (err)
> >                     goto err;
> >     }
> > @@ -248,8 +260,12 @@ int __init nf_tables_core_module_init(void)
> >     return 0;
> >  
> >  err:
> > +   while (j-- > 0)
> > +           nft_unregister_expr(nft_basic_types[j]);
> > +
> >     while (i-- > 0)
> > -           nft_unregister_expr(nft_basic_types[i]);
> > +           nft_unregister_obj(nft_basic_objects[i]);
> > +
> >     return err;

Do I read this right in that this is a error unroll bug fix?
If so, could you please submit this as indepentent patch?

Fixes should go into nf.git whereas feature goes to nf-next.git.

> > +struct nft_secmark {
> > +   char ctx[NFT_SECMARK_CTX_MAXLEN];
> > +   int len;
> > +};
> > +
> > +static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = {
> > +   [NFTA_SECMARK_CTX]     = { .type = NLA_STRING, .len = 
> > NFT_SECMARK_CTX_MAXLEN },
> > +};
> > +
> > +static void nft_secmark_obj_eval(struct nft_object *obj, struct nft_regs 
> > *regs, const struct nft_pktinfo *pkt)
> > +{
> > +   const struct nft_secmark *priv = nft_obj_data(obj);
> > +   struct sk_buff *skb = pkt->skb;
> > +   int err;
> > +   u32 secid = 0;
> > +
> > +   /* skip if packet has already a secmark */
> > +   if (skb->secmark)
> > +           return;

xt_SECMARK doesn't do this and will allow relabeling.
What do the LSM experts think?

> > +   err = security_secctx_to_secid(priv->ctx, priv->len, &secid);

Could someone familiar with how LSMs work clarify if this has to be
called per-packet?

xt_SECMARK.c does this ctx -> secid mapping once, when the iptables rule
gets added, whereas this patch does it once for each packet.

Is the ctx -> secid mapping stable?

If yes, the code above should be moved to the ->init() hook, otherwise
we'll need to fix xt_SECMARK.c.

> > +   if (err) {
> > +           if (err == -EINVAL)
> > +                   pr_notice_ratelimited("invalid security context 
> > \'%s\'\n", priv->ctx);
> > +           else
> > +                   pr_notice_ratelimited("unable to convert security 
> > context \'%s\': %d\n", priv->ctx, -err);
> > +           return;
> > +   }

Please remove these printks(), they do not really help as user can't
take any action anyway.

> > +   err = security_secmark_relabel_packet(secid);

Hmm, this function uses current() to check permissions of calling
task, so this function canot be used in ->eval() path.

Network stack causes random results of "current()", as network
processing can "steal" cpu from some arbitrary task when
softinterrupt kicks in.

->init() is fine, as its in process context and current will be the task
installing the nftables rule.

_______________________________________________
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Reply via email to