Re: Kernel memory corruption in CIPSO labeled TCP packets processing.

[Paul Moore <paul@paul-moore.com>]

On Tue, Jan 15, 2019 at 12:55 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 1/15/2019 9:06 AM, Nazarov Sergey wrote:
> > Hello!
> > Security modules (selinux, smack) use icmp_send for discarded incorrectly labeled network packets.
> > This could be on TCP level too (security_sock_rcv_skb -> cipso_v4_error for INET stream connection, for example).
> > icmp_send calls ip_option_echo, which uses IPCB to take compiled IP options.
> > After moving IP header data to the end of the struct tcp_skb_cb (since 3.18 kernel), this could lead to
> > kernel memory corruption when IP options copying.
>
> Can you explain how that corruption might occur?
> Do you have a test case?

Thanks for pointing this out Nazarov.

Presumably we are going to hit a problem whenever icmp_send is called
from outside the IP layer in the stack.  We fixed a similar problem a
few years back with 04f81f0154e4 ("cipso: don't use IPCB() to locate
the CIPSO IP option").

I've CC'd netdev, as I'm guessing they will have some thoughts on
this, but my initial reaction is that your proposed patch isn't as
generic as it should be for code that lives in icmp_send().  I suspect
the safe thing to do would be to call ip_options_compile() again on
skb_in and build a local copy of the ip_options struct that could then
be used in the call to __ip_options_echo(); the code could either live
in icmp_send() or some new ip_options_echo() variant
(ip_options_echo_safe()?  I dunno).  Unfortunately, calling
ip_options_compile() is going to add some overhead, and may be a
non-starter for the netdev folks, but this is error path code, so it
might be acceptable.  Hopefully the netdev folks will have some
better, more clever suggestions.

> > This patch fix a bug, but I'm not sure, that this is a best solution. Perhaps someone more familiar with the
> > linux TCP/IP stack will offer a better one.
> > Thanks.
> >
> > --- a/net/ipv4/icmp.c
> > +++ b/net/ipv4/icmp.c
> > @@ -679,7 +679,8 @@ void icmp_send(struct sk_buff *skb_in, i
> >                                         iph->tos;
> >       mark = IP4_REPLY_MARK(net, skb_in->mark);
> >
> > -     if (ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in))
> > +     if (__ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in,
> > +                     ip_hdr(skb_in)->protocol == IPPROTO_TCP ? &TCP_SKB_CB(skb_in)->header.h4.opt : &IPCB(skb_in)->opt))
> >               goto out_unlock;
> >
> >



-- 
paul moore
www.paul-moore.com