Re: [nft PATCH v4 2/6] src: add input flag NFT_CTX_INPUT_NO_DNS to avoid blocking

[Phil Sutter <phil@nwl.cc>]

On Thu, Aug 10, 2023 at 10:43:38AM +0200, Thomas Haller wrote:
> Hi Phil,
> 
> On Thu, 2023-08-10 at 09:45 +0200, Phil Sutter wrote:
> > On Wed, Aug 09, 2023 at 09:17:36PM +0200, Thomas Haller wrote:
> > 
> > 
> > > 
> > > It seems prudent that libnftables provides a mode of operation so
> > > that
> > > it doesn't block the calling application. Otherwise, it is a
> > > problem
> > > for applications that care about that.
> > 
> > Hmm. In that case, one might also have to take care of calls to
> > getprotobyname() and maybe others (getaddrinfo()?). Depending on
> > nsswitch.conf it may block, too, right?
> 
> getaddrinfo() is avoided by NFT_CTX_INPUT_NO_DNS.
> 
> ... except at one place in `inet_service_type_parse()`, where
> getaddrinfo() is used to parse the service. I don't think that has any
> reason to block(*), has it?.
> 
> getprotobyname() also should not block(*)  as it merely reads
> /etc/protocols (in musl it's even hard-coded).
> 
> 
> (*) reading from /etc or talking netlink to kernel is sufficiently fast
> so I consider it "non-blocking".

I think these functions support /etc/nsswitch.conf, so feeding them
names is not necessarily only a local lookup. But anyway, this doesn't
quite matter: /etc/nsswitch.conf is in control of the user, just as is
the input fed into run_cmd_from_*(). So for the sake of the discussion
here, it doesn't make a difference.

> In the first version, the flag was called NFT_CTX_NO_BLOCK. It had the
> goal to avoid any significant blocking. The flag got renamed to
> NFT_CTX_INPUT_NO_DNS, which on the surface has the different goal to
> only accept plain IP addresses.
> 
> If there are other places that still can block, they should be
> identified and addressed. But that's then separate from NO_DNS flag.

Yes, I believe the various name to number lookups are the only potential
blockers.

> > > > > And that the application doesn't make a mistake with that
> > > > > ([1]).
> > > > > 
> > > > > [1]
> > > > > https://github.com/firewalld/firewalld/commit/4db89e316f2d60f3cf856a7025a96a61e40b1e5a
> > > > 
> > > > This is just a bug in firewall-cmd, missing to convert ranges
> > > > into
> > > > JSON
> > > > format. I don't see the benefit for users which no longer may use
> > > > host
> > > > names in that spot.
> > > 
> > > Which spot do you mean? /sbin/nft is not affected, unless it opts-
> > > in to
> > > the new flag. firewalld never supported hostnames at that spot
> > > anyway
> > > (or does it?).
> > 
> > I'm pretty sure it does, albeit maybe not officially.
> 
> That would be important to verify. I will check, thank you.

Did you find time for it already?

Cheers, Phil