Re: [PATCH net] l2tp: Allow duplicate session creation with UDP

[Guillaume Nault <gnault@redhat.com>]

On Fri, Jan 17, 2020 at 07:29:12PM +0000, Tom Parkin wrote:
> On  Fri, Jan 17, 2020 at 17:36:27 +0100, Guillaume Nault wrote:
> > On Thu, Jan 16, 2020 at 09:23:32PM +0000, Tom Parkin wrote:
> > > On  Thu, Jan 16, 2020 at 20:05:56 +0100, Guillaume Nault wrote:
> > > > What makes me uneasy is that, as soon as the l2tp_ip or l2tp_ip6 module
> > > > is loaded, a peer can reach whatever L2TPv3 session exists on the host
> > > > just by sending an L2TP_IP packet to it.
> > > > I don't know how practical it is to exploit this fact, but it looks
> > > > like it's asking for trouble.
> > > 
> > > Yes, I agree, although practically it's only a slightly easier
> > > "exploit" than L2TP/UDP offers.
> > > 
> > > The UDP case requires a rogue packet to be delivered to the correct
> > > socket AND have a session ID matching that of one in the associated
> > > tunnel.
> > > 
> > > It's a slightly more arduous scenario to engineer than the existing
> > > L2TPv3/IP case, but only a little.
> > > 
> > In the UDP case, we have a socket connected to the peer (or at least
> > bound to a local address). That is, some local setup is needed. With
> > l2tp_ip, we don't even need to have an open socket. Everything is
> > triggered remotely. And here, "remotely" means "any host on any IP
> > network the LCCE is connected", because the destination address can
> > be any address assigned to the LCCE, even if it's not configured to
> > handle any kind of L2TP. But well, after thinking more about our L2TPv3
> > discussiong, I guess that's how the designer's of the protocol wanted.
> 
> I note that RFC 3931 provides for a cookie in the data packet header
> to mitigate against data packet spoofing (section 8.2).
>
Indeed, I forgot about the L2TPv3 cookie.

> More generally I've not tried to see what could be done to spoof
> session data packets, so I can't really comment further.  It would be
> interesting to try spoofing packets and see if the kernel code could
> do more to limit any potential problems.
> 
> > > > > For normal operation, you just need to get the wrong packet on the
> > > > > wrong socket to run into trouble of course.  In such a situation
> > > > > having a unique session ID for v3 helps you to determine that
> > > > > something has gone wrong, which is what the UDP encap recv path does
> > > > > if the session data packet's session ID isn't found in the context of
> > > > > the socket that receives it.
> > > > Unique global session IDs might help troubleshooting, but they also
> > > > break some use cases, as reported by Ridge. At some point, we'll have
> > > > to make a choice, or even add a knob if necessary.
> > > 
> > > I suspect we need to reach agreement on what RFC 3931 implies before
> > > making headway on what the kernel should ideally do.
> > > 
> > > There is perhaps room for pragmatism given that the kernel
> > > used to be more permissive prior to dbdbc73b4478, and we weren't
> > > inundated with reports of session ID clashes.
> > > 
> > To summarise, my understanding is that global session IDs would follow
> > the spirit of RFC 3931 and would allow establishing multiple L2TPv3
> > connections (tunnels) over the same 5-tuple (or 3-tuple for IP encap).
> > Per socket session IDs don't, but would allow fixing Ridge's case.
> 
> I'm not 100% certain what "per socket session IDs" means here.  Could
> you clarify?
> 
By "per socket session IDs", I mean that the session IDs have to be
interpreted in the context of their parent tunnel socket (the current
l2tp_udp_recv_core() approach). That's opposed to "global session IDs"
which have netns-wide significance (the current l2tp_ip_recv()
approach).