[PATCH net v1] net/sched: cls_api: complement tcf_tfilter_dump_policy

[PATCH net v1] net/sched: cls_api: complement tcf_tfilter_dump_policy

[Lin Ma]

In function `tc_dump_tfilter`, the attributes array is parsed via
tcf_tfilter_dump_policy which only describes TCA_DUMP_FLAGS. However,
the NLA TCA_CHAIN is also accessed with `nla_get_u32`. According to the
commit 5e2424708da7 ("xfrm: add forgotten nla_policy for
XFRMA_MTIMER_THRESH"), such a missing piece could lead to a potential
heap data leak.

The access to TCA_CHAIN is introduced in commit 5bc1701881e3 ("net:
sched: introduce multichain support for filters") and no nla_policy is
provided for parsing at that point. Later on, tcf_tfilter_dump_policy is
introduced in commit f8ab1807a9c9 ("net: sched: introduce terse dump
flag") while still ignoring the fact that TCA_CHAIN needs a check. This
patch does that by complementing the policy.

Note that other functions that access TCA_CHAIN, like tc_new_tfilter,
are not vulnerable as they choose rtm_tca_policy as the parsing policy
which describes the TCA_CHAIN already.

Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
 net/sched/cls_api.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 1976bd163986..2b5b8eca2ee3 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2732,6 +2732,7 @@ static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
 }
 
 static const struct nla_policy tcf_tfilter_dump_policy[TCA_MAX + 1] = {
+	[TCA_CHAIN]      = { .type = NLA_U32 },
 	[TCA_DUMP_FLAGS] = NLA_POLICY_BITFIELD32(TCA_DUMP_FLAGS_TERSE),
 };
 
-- 
2.17.1

Re: [PATCH net v1] net/sched: cls_api: complement tcf_tfilter_dump_policy

[Jamal Hadi Salim]

Hi,

On Sun, Dec 24, 2023 at 11:54 AM Lin Ma <linma@zju.edu.cn> wrote:
>
> In function `tc_dump_tfilter`, the attributes array is parsed via
> tcf_tfilter_dump_policy which only describes TCA_DUMP_FLAGS. However,
> the NLA TCA_CHAIN is also accessed with `nla_get_u32`. According to the
> commit 5e2424708da7 ("xfrm: add forgotten nla_policy for
> XFRMA_MTIMER_THRESH"), such a missing piece could lead to a potential
> heap data leak.
>

Can you clarify what "heap data leak" you are referring to?
As much as i can see any reference to NLA_TCA_CHAIN is checked for
presence before being put to use. So far that reason I  dont see how
this patch qualifies as "net". It looks like an enhancement to me
which should target net-next, unless i am missing something obvious.

cheers,
jamal

> The access to TCA_CHAIN is introduced in commit 5bc1701881e3 ("net:
> sched: introduce multichain support for filters") and no nla_policy is
> provided for parsing at that point. Later on, tcf_tfilter_dump_policy is
> introduced in commit f8ab1807a9c9 ("net: sched: introduce terse dump
> flag") while still ignoring the fact that TCA_CHAIN needs a check. This
> patch does that by complementing the policy.
>
> Note that other functions that access TCA_CHAIN, like tc_new_tfilter,
> are not vulnerable as they choose rtm_tca_policy as the parsing policy
> which describes the TCA_CHAIN already.
>
> Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
> Signed-off-by: Lin Ma <linma@zju.edu.cn>
> ---
>  net/sched/cls_api.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 1976bd163986..2b5b8eca2ee3 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -2732,6 +2732,7 @@ static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
>  }
>
>  static const struct nla_policy tcf_tfilter_dump_policy[TCA_MAX + 1] = {
> +       [TCA_CHAIN]      = { .type = NLA_U32 },
>         [TCA_DUMP_FLAGS] = NLA_POLICY_BITFIELD32(TCA_DUMP_FLAGS_TERSE),
>  };
>
> --
> 2.17.1
>

Re: [PATCH net v1] net/sched: cls_api: complement tcf_tfilter_dump_policy

[Lin Ma]

Hello Jamal,

> 
> Can you clarify what "heap data leak" you are referring to?
> As much as i can see any reference to NLA_TCA_CHAIN is checked for
> presence before being put to use. So far that reason I  dont see how
> this patch qualifies as "net". It looks like an enhancement to me
> which should target net-next, unless i am missing something obvious.
> 

Sure, thanks for your reply, (and merry Christmas :D).
I didn't mention the detail as I consider the commit message in
`5e2424708da7` could make a point. In short, the code

```
if (tca[TCA_CHAIN] && nla_get_u32(tca[TCA_CHAIN])
```

only checks if the attribute TCA_CHAIN exists but never checks about
the attribute length because that attribute is parsed by the function
nlmsg_parse_deprecated which will parse an attribute even not described
in the given policy (here, the tcf_tfilter_dump_policy).

Moreover, the netlink message is allocated via netlink_alloc_large_skb
(see net/netlink/af_netlink.c) that does not clear out the heap buffer.
Hence a malicious user could send a malicious TCA_CHAIN attribute here
without putting any payload and the above `nla_get_u32` could dereference
a dirty data that is sprayed by the user.

Other place gets TCA_CHAIN with provide policy rtm_tca_policy that has a
description.

```
[TCA_CHAIN]		= { .type = NLA_U32 },
```

and this patch aims to do so.

Unfortunately, I have not opened the exploit for CVE-2023-3773
(https://access.redhat.com/security/cve/cve-2023-3773) yet but the idea
is similar and you can take it as an example.

> cheers,
> jamal
> 

Regards
Lin

Re: [PATCH net v1] net/sched: cls_api: complement tcf_tfilter_dump_policy

[Jamal Hadi Salim]

On Mon, Dec 25, 2023 at 8:39 PM Lin Ma <linma@zju.edu.cn> wrote:
>
> Hello Jamal,
>
> >
> > Can you clarify what "heap data leak" you are referring to?
> > As much as i can see any reference to NLA_TCA_CHAIN is checked for
> > presence before being put to use. So far that reason I  dont see how
> > this patch qualifies as "net". It looks like an enhancement to me
> > which should target net-next, unless i am missing something obvious.
> >
>
> Sure, thanks for your reply, (and merry Christmas :D).
> I didn't mention the detail as I consider the commit message in
> `5e2424708da7` could make a point. In short, the code
>
> ```
> if (tca[TCA_CHAIN] && nla_get_u32(tca[TCA_CHAIN])
> ```
>
> only checks if the attribute TCA_CHAIN exists but never checks about
> the attribute length because that attribute is parsed by the function
> nlmsg_parse_deprecated which will parse an attribute even not described
> in the given policy (here, the tcf_tfilter_dump_policy).
>
> Moreover, the netlink message is allocated via netlink_alloc_large_skb
> (see net/netlink/af_netlink.c) that does not clear out the heap buffer.
> Hence a malicious user could send a malicious TCA_CHAIN attribute here
> without putting any payload and the above `nla_get_u32` could dereference
> a dirty data that is sprayed by the user.
>
> Other place gets TCA_CHAIN with provide policy rtm_tca_policy that has a
> description.
>
> ```
> [TCA_CHAIN]             = { .type = NLA_U32 },
> ```
>
> and this patch aims to do so.
>
> Unfortunately, I have not opened the exploit for CVE-2023-3773
> (https://access.redhat.com/security/cve/cve-2023-3773) yet but the idea
> is similar and you can take it as an example.
>

Sorry, still trying to follow your reasoning that this is a "net issue":
As you point out, the skb will have enough space to carry the 32 bit
value. Worst case is we read garbage. And the dump, using this garbage
chain index,  will not find the chain or will find some unintended
chain. Am i missing something?

Can you send me a repro (privately) that actually causes the "heap
data leak" if you have one?

cheers,
jamal


> > cheers,
> > jamal
> >
>
> Regards
> Lin

Re: [PATCH net v1] net/sched: cls_api: complement tcf_tfilter_dump_policy

[Jamal Hadi Salim]

On Wed, Dec 27, 2023 at 12:02 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Mon, Dec 25, 2023 at 8:39 PM Lin Ma <linma@zju.edu.cn> wrote:
> >
> > Hello Jamal,
> >
> > >
> > > Can you clarify what "heap data leak" you are referring to?
> > > As much as i can see any reference to NLA_TCA_CHAIN is checked for
> > > presence before being put to use. So far that reason I  dont see how
> > > this patch qualifies as "net". It looks like an enhancement to me
> > > which should target net-next, unless i am missing something obvious.
> > >
> >
> > Sure, thanks for your reply, (and merry Christmas :D).
> > I didn't mention the detail as I consider the commit message in
> > `5e2424708da7` could make a point. In short, the code
> >
> > ```
> > if (tca[TCA_CHAIN] && nla_get_u32(tca[TCA_CHAIN])
> > ```
> >
> > only checks if the attribute TCA_CHAIN exists but never checks about
> > the attribute length because that attribute is parsed by the function
> > nlmsg_parse_deprecated which will parse an attribute even not described
> > in the given policy (here, the tcf_tfilter_dump_policy).
> >
> > Moreover, the netlink message is allocated via netlink_alloc_large_skb
> > (see net/netlink/af_netlink.c) that does not clear out the heap buffer.
> > Hence a malicious user could send a malicious TCA_CHAIN attribute here
> > without putting any payload and the above `nla_get_u32` could dereference
> > a dirty data that is sprayed by the user.
> >
> > Other place gets TCA_CHAIN with provide policy rtm_tca_policy that has a
> > description.
> >
> > ```
> > [TCA_CHAIN]             = { .type = NLA_U32 },
> > ```
> >
> > and this patch aims to do so.
> >
> > Unfortunately, I have not opened the exploit for CVE-2023-3773
> > (https://access.redhat.com/security/cve/cve-2023-3773) yet but the idea
> > is similar and you can take it as an example.
> >
>
> Sorry, still trying to follow your reasoning that this is a "net issue":
> As you point out, the skb will have enough space to carry the 32 bit
> value. Worst case is we read garbage. And the dump, using this garbage
> chain index,  will not find the chain or will find some unintended
> chain. Am i missing something?
>
> Can you send me a repro (privately) that actually causes the "heap
> data leak" if you have one?
>

To clarify what triggered me is your tie of this as an exploit and
quoting CVEs. Maybe not so much net vs net-next.

cheers,
jamal

> cheers,
> jamal
>
>
> > > cheers,
> > > jamal
> > >
> >
> > Regards
> > Lin

Re: [PATCH net v1] net/sched: cls_api: complement tcf_tfilter_dump_policy

[Lin Ma]

Hello Jamal,

> >
> > Sorry, still trying to follow your reasoning that this is a "net issue":
> > As you point out, the skb will have enough space to carry the 32 bit
> > value. Worst case is we read garbage. And the dump, using this garbage
> > chain index,  will not find the chain or will find some unintended
> > chain. Am i missing something?

Thanks for your replying. 

I investigated the code and yes, as you said, the skb data will carry
a tailing space used for putting `struct skb_shared_info`. Hence, 32
bit is not enough here to conduct an overflow read to next object.

Hence I guess you have not missed anything but I do. For the
CVE-2023-3773, the read value is dumped to user-space so the leak is
direct. But since the chain index is not directly dumped
into userspace. The attacker can only exploit this via a side-channel
manner. Assuming the attacker could create as many chain as he can
(2**32 maybe ;P), then the dump from the garbage chain index will leak
the kernel data indirectly.

> >
> > Can you send me a repro (privately) that actually causes the "heap
> > data leak" if you have one?
> 
> To clarify what triggered me is your tie of this as an exploit and
> quoting CVEs. Maybe not so much net vs net-next.

There may be a misunderstanding here. I didn't write such a side-channel
exploit here and as you point out, this is not an easy and worthy task.
(but If you are asking the exploit for CVE-2023-3773, I will inform you
 when it is send to oss-security)

Anyway, I believe you are right. Given the fact that I ignore the
difficulty of this exploitation, such a bug rather than a vulnerability
should go to net-next instead of net. 

Shall I add any tag from you like Suggested or Reviewed?

> cheers,
> jamal
> 

Thanks
Lin