Re: [conntrack-tools PATCH] nfct: Support for non-lazy binding

[Phil Sutter <phil@nwl.cc>]

Hi,

On Tue, Mar 08, 2022 at 08:32:00PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Feb 08, 2022 at 05:01:00PM +0100, Phil Sutter wrote:
> > For security purposes, distributions might want to pass -Wl,-z,now
> > linker flags to all builds, thereby disabling lazy binding globally.
> > 
> > In the past, nfct relied upon lazy binding: It uses the helper objects'
> > parsing functions without but doesn't provide all symbols the objects
> > use.
> > 
> > Add a --disable-lazy configure option to add those missing symbols to
> > nfct so it may be used in those environments.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> > This patch supersedes the previously submitted "Merge nfct tool into
> > conntrackd", providing a solution which is a) optional and b) doesn't
> > bloat nfct-only use-cases that much.
> > ---
> >  configure.ac    | 12 ++++++++++--
> >  src/Makefile.am |  7 +++++++
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index b12b722a3396d..43baf8244ad64 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -48,6 +48,9 @@ AC_ARG_ENABLE([cttimeout],
> >  AC_ARG_ENABLE([systemd],
> >          AS_HELP_STRING([--enable-systemd], [Build systemd support]),
> >          [enable_systemd="$enableval"], [enable_systemd="no"])
> > +AC_ARG_ENABLE([lazy],
> > +        AS_HELP_STRING([--disable-lazy], [Disable lazy binding in nfct]),
> > +        [enable_lazy="$enableval"], [enable_lazy="yes"])
> >  
> >  AC_CHECK_HEADER([rpc/rpc_msg.h], [AC_SUBST([LIBTIRPC_CFLAGS],'')], [PKG_CHECK_MODULES([LIBTIRPC], [libtirpc])])
> >  
> > @@ -78,7 +81,11 @@ AC_CHECK_HEADERS(arpa/inet.h)
> >  AC_CHECK_FUNCS(inet_pton)
> >  
> >  # Let nfct use dlopen() on helper libraries without resolving all symbols.
> > -AX_CHECK_LINK_FLAG([-Wl,-z,lazy], [AC_SUBST([LAZY_LDFLAGS], [-Wl,-z,lazy])])
> > +AS_IF([test "x$enable_lazy" = "xyes"], [
> > +	AX_CHECK_LINK_FLAG([-Wl,-z,lazy],
> > +			   [AC_SUBST([LAZY_LDFLAGS], [-Wl,-z,lazy])])
> > +])
> > +AM_CONDITIONAL([HAVE_LAZY], [test "x$enable_lazy" = "xyes"])
> >  
> >  if test ! -z "$libdir"; then
> >  	MODULE_DIR="\\\"$libdir/conntrack-tools/\\\""
> > @@ -92,4 +99,5 @@ echo "
> >  conntrack-tools configuration:
> >    userspace conntrack helper support:	${enable_cthelper}
> >    conntrack timeout support:		${enable_cttimeout}
> > -  systemd support:			${enable_systemd}"
> > +  systemd support:			${enable_systemd}
> > +  use lazy binding:                     ${enable_lazy}"
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 1d56394698a68..95cff7d528d44 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -18,6 +18,9 @@ nfct_SOURCES = nfct.c
> >  if HAVE_CTHELPER
> >  nfct_SOURCES += helpers.c			\
> >  		nfct-extensions/helper.c
> > +if !HAVE_LAZY
> > +nfct_SOURCES += expect.c utils.c
> > +endif
> 
> If the problem are the symbols in these two files, could you just
> build them always into nfct? No need for the extra --disable-lazy at
> ./configure time.

Not just them, the remaining patch is required also:

[...]
> >  if HAVE_CTHELPER
> >  nfct_LDADD += ${LIBNETFILTER_CTHELPER_LIBS}
> > +if !HAVE_LAZY
> > +nfct_LDADD += ${LIBNETFILTER_CONNTRACK_LIBS} \
> > +	      ${LIBNETFILTER_QUEUE_LIBS}
> > +endif
> >  endif

Without the extra libraries, expect.c fails linking, also dlopen() will
fail without lazy binding. If you consider the extra 10k binary size and
the extra library dependencies negligible, that's fine with me. I'll
then resubmit after dropping the whole HAVE_LAZY logic.

Cheers, Phil