[PATCH nf] netfilter: ip6t_rt: fix rt0_hdr parsing in rt_mt6

[PATCH nf] netfilter: ip6t_rt: fix rt0_hdr parsing in rt_mt6

[Xin Long]

In rt_mt6(), when it's a nonlinear skb, the 1st skb_header_pointer()
only copies sizeof(struct ipv6_rt_hdr) to _route that rh points to.
The access by ((const struct rt0_hdr *)rh)->reserved will overflow
the buffer. So this access should be moved below the 2nd call to
skb_header_pointer().

Besides, after the 2nd skb_header_pointer(), its return value should
also be checked, othersize, *rp may cause null-pointer-ref.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/ipv6/netfilter/ip6t_rt.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/netfilter/ip6t_rt.c b/net/ipv6/netfilter/ip6t_rt.c
index 733c83d38b30..d25192949217 100644
--- a/net/ipv6/netfilter/ip6t_rt.c
+++ b/net/ipv6/netfilter/ip6t_rt.c
@@ -83,11 +83,7 @@ static bool rt_mt6(const struct sk_buff *skb, struct xt_action_param *par)
 		 !(rtinfo->flags & IP6T_RT_LEN) ||
 		  ((rtinfo->hdrlen == hdrlen) ^
 		   !!(rtinfo->invflags & IP6T_RT_INV_LEN)));
-	pr_debug("res %02X %02X %02X ",
-		 rtinfo->flags & IP6T_RT_RES,
-		 ((const struct rt0_hdr *)rh)->reserved,
-		 !((rtinfo->flags & IP6T_RT_RES) &&
-		   (((const struct rt0_hdr *)rh)->reserved)));
+	pr_debug("res flag %02X ", rtinfo->flags & IP6T_RT_RES);
 
 	ret = (segsleft_match(rtinfo->segsleft[0], rtinfo->segsleft[1],
 			      rh->segments_left,
@@ -107,7 +103,12 @@ static bool rt_mt6(const struct sk_buff *skb, struct xt_action_param *par)
 						       reserved),
 					sizeof(_reserved),
 					&_reserved);
+		if (!rp) {
+			par->hotdrop = true;
+			return false;
+		}
 
+		pr_debug("res value %02X ", *rp);
 		ret = (*rp == 0);
 	}
 
-- 
2.27.0

Re: [PATCH nf] netfilter: ip6t_rt: fix rt0_hdr parsing in rt_mt6

[Florian Westphal]

Xin Long <lucien.xin@gmail.com> wrote:
> In rt_mt6(), when it's a nonlinear skb, the 1st skb_header_pointer()
> only copies sizeof(struct ipv6_rt_hdr) to _route that rh points to.
> The access by ((const struct rt0_hdr *)rh)->reserved will overflow
> the buffer. So this access should be moved below the 2nd call to
> skb_header_pointer().
> 
> Besides, after the 2nd skb_header_pointer(), its return value should
> also be checked, othersize, *rp may cause null-pointer-ref.

Patch looks good but I think you can just axe these pr_debug statements
instead of moving them.

Before pr_debug conversion these statments were #if-0 out, I don't think
they'll be missed if they are removed.

Re: [PATCH nf] netfilter: ip6t_rt: fix rt0_hdr parsing in rt_mt6

[Pablo Neira Ayuso]

On Tue, Oct 12, 2021 at 12:02:04PM +0200, Florian Westphal wrote:
> Xin Long <lucien.xin@gmail.com> wrote:
> > In rt_mt6(), when it's a nonlinear skb, the 1st skb_header_pointer()
> > only copies sizeof(struct ipv6_rt_hdr) to _route that rh points to.
> > The access by ((const struct rt0_hdr *)rh)->reserved will overflow
> > the buffer. So this access should be moved below the 2nd call to
> > skb_header_pointer().
> > 
> > Besides, after the 2nd skb_header_pointer(), its return value should
> > also be checked, othersize, *rp may cause null-pointer-ref.
> 
> Patch looks good but I think you can just axe these pr_debug statements
> instead of moving them.
> 
> Before pr_debug conversion these statments were #if-0 out, I don't think
> they'll be missed if they are removed.

Agreed.

Re: [PATCH nf] netfilter: ip6t_rt: fix rt0_hdr parsing in rt_mt6

[Xin Long]

On Tue, Oct 12, 2021 at 6:30 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Tue, Oct 12, 2021 at 12:02:04PM +0200, Florian Westphal wrote:
> > Xin Long <lucien.xin@gmail.com> wrote:
> > > In rt_mt6(), when it's a nonlinear skb, the 1st skb_header_pointer()
> > > only copies sizeof(struct ipv6_rt_hdr) to _route that rh points to.
> > > The access by ((const struct rt0_hdr *)rh)->reserved will overflow
> > > the buffer. So this access should be moved below the 2nd call to
> > > skb_header_pointer().
> > >
> > > Besides, after the 2nd skb_header_pointer(), its return value should
> > > also be checked, othersize, *rp may cause null-pointer-ref.
> >
> > Patch looks good but I think you can just axe these pr_debug statements
> > instead of moving them.
> >
> > Before pr_debug conversion these statments were #if-0 out, I don't think
> > they'll be missed if they are removed.
>
> Agreed.
Posted v2. Thanks.