Re: [PATCH 3/3] udp: Check for answers to forwarded DNS queries before handling local redirects

[Stefano Brivio <sbrivio@redhat.com>]

On Sat, 5 Nov 2022 12:19:55 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Nov 03, 2022 at 07:42:51AM +0100, Stefano Brivio wrote:
> > On Thu, 3 Nov 2022 14:42:13 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Thu, Nov 03, 2022 at 12:04:43AM +0100, Stefano Brivio wrote:  
> > > > Now that we allow loopback DNS addresses to be used as targets for
> > > > forwarding, we need to check if DNS answers come from those targets,
> > > > before deciding to eventually remap traffic for local redirects.
> > > > 
> > > > Otherwise, the source address won't match the one configured as
> > > > forwarder, which means that the guest or the container will refuse
> > > > those responses.
> > > > 
> > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > > ---
> > > >  udp.c | 17 ++++++++---------
> > > >  1 file changed, 8 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/udp.c b/udp.c
> > > > index 4b201d3..7c77e09 100644
> > > > --- a/udp.c
> > > > +++ b/udp.c
> > > > @@ -680,8 +680,10 @@ static void udp_sock_fill_data_v4(const struct ctx *c, int n,
> > > >  	src = ntohl(b->s_in.sin_addr.s_addr);
> > > >  	src_port = ntohs(b->s_in.sin_port);
> > > >  
> > > > -	if (src >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET ||
> > > > -	    src == INADDR_ANY || src == ntohl(c->ip4.addr_seen)) {
> > > > +	if (c->ip4.dns_fwd && src == htonl(c->ip4.dns[0]) && src_port == 53) {    
> > > 
> > > I guess this is not a newly introduced bug, but for the case of
> > > multiple host nameservers, don't you need to check against everything
> > > in the ip4.dns[] array, not just entry 0?  
> > 
> > No, because that's the only one we're using as target for forwarded
> > queries -- and DNS answers we want to check here are only the forwarded
> > ones.  
> 
> *thinks*  .. ok, that makes sense.  But if that's the case, won't
> ip4.dns[0] be the only entry in ip4.dns[] we use for anything at all?
> Can we drop the table and just keep one entry?

Now that we have ip{4,6}.dns_send[], yes.

We could rename .dns_send[] back to .dns[] and change the current
.dns[] to .own_dns, or .fwd_dns_target, something like that. Other naming
ideas welcome.

I wanted the change in 2/3 to be simple and fix-like, but I can do this
rework soon so that you don't _have_ to. :)

-- 
Stefano