| 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
| ||
|
Message-ID: <0b4f0cd3-181f-4be6-890c-1d225ac1f161@schaufler-ca.com>
Date: Thu, 24 Oct 2024 10:57:41 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: Pablo Neira Ayuso <pablo@...filter.org>
Cc: paul@...l-moore.com, linux-security-module@...r.kernel.org,
jmorris@...ei.org, serge@...lyn.com, keescook@...omium.org,
john.johansen@...onical.com, penguin-kernel@...ove.sakura.ne.jp,
stephen.smalley.work@...il.com, linux-kernel@...r.kernel.org,
selinux@...r.kernel.org, mic@...ikod.net, netdev@...r.kernel.org,
audit@...r.kernel.org, netfilter-devel@...r.kernel.org,
Todd Kjos <tkjos@...gle.com>, Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v3 2/5] LSM: Replace context+len with lsm_context
On 10/24/2024 9:10 AM, Pablo Neira Ayuso wrote:
> Hi Casey,
>
> This is a review of the netfilter chunk.
Thank you.
> On Wed, Oct 23, 2024 at 02:21:55PM -0700, Casey Schaufler wrote:
>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
>> index 86a57a3afdd6..dd74d4c67c69 100644
>> --- a/net/netfilter/nf_conntrack_netlink.c
>> +++ b/net/netfilter/nf_conntrack_netlink.c
>> @@ -360,8 +360,8 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
>> struct lsm_context ctx;
>> int ret;
>>
>> - ret = security_secid_to_secctx(ct->secmark, &ctx.context, &ctx.len);
>> - if (ret)
>> + ret = security_secid_to_secctx(ct->secmark, &ctx);
>> + if (ret < 0)
>> return 0;
>>
>> ret = -1;
>> @@ -665,8 +665,8 @@ static inline int ctnetlink_secctx_size(const struct nf_conn *ct)
>> #ifdef CONFIG_NF_CONNTRACK_SECMARK
>> int len, ret;
>>
>> - ret = security_secid_to_secctx(ct->secmark, NULL, &len);
>> - if (ret)
>> + ret = security_secid_to_secctx(ct->secmark, NULL);
> This breaks here.
>
> len is really used, this should be instead:
>
> ret = security_secid_to_secctx(ct->secmark, &ctx);
>
> [...]
> return nla_total_size(0) /* CTA_SECCTX */
> + nla_total_size(sizeof(char) * ctx.len); /* CTA_SECCTX_NAME */
> #else
> return 0;
> #endif
> }
I'll fix that.
>> + if (ret < 0)
>> return 0;
>>
>> return nla_total_size(0) /* CTA_SECCTX */
>> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
>> index 5f7fd23b7afe..502cf10aab41 100644
>> --- a/net/netfilter/nf_conntrack_standalone.c
>> +++ b/net/netfilter/nf_conntrack_standalone.c
>> @@ -175,8 +175,8 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
>> struct lsm_context ctx;
>> int ret;
>>
>> - ret = security_secid_to_secctx(ct->secmark, &ctx.context, &ctx.len);
>> - if (ret)
>> + ret = security_secid_to_secctx(ct->secmark, &ctx);
>> + if (ret < 0)
>> return;
>>
>> seq_printf(s, "secctx=%s ", ctx.context);
>> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
>> index 37757cd77cf1..5110f29b2f40 100644
>> --- a/net/netfilter/nfnetlink_queue.c
>> +++ b/net/netfilter/nfnetlink_queue.c
>> @@ -470,18 +470,18 @@ static int nfqnl_put_sk_classid(struct sk_buff *skb, struct sock *sk)
>> return 0;
>> }
>>
>> -static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
>> +static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsm_context *ctx)
>> {
>> u32 seclen = 0;
>> #if IS_ENABLED(CONFIG_NETWORK_SECMARK)
>> +
> remove unneeded line.
Will do.
>> if (!skb || !sk_fullsock(skb->sk))
>> return 0;
>>
>> read_lock_bh(&skb->sk->sk_callback_lock);
>>
>> if (skb->secmark)
>> - security_secid_to_secctx(skb->secmark, secdata, &seclen);
>> -
>> + seclen = security_secid_to_secctx(skb->secmark, ctx);
>> read_unlock_bh(&skb->sk->sk_callback_lock);
>> #endif
>> return seclen;
>> @@ -567,8 +567,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>> enum ip_conntrack_info ctinfo = 0;
>> const struct nfnl_ct_hook *nfnl_ct;
>> bool csum_verify;
>> - struct lsm_context scaff; /* scaffolding */
>> - char *secdata = NULL;
>> + struct lsm_context ctx;
> Help us make this get closer to revert xmas tree:
>
> enum ip_conntrack_info ctinfo = 0;
> const struct nfnl_ct_hook *nfnl_ct;
> + struct lsm_context ctx;
> bool csum_verify;
> - struct lsm_context scaff; /* scaffolding */
> - char *secdata = NULL;
Will do.
>> bool csum_verify;
>> - struct lsm_context scaff; /* scaffolding */
>> - char *secdata = NULL;
>> u32 seclen = 0;
>> ktime_t tstamp;
>>
>> @@ -643,8 +642,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>> }
>>
>> if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
>> - seclen = nfqnl_get_sk_secctx(entskb, &secdata);
>> - if (seclen)
>> + seclen = nfqnl_get_sk_secctx(entskb, &ctx);
>> + if (seclen >= 0)
>> size += nla_total_size(seclen);
>> }
>>
>> @@ -783,7 +782,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>> if (nfqnl_put_sk_classid(skb, entskb->sk) < 0)
>> goto nla_put_failure;
>>
>> - if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
>> + if (seclen && nla_put(skb, NFQA_SECCTX, ctx.len, ctx.context))
>> goto nla_put_failure;
>>
>> if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0)
>> @@ -811,10 +810,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>> }
>>
>> nlh->nlmsg_len = skb->len;
>> - if (seclen) {
>> - lsmcontext_init(&scaff, secdata, seclen, 0);
>> - security_release_secctx(&scaff);
>> - }
>> + if (seclen >= 0)
>> + security_release_secctx(&ctx);
>> return skb;
>>
>> nla_put_failure:
>> @@ -822,10 +819,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>> kfree_skb(skb);
>> net_err_ratelimited("nf_queue: error creating packet message\n");
>> nlmsg_failure:
>> - if (seclen) {
>> - lsmcontext_init(&scaff, secdata, seclen, 0);
>> - security_release_secctx(&scaff);
>> - }
>> + if (seclen >= 0)
>> + security_release_secctx(&ctx);
>> return NULL;
>> }
>>
Powered by blists - more mailing lists