Hi David, On Fri, Nov 04, 2022 at 07:34:46PM +0000, David CARLIER wrote: > Hi, > > here a little patch to port the insecure-setuid-wanted directive on FreeBSD.
Thanks. I'm having a few comments below. > From be693024d7e49173f7ff37566232238fc5ea1887 Mon Sep 17 00:00:00 2001 > From: David CARLIER <[email protected]> > Date: Fri, 4 Nov 2022 19:24:03 +0000 > Subject: [PATCH] BUILD: insecure-setuid-wanted support on FreeBSD. > > using the procctl api to ignore the suid/sgid bits to be ignored. > --- > src/haproxy.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/src/haproxy.c b/src/haproxy.c > index 806497062..94b9bde4e 100644 > --- a/src/haproxy.c > +++ b/src/haproxy.c > @@ -3003,7 +3003,7 @@ static void *run_thread_poll_loop(void *data) > pthread_mutex_unlock(&init_mutex); > #endif > > -#if defined(PR_SET_NO_NEW_PRIVS) && defined(USE_PRCTL) > +#if (defined(PR_SET_NO_NEW_PRIVS) && defined(USE_PRCTL)) || > (defined(PROC_NO_NEW_PRIVS_CTL) && defined(USE_PROCCTL)) At this point this should move to compat.h, to do something like this for example: #if (defined(PR_SET_NO_NEW_PRIVS) && defined(USE_PRCTL)) || \ /* linux */ (defined(PROC_NO_NEW_PRIVS_CTL) && defined(USE_PROCCTL)) /* freebsd */ #define HAVE_NO_NEW_PRIVS #endif Then here you'd just use: #ifdef HAVE_NO_NEW_PRIVS > /* Let's refrain from using setuid executables. This way the impact of > * an eventual vulnerability in a library remains limited. It may > * impact external checks but who cares about them anyway ? In the > @@ -3014,7 +3014,14 @@ static void *run_thread_poll_loop(void *data) > */ > if (!(global.tune.options & GTUNE_INSECURE_SETUID) && !master) { > static int warn_fail; > - if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) == -1 && > !_HA_ATOMIC_FETCH_ADD(&warn_fail, 1)) { > +#if defined(USE_PRCTL) > + if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) == -1 > +#else > + int refrain_setuid = PROC_NO_NEW_PRIVS_ENABLE; > + /* we can save one syscall once freebsd 14 becomes the minimum > version, removing getpid */ > + if (procctl(P_PID, getpid(), PROC_NO_NEW_PRIVS_CTL, > &refrain_setuid) == -1 > +#endif > + && !_HA_ATOMIC_FETCH_ADD(&warn_fail, 1)) { And the part above is not nice, it breaks an "if" expression making it much harder to follow, which is particularly problematic in code aimed at improving security. I wouldn't be surprised if it would even break auto-indent or parens matching on some editors. Better put that in a separate function such as ha_no_new_privs() which calls the suitable syscall and arguments depending on which approach is desired. It could be left in this file I guess, even though for the long term I think we'll finally create an "src/sys.c" file to support system specific abstraction but we'd rather do that later. Thanks, Willy
