Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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 <cgzones@xxxxxxxxxxxxxx>
>
> 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.
