Re: pledge net/ngircd
On Fri, Feb 14, 2020 at 06:31:50PM +0100, Solene Rapenne wrote:
> [...]
> 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
>
Ping?
While we are there, mentioned pledge() in the Makefile, new patch below:
Index: Makefile
===
RCS file: /cvs/ports/net/ngircd/Makefile,v
retrieving revision 1.18
diff -u -p -u -p -r1.18 Makefile
--- Makefile12 Jul 2019 20:48:34 - 1.18
+++ Makefile21 Feb 2020 12:36:53 -
@@ -4,6 +4,8 @@ COMMENT = lightweight irc server
DISTNAME = ngircd-25
+REVISION = 0
+
CATEGORIES = net
HOMEPAGE = https://ngircd.barton.de/
@@ -13,6 +15,7 @@ MAINTAINER = Giannis Tsaraias http://ngircd.barton.de/pub/ngircd/ \
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 - 1.4
+++ patches/patch-src_ngircd_ngircd_c 21 Feb 2020 12:36:53 -
@@ -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.cTue 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 -
+++ patches/patch-src_ngircd_proc_c 21 Feb 2020 12:36:53 -
@@ -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
Re: pledge net/ngircd
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 - 1.18
> +++ Makefile 11 Feb 2020 10:24:57 -
> @@ -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 - 1.4
> +++ patches/patch-src_ngircd_ngircd_c 11 Feb 2020 10:24:57 -
> @@ -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 -
> +++ patches/patch-src_ngircd_proc_c 11 Feb 2020 10:24:57 -
> @@ -0,0 +1,15 @@
> +$OpenBSD$
> +
> +Index: src/ngircd/proc.c
> +--- src/ngircd/proc.c.orig
>
Re: pledge net/ngircd
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
--- Makefile12 Jul 2019 20:48:34 - 1.18
+++ Makefile11 Feb 2020 10:24:57 -
@@ -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 - 1.4
+++ patches/patch-src_ngircd_ngircd_c 11 Feb 2020 10:24:57 -
@@ -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.cTue 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 -
+++ patches/patch-src_ngircd_proc_c 11 Feb 2020 10:24:57 -
@@ -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",
Re: pledge net/ngircd
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.
Re: pledge net/ngircd
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?
pledge net/ngircd
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.
Index: Makefile
===
RCS file: /cvs/ports/net/ngircd/Makefile,v
retrieving revision 1.18
diff -u -p -u -p -r1.18 Makefile
--- Makefile12 Jul 2019 20:48:34 - 1.18
+++ Makefile7 Feb 2020 14:15:32 -
@@ -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 - 1.4
+++ patches/patch-src_ngircd_ngircd_c 7 Feb 2020 14:15:32 -
@@ -1,7 +1,18 @@
$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.cTue 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,9 @@ main(int argc, const char *argv[])
+ exit(1);
+ }
+
++ 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 +566,7 @@ Setup_FDStreams(int fd)
#if !defined(SINGLE_USER_OS)
/**
@@ -10,7 +21,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 +590,7 @@ NGIRCd_getNobodyID(uid_t *uid, gid_t *gid )
}
#endif
@@ -19,7 +30,7 @@ $OpenBSD: patch-src_ngircd_ngircd_c,v 1.
if (!pwd)
return false;
-@@ -703,11 +703,11 @@ NGIRCd_Init(bool NGIRCd_NoDaemon)
+@@ -703,11 +706,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 -
+++ patches/patch-src_ngircd_proc_c 7 Feb 2020 14:15:32 -
@@ -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
