Re: [PATCH]: BUILD: insecure-setuid-wanted support on FreeBSD
On Thu, Dec 08, 2022 at 08:40:02AM +, David CARLIER wrote: > ping :) is the mailing list still the way for patches or is github more > appropriate for better traceability ? The mailing list is still preferred for patches, but I previously got the impression that you resent the same. Better make it clear when it's updated if that's the case. Will have a look, thanks. Willy
Re: [PATCH]: BUILD: insecure-setuid-wanted support on FreeBSD
ping :) is the mailing list still the way for patches or is github more appropriate for better traceability ? On Mon, 7 Nov 2022 at 13:37, David CARLIER wrote: > I sent a new one after, resending in case it got lost. > > On Mon, 7 Nov 2022 at 12:36, Tim Düsterhus wrote: > >> David, >> >> On 11/7/22 09:00, David CARLIER wrote: >> > Thanks here a corrected version. >> > >> >> It looks like you accidentally attached the same patch as before. >> >> Best regards >> Tim Düsterhus >> >
Re: [PATCH]: BUILD: insecure-setuid-wanted support on FreeBSD
I sent a new one after, resending in case it got lost. On Mon, 7 Nov 2022 at 12:36, Tim Düsterhus wrote: > David, > > On 11/7/22 09:00, David CARLIER wrote: > > Thanks here a corrected version. > > > > It looks like you accidentally attached the same patch as before. > > Best regards > Tim Düsterhus > 0001-BUILD-insecure-setuid-wanted-support-on-FreeBSD.patch Description: Binary data
Re: [PATCH]: BUILD: insecure-setuid-wanted support on FreeBSD
David, On 11/7/22 09:00, David CARLIER wrote: Thanks here a corrected version. It looks like you accidentally attached the same patch as before. Best regards Tim Düsterhus
Re: [PATCH]: BUILD: insecure-setuid-wanted support on FreeBSD
Thanks here a corrected version.
Regards.
On Mon, 7 Nov 2022 at 07:01, Willy Tarreau wrote:
> Hi David,
>
> On Fri, Nov 04, 2022 at 07:34:46PM +, 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
> > Date: Fri, 4 Nov 2022 19:24:03 +
> > 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(_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(_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,
> _setuid) == -1
> > +#endif
> > + && !_HA_ATOMIC_FETCH_ADD(_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
Re: [PATCH]: BUILD: insecure-setuid-wanted support on FreeBSD
Hi David,
On Fri, Nov 04, 2022 at 07:34:46PM +, 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
> Date: Fri, 4 Nov 2022 19:24:03 +
> 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(_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(_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,
> _setuid) == -1
> +#endif
> + && !_HA_ATOMIC_FETCH_ADD(_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
