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

[Thomas Haller <thaller@redhat.com>]

Hi,

On Wed, 2023-08-09 at 12:20 +0200, Phil Sutter wrote:
> On Tue, Aug 08, 2023 at 10:05:19PM +0200, Thomas Haller wrote:
> > On Tue, 2023-08-08 at 15:24 +0200, Phil Sutter wrote:
> > > On Thu, Aug 03, 2023 at 09:35:16PM +0200, Thomas Haller wrote:
> > > > getaddrinfo() blocks while trying to resolve the name. Blocking
> > > > the
> > > > caller of the library is in many cases undesirable. Also, while
> > > > reconfiguring the firewall, it's not clear that resolving names
> > > > via
> > > > the network will work or makes sense.
> > > > 
> > > > Add a new input flag NFT_CTX_INPUT_NO_DNS to opt-out from
> > > > getaddrinfo()
> > > > and only accept plain IP addresses.
> > > 
> > > This sounds like user input validation via backend. Another way
> > > to
> > > solve
> > > the problem at hand is to not insert host names into the
> > > rules(et)
> > > fed
> > > into libnftables, right?
> > 
> > Right. More generally, ensure not to pass any non-addresses in JSON
> > that would be resolved.
> 
> Well, detecting if a string constitutes a valid IP address is rather
> trivial. In Python, there's even 'ipaddress' module for that job.

firewalld messed it up, showing that it can happen.

> 
> > Which requires that the user application is keenly aware,
> > understands
> > and validates the input data. For example, there couldn't be a
> > "expert
> > option" where the admin configures arbitrary JSON.
> 
> Why is host resolution a problem in such scenario? The fact that
> using
> host names instead of IP addresses may result in significant delays
> due
> to the required DNS queries is pretty common knowledge among system
> administrators.


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.


> 
> > 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?).


Thomas