Thanks here a corrected version. Regards.
On Mon, 7 Nov 2022 at 07:01, Willy Tarreau <w...@1wt.eu> wrote:
> 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 <devne...@gmail.com>
> > 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
>
0001-BUILD-insecure-setuid-wanted-support-on-FreeBSD.patch
Description: Binary data
