Re: pledge net/ngircd

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: pledge net/ngircd

Matthias Schmidt
Hi Michael,

On 10.02.2020 14:31, Michael wrote:

> On Fri, Feb 07, 2020 at 03:27:33PM +0100, Michael wrote:
>> Hello ports@,
>>
>> this patch adds pledge() to net/ngircd. Tested on amd64 with ngircd
>> running with TLS. Unfortunately the promises can't be further reduced
>> since this would break /rehash (i.e. reloading the config) later. But
>> this is better than nothing.
>>
>> [...]
>
> solene@ pointed out that if the option "PidFile" is being used
> unlink()ing the file later fails. However I personally don't like
> adding
> another promise just for that. I can't see any sensible use case for
> ngircds PID file; the Option itself is not set by default.
>
> So my idea would be to either skip or remove the PidFile code, or just
> ignore the issue. The abort happens after shutting everything else down
> and starting ngircd again works even if the old PID file is still in
> place. Both variants mean changing or breaking functionality but that
> would be bearable given the low impact IMHO. Using unveil() might also
> be an option.
>
> Any thoughts on this?

Active ngircd user here. I personally use the PID file with my
monitoring system
for process supervision (monit in my case). Although I could use process
name
matching, getting the PID from the PIDFile seems more natural.

Cheers

       Matthias

PS: I would appreciate a pledged ngircd very much.

Reply | Threaded
Open this post in threaded view
|

Re: pledge net/ngircd

Solene Rapenne
On Tue, Feb 11, 2020 at 11:45:44AM +0100, Michael wrote:

