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

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 [off-list ref] wrote:

quoted

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.

quoted

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!

quoted

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.

quoted

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