A missing check bug before calling ip_route_output_flow()

[Jinmeng Zhou]

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!

Function ipv4_sk_update_pmtu() lacks a LSM check, a.k.a
security_sk_classify_flow(),
before calling ip_route_output_flow().

Specifically, we find that ip_route_output_flow() is used at 18 places in total.
In most cases, either the function is placed behind the security check
security_sk_classify_flow() or its last parameter is NULL.
(i.e., if the last parameter of ip_route_output_flow() is NULL,
usually, there may be no need to do a security check.)

However, we find only two usages in function ipv4_sk_update_pmtu(),
the last parameter is not NULL as well as 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(),
__udp4_lib_err().
They are likely to do the error handling.
Therefore, we think there is a missing check bug before calling
ip_route_output_flow().
---------------------------------------------------------------------------------
Original email:

> Thu, 6 May 2021 11:01:24 -0700 Jakub Kicinski <kuba@xxxxxxxxxx> 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


Thanks again!

Best regards,
Jinmeng Zhou