> Hello Matthias,
>
> On Mon, Feb 10, 2020 at 02:43:05PM +0100, Matthias Schmidt wrote:
> > Hi Michael,
> >
> > On 10.02.2020 14:31, Michael wrote:
> > > On Fri, Feb 07, 2020 at 03:27:33PM +0100, Michael wrote:
> > > > Hello ports@,
> > > >
> > > > this patch adds pledge() to net/ngircd. Tested on amd64 with ngircd
> > > > running with TLS. Unfortunately the promises can't be further reduced
> > > > since this would break /rehash (i.e. reloading the config) later. But
> > > > this is better than nothing.
> > > >
> > > > [...]
> > >
> > > solene@ pointed out that if the option "PidFile" is being used
> > > unlink()ing the file later fails. However I personally don't like adding
> > > another promise just for that. I can't see any sensible use case for
> > > ngircds PID file; the Option itself is not set by default.
> > >
> > > So my idea would be to either skip or remove the PidFile code, or just
> > > ignore the issue. The abort happens after shutting everything else down
> > > and starting ngircd again works even if the old PID file is still in
> > > place. Both variants mean changing or breaking functionality but that
> > > would be bearable given the low impact IMHO. Using unveil() might also
> > > be an option.
> > >
> > > Any thoughts on this?
> >
> > Active ngircd user here. I personally use the PID file with my monitoring
> > system
> > for process supervision (monit in my case). Although I could use process
> > name
> > matching, getting the PID from the PIDFile seems more natural.
> >
> > Cheers
> >
> >       Matthias
> >
> > PS: I would appreciate a pledged ngircd very much.
> >
>
> Yes, pledge() for ngircd is long overdue :) Below is an updated patch
> that handles having PidFile configured. I am unsure if details like
> having PidFile set enables the "cpath" promise should be documented or
> not.
>
> Index: Makefile
> ===================================================================
> RCS file: /cvs/ports/net/ngircd/Makefile,v
> retrieving revision 1.18
> diff -u -p -u -p -r1.18 Makefile
> --- Makefile 12 Jul 2019 20:48:34 -0000 1.18
> +++ Makefile 11 Feb 2020 10:24:57 -0000
> @@ -4,6 +4,8 @@ COMMENT = lightweight irc server
>  
>  DISTNAME = ngircd-25
>  
> +REVISION = 0
> +
>  CATEGORIES = net
>  
>  HOMEPAGE = https://ngircd.barton.de/
> Index: patches/patch-src_ngircd_ngircd_c
> ===================================================================
> RCS file: /cvs/ports/net/ngircd/patches/patch-src_ngircd_ngircd_c,v
> retrieving revision 1.4
> diff -u -p -u -p -r1.4 patch-src_ngircd_ngircd_c
> --- patches/patch-src_ngircd_ngircd_c 3 Dec 2014 10:32:18 -0000 1.4
> +++ patches/patch-src_ngircd_ngircd_c 11 Feb 2020 10:24:57 -0000
> @@ -1,7 +1,25 @@
>  $OpenBSD: patch-src_ngircd_ngircd_c,v 1.4 2014/12/03 10:32:18 jasper Exp $
> ---- src/ngircd/ngircd.c.orig Mon Jul 14 13:26:07 2014
> -+++ src/ngircd/ngircd.c Tue Dec  2 20:05:31 2014
> -@@ -563,7 +563,7 @@ Setup_FDStreams(int fd)
> +Index: src/ngircd/ngircd.c
> +--- src/ngircd/ngircd.c.orig
> ++++ src/ngircd/ngircd.c
> +@@ -259,6 +259,16 @@ main(int argc, const char *argv[])
> + exit(1);
> + }
> +
> ++ /* XXX using a PID file needs cpath to unlink() later */
> ++ if(Conf_PidFile[0]) {
> ++ if ( pledge("stdio inet dns rpath proc getpw cpath", NULL) == -1)
> ++ err(1, "pledge");
> ++ }
> ++ else {
> ++ if ( pledge("stdio inet dns rpath proc getpw", NULL) == -1)
> ++ err(1, "pledge");
> ++ }
> ++
> + /* Initialize modules, part II: these functions are eventually
> + * called with already dropped privileges ... */
> + Channel_Init();
> +@@ -563,7 +573,7 @@ Setup_FDStreams(int fd)
>   #if !defined(SINGLE_USER_OS)
>  
>   /**
> @@ -10,7 +28,7 @@ $OpenBSD: patch-src_ngircd_ngircd_c,v 1.
>    *
>    * @param uid User ID
>    * @param gid Group ID
> -@@ -587,7 +587,7 @@ NGIRCd_getNobodyID(uid_t *uid, gid_t *gid )
> +@@ -587,7 +597,7 @@ NGIRCd_getNobodyID(uid_t *uid, gid_t *gid )
>   }
>   #endif
>  
> @@ -19,7 +37,7 @@ $OpenBSD: patch-src_ngircd_ngircd_c,v 1.
>   if (!pwd)
>   return false;
>  
> -@@ -703,11 +703,11 @@ NGIRCd_Init(bool NGIRCd_NoDaemon)
> +@@ -703,11 +713,11 @@ NGIRCd_Init(bool NGIRCd_NoDaemon)
>   if (Conf_UID == 0) {
>   pwd = getpwuid(0);
>   Log(LOG_INFO,
> Index: patches/patch-src_ngircd_proc_c
> ===================================================================
> RCS file: patches/patch-src_ngircd_proc_c
> diff -N patches/patch-src_ngircd_proc_c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-src_ngircd_proc_c 11 Feb 2020 10:24:57 -0000
> @@ -0,0 +1,15 @@
> +$OpenBSD$
> +
> +Index: src/ngircd/proc.c
> +--- src/ngircd/proc.c.orig
> ++++ src/ngircd/proc.c
> +@@ -76,6 +76,9 @@ Proc_Fork(PROC_STAT *proc, int *pipefds, void (*cbfunc
> + return -1;
> + case 0:
> + /* New child process: */
> ++ /* XXX no PAM, fork only for DNS */
> ++ if (pledge("stdio dns", NULL) == -1)  
> ++ err(1, "pledge");
> + #ifdef HAVE_ARC4RANDOM_STIR
> + arc4random_stir();
> + #endif
>

I'm ok with the last patch, it's harmless (no special configuration
required) and will improve security.

I CC maintainer, it's up to tsg now