Received-SPF: pass (google.com: domain of linux-kernel+bounces-12154-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20231224165413.831486-1-linma@zju.edu.cn> <CAM0EoMm8F3UE3N-PBZmJHQpYYjiV23JKf6jGsvzzWs0PBd+AWQ@mail.gmail.com>
 <6aab36aa.56337.18ca3c6af7a.Coremail.linma@zju.edu.cn>
In-Reply-To: <6aab36aa.56337.18ca3c6af7a.Coremail.linma@zju.edu.cn>
From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Wed, 27 Dec 2023 12:02:49 -0500
Message-ID: <CAM0EoMmBp6SWDGhPkusnx0jh4y=1k9ggS+5UpV+0MtEccDgyXw@mail.gmail.com>
Subject: Re: [PATCH net v1] net/sched: cls_api: complement tcf_tfilter_dump_policy
To: Lin Ma <linma@zju.edu.cn>
Cc: xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net, 
	edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, 
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Dec 25, 2023 at 8:39=E2=80=AFPM 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]             =3D { .type =3D 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