linux-kernel - Re: [PATCH 4/5] conntrack: export lsm context rather than internal secid via netlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1286925891.2614.13.camel@localhost.localdomain>
Date:	Tue, 12 Oct 2010 19:24:51 -0400
From:	Eric Paris <eparis@...hat.com>
To:	Paul Moore <paul.moore@...com>
Cc:	linux-kernel@...r.kernel.org, netfilter-devel@...r.kernel.org,
	netfilter@...r.kernel.org, jmorris@...ei.org,
	selinux@...ho.nsa.gov, sds@...ho.nsa.gov, jengelh@...ozas.de,
	linux-security-module@...r.kernel.org, mr.dash.four@...glemail.com,
	pablo@...filter.org
Subject: Re: [PATCH 4/5] conntrack: export lsm context rather than internal
 secid via netlink

On Tue, 2010-10-12 at 19:14 -0400, Paul Moore wrote:
> On Tue, 2010-10-12 at 11:40 -0400, Eric Paris wrote:
> > The conntrack code can export the internal secid to userspace.  These are
> > dynamic, can change on lsm changes, and have no meaning in userspace.  We
> > should instead be sending lsm contexts to userspace instead.  This patch sends
> > the secctx (rather than secid) to userspace over the netlink socket.  We use a
> > new field CTA_SECCTX and stop using the the old CTA_SECMARK field since it did
> > not send particularly useful information.
> 
> Looks fine to me in principal, just a nit-picky comment below ...
> 
> > -ctnetlink_dump_secmark(struct sk_buff *skb, const struct nf_conn *ct)
> > +ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> >  {
> > -	NLA_PUT_BE32(skb, CTA_SECMARK, htonl(ct->secmark));
> > -	return 0;
> > +	struct nlattr *nest_secctx;
> > +	int len, ret;
> > +	char *secctx;
> > +
> > +	ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = -1;
> > +	nest_secctx = nla_nest_start(skb, CTA_SECCTX | NLA_F_NESTED);
> > +	if (!nest_secctx)
> > +		goto nla_put_failure;
> >  
> > +	NLA_PUT_STRING(skb, CTA_SECCTX_NAME, secctx);
> > +	nla_nest_end(skb, nest_secctx);
> > +
> > +	ret = 0;
> >  nla_put_failure:
> > -	return -1;
> > +	security_release_secctx(secctx, len);
> > +	return ret;
> >  }
> 
> All the ret assignments bother me, I also don't think "nla_put_failure"
> is a good choice since we run this code both on success and failure -
> how about this:

#define NLA_PUT(skb, attrtype, attrlen, data) \
        do { \
                if (unlikely(nla_put(skb, attrtype, attrlen, data) < 0)) \
                        goto nla_put_failure; \
        } while(0)

#define NLA_PUT_STRING(skb, attrtype, value) \
        NLA_PUT(skb, attrtype, strlen(value) + 1, value)

so we can't get rid of the nla_put_failure tag and it also means your
ret values aren't quite right, we have to set ret = -1 before the
NLA_PUT_STRING()....

> ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> {
> 	struct nlattr *nest_secctx;
> 	int len, ret;
> 	char *secctx;
> 
> 	ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
> 	if (ret)
> 		return ret;
> 
> 	nest_secctx = nla_nest_start(skb, CTA_SECCTX | NLA_F_NESTED);
> 	if (!nest_secctx) {
> 		ret = -1;
> 		goto dump_secctx_out;
> 	}
> 
> 	NLA_PUT_STRING(skb, CTA_SECCTX_NAME, secctx);
> 	nla_nest_end(skb, nest_secctx);
> 
> dump_secctx_out:
> 	security_release_secctx(secctx, len);
> 	return ret;
> }
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/