On Wed, Jul 10, 2024 at 07:13:26PM +0200, Stefano Brivio wrote:
> On Wed, 10 Jul 2024 10:23:14 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Jul 10, 2024 at 12:32:33AM +0200, Stefano Brivio wrote:
> > > On Fri,  5 Jul 2024 12:07:18 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
[snip]
> > > > +		 * need to.
> > > > +		 */
> > > > +		/* cppcheck-suppress nullPointer */
> > > > +		while (recv(uflow->s[TGTSIDE], NULL, 0, MSG_DONTWAIT) >= 0)
> > > > +			;  
> > > 
> > > Could a local attacker (another user) attempt to use this for denial of
> > > service?  
> > 
> > Ah, interesting question.
> > 
> > > Of course, somebody could flood us anyway and we would get and handle
> > > all the events that that causes. But this case is different because we
> > > could get stuck for an unlimited amount of time without serving other
> > > sockets at all.  
> > 
> > Right.
> > 
> > > If that's a possibility, perhaps a limit for this loop (a maximum
> > > amount of recv()) tries would be a good idea. I'm not sure how we should
> > > handle the case where we exceed the threshold.  
> > 
> > We could fail to create the flow.  That would limit the damage, but
> > see below.
> > 
> > > Another one, which adds some complexity, but looks more correct to me,
> > > would be to try a single recv() call, and if we get data from it, fail
> > > creating the new flow entirely.  
> > 
> > Right.  I also considered a single recvmmsg(); the difficulty with
> > that is making suitable arrays for it, since the normal ones may be in
> > use at this point.
> 
> For UDP, we don't need to make the buffers large enough to fit packets
> into them, see udp(7):
> 
>   All receive operations return only one packet. When the packet is
>   smaller than the passed buffer, only that much data is returned; when it
>   is bigger, the packet is truncated and the MSG_TRUNC flag is set.
> 
> so probably 1024 bytes (1 * UIO_MAXIOV) on the stack would be enough...
> or maybe we can even pass 0 as size and NULL buffers?

We can (see doc/platform-requirements/recv-zero.c).  And even if we
couldn't, we could use the same 1 byte buffer for all the datagrams.
We basically just need the actual msghdr array.

> If recvmmsg() returns UIO_MAXIOV, then something weird is going on and
> we can abort the "connection" attempt.

Seems reasonable; I've coded something up.

> > This removes the potential DoS of wedging passt completely, but raises
> > the possibility of a different one.  Could an attacker - not
> > necessarily on the same host, but network-closer than the guest's
> > intended endpoint - prevent the guest from connecting to a particular
> > service by spamming fake reply packets here.
> 
> Oops, I didn't think about that, that's also concerning. With my
> suggestion above, 1024 packets is all an attacker would need.
> 
> But maybe there's a safe way to deal with this that's still simpler
> than a separate handler: using recvmmsg(), going through msg_name of
> the (truncated) messages we received with it, and trying to draw some
> conclusion about what kind of attack we're dealing with from there. I'm
> not sure exactly which conclusion, though.
> 
> > I believe it would need to anticipate the guest's source port to do so,
> > which might be good enough.
> 
> Linux does source port randomisation for UDP starting from 2.6.24, so
> that should be good enough, but given that we try to preserve the
> source port from the guest, we might make a guest that doesn't do port
> randomisation (albeit unlikely to be found) vulnerable to this.

Right.  It feels like we're at least not making the guest dramatically
more vulnerable to something here, so I'm inclined to treat it as good
enough for now.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson