Fwd: the missing check bug before calling ip_route_output_flow().

[Jinmeng Zhou <jjjinmeng.zhou@gmail.com>]

Dear maintainers,

hi, our team has found and reported a missing check bug on Linux
kernel v5.10.7 using static analysis.
We are looking forward to having more experts' eyes on this. Thank you!


> Thu, 6 May 2021 11:01:24 -0700 Jakub Kicinski <kuba@kernel.org> wrote:
> > On Thu, 6 May 2021 15:50:33 +0800 Jinmeng Zhou wrote:
> > hi, our team has found a missing check bug on Linux kernel v5.10.7 using
> static analysis.
> > We think there is a missing check bug in ip_route_output_key() before calling
> function ip_route_output_flow().
>
> Thank you for the report!
>
> > There is a check calls to security_sk_classify_flow() in function ip_route_newports().
> > 1. // check security_sk_classify_flow() ///////////////
> > 2. static inline struct rtable *ip_route_newports(struct flowi4 *fl4, struct rtable *rt,
> > 3.       __be16 orig_sport, __be16 orig_dport,
> > 4.       __be16 sport, __be16 dport,
> > 5.       struct sock *sk)
> > 6. {
> > 7. ...
> > 8.   security_sk_classify_flow(sk, flowi4_to_flowi(fl4));
> > 9.   return ip_route_output_flow(sock_net(sk), fl4, sk);
> > 10. ...
> > 11. }
> >
> > While, ip_route_output_key() does not have check.
> > 1. // no check ////////////////////////////////////
> > 2. static inline struct rtable *ip_route_output_key(struct net *net, struct flowi4 *flp)
> > 3. {
> > 4.   return ip_route_output_flow(net, flp, NULL);
> > 5. }
> >
> > On the path from user-reachable function to ip_route_output_key() also does not contain this check. There is a call chain:
> > nft_reject_ipv4_eval() =>
> > nf_send_reset() =>
>
> This path looks like ICMP reject path, so it's not run in a context of
> any process, I'm not sure security checks make sense in such context.
> But again please circulate the report more widely, add people who have
> touched the code in the past and relevant mailing lists.
>
> > ip_route_me_harder() =>
> > ip_route_output_key()
> >
> > 1. static const struct nft_expr_ops nft_reject_ipv4_ops = {
> > 2.
> > 3.   .eval = nft_reject_ipv4_eval,
> > 4.
> > 5. };
> > 6. static int __init nft_reject_ipv4_module_init(void)
> > 7. {
> > 8.   return nft_register_expr(&nft_reject_ipv4_type);
> > 9. }
> > 10. module_init(nft_reject_ipv4_module_init);
> >
> > Therefore we think the buggy function can be triggered.
> >
> > Thanks!
> >
> >
> > Best regards,
> > Jinmeng Zhou


We want to add further explanation.
We find that ip_route_output_flow() is used at 18 places in total.
In most cases, the function is placed behind the security check
security_sk_classify_flow() or its last parameter is NULL.

We also observe if the last parameter of ip_route_output_flow() is NULL,
usually, there will be no security check.

However, we find only 2 usages in function ipv4_sk_update_pmtu() where
the last parameter is not NULL and there is no security check.


1. void ipv4_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, u32 mtu)
2. {
3.    ...
4.    if (odst->obsolete && !odst->ops->check(odst, 0)) {
5.    rt = ip_route_output_flow(sock_net(sk), &fl4, sk);
6.    if (IS_ERR(rt))
7.      goto out;
8.
9.       new = true;
10.   }
11.
12.   __ip_rt_update_pmtu((struct rtable *)xfrm_dst_path(&rt->dst), &fl4, mtu);
13.
14.   if (!dst_check(&rt->dst, 0)) {
15.     if (new)
16.       dst_release(&rt->dst);
17.
18.     rt = ip_route_output_flow(sock_net(sk), &fl4, sk);
19.     if (IS_ERR(rt))
20.       goto out;
21.
22.     new = true;
23.   }
24.   ...
25. }

ipv4_sk_update_pmtu() is called by 3 callers, ping_err(), raw_err()
and __udp4_lib_err().
They are likely to handle the error condition.


Thanks!

Best regards,
Jinmeng Zhou