inn.conf: Support for `newsuser', `newsgrp' options (patch is, hopefully, included)

Ivan Shmakov oneingray at gmail.com
Fri Dec 14 08:48:53 UTC 2007


>>>>> Russ Allbery <rra at stanford.edu> writes:

 >> Is there a problem when the first paragraph of the message is
 >> indented with TABs?

 > Ecartis does all sorts of fascinating things to messages for no
 > particularly good reasons.

	Oh, now I see, it strips MIME attachments, too!

 > Your plan sounds good to me.  I wish I had time to write code.  :)

	I've actually have written some code already.  Could you take a
	look at it?

	There's an issue with get_news_uid_gid () -- it assumes innconf
	!= NULL (i. e., `inn.conf' was read.)  However, when `inndstart'
	is started ``setuid root'', it's way more secure to read
	`inn.conf' /after/ dropping privileges (as it's done now.)  So,
	get_news_uid_gid () should contain a fallback, like:

   const char *newsuser = innconf != 0 ? innconf->newsuser : NEWSUSER;
   const char *newsgrp  = innconf != 0 ? innconf->newsgrp  : NEWSGRP;

	to be used in `inndstart'.

	There may be other issues with the code, so I'd ask not to apply
	these patches right now.  However, I'll appreciate any comments
	on them.

	The change to use get_news_uid_gid () instead of NEWSUSER and
	NEWSGRP is probably to be done across the whole source at once.

	BTW, since setuid_news () was a way to save one from ``messing
	up the ownership'', it was better to refer to EUID, and not UID.
	Though, it's a non-issue, unless one of the binaries gets
	somehow installed setuid.

	The first patch (hope Ecartis doesn't strip diffs):

diff -drHu inn2-2.4.3+20070806-debian-1/include/inn/innconf.h inn2-2.4.3+20070806-debian-1-newsuser/include/inn/innconf.h
--- inn2-2.4.3+20070806-debian-1/include/inn/innconf.h	2007-08-06 16:07:29.000000000 +0700
+++ inn2-2.4.3+20070806-debian-1-newsuser/include/inn/innconf.h	2007-12-11 22:23:57.000000000 +0600
@@ -26,6 +26,8 @@
     char *mta;                  /* MTA for mailing to moderators, innmail */
     char *pathhost;             /* Entry for the Path line */
     char *server;               /* Default server to connect to */
+    char *newsuser;		/* User to run under */
+    char *newsgrp;		/* Group to run under */
 
     /* Feed Configuration */
     long artcutoff;             /* Max accepted article age */
diff -drHu inn2-2.4.3+20070806-debian-1/lib/innconf.c inn2-2.4.3+20070806-debian-1-newsuser/lib/innconf.c
--- inn2-2.4.3+20070806-debian-1/lib/innconf.c	2007-08-06 16:07:29.000000000 +0700
+++ inn2-2.4.3+20070806-debian-1-newsuser/lib/innconf.c	2007-12-11 22:33:17.000000000 +0600
@@ -121,6 +121,9 @@
     { K(sourceaddress6),        STRING  (NULL) },
     { K(timer),                 NUMBER  (0) },
 
+    { K(newsuser),              STRING  (NEWSUSER) },
+    { K(newsgrp),               STRING  (NEWSGRP) },
+
     { K(patharchive),           STRING  (NULL) },
     { K(patharticles),          STRING  (NULL) },
     { K(pathbin),               STRING  (NULL) },
diff -drHu inn2-2.4.3+20070806-debian-1/lib/Makefile inn2-2.4.3+20070806-debian-1-newsuser/lib/Makefile
--- inn2-2.4.3+20070806-debian-1/lib/Makefile	2007-08-06 16:07:29.000000000 +0700
+++ inn2-2.4.3+20070806-debian-1-newsuser/lib/Makefile	2007-12-11 23:15:46.000000000 +0600
@@ -10,7 +10,8 @@
 		conffile.c confparse.c daemonize.c date.c dbz.c defdist.c \
 		fdflags.c fdlimit.c genid.c getfqdn.c getmodaddr.c gettime.c \
 		hash.c hashtab.c innconf.c inndcomm.c list.c localopen.c \
-		lockfile.c makedir.c md5.c messages.c mmap.c parsedate.c \
+		lockfile.c makedir.c md5.c messages.c mmap.c newsuser.c \
+		parsedate.c \
 		qio.c radix32.c readin.c remopen.c reservedfd.c resource.c \
 		sendarticle.c sendpass.c sequence.c sockaddr.c timer.c tst.c \
 		uwildmat.c vector.c version.c wire.c xfopena.c xmalloc.c \
diff -drHuN inn2-2.4.3+20070806-debian-1/include/newsuser.h inn2-2.4.3+20070806-debian-1-newsuser/include/newsuser.h
--- inn2-2.4.3+20070806-debian-1/include/newsuser.h	1970-01-01 07:00:00.000000000 +0700
+++ inn2-2.4.3+20070806-debian-1-newsuser/include/newsuser.h	2007-12-11 22:04:11.000000000 +0600
@@ -0,0 +1,21 @@
+/*** newsuser.h --- Ensure running as `news' user / group: header  -*- C -*- */
+
+/*** Ivan Shmakov, 2007 */
+/** This code is in the public domain */
+
+/*** Code: */
+#ifndef NEWSUSER_H
+#define NEWSUSER_H 1
+
+#include "config.h"
+#include "clibrary.h"
+
+int get_news_uid_gid (uid_t *uid, gid_t *gid, int may_die_p);
+
+/** the following functions die () on failure */
+void ensure_news_user (int may_setuid_p);
+void ensure_news_grp  (int may_setgid_p);
+void ensure_news_user_grp (int may_setuid_p, int may_setgid_p);
+
+#endif
+/*** newsuser.h ends here */
diff -drHuN inn2-2.4.3+20070806-debian-1/lib/newsuser.c inn2-2.4.3+20070806-debian-1-newsuser/lib/newsuser.c
--- inn2-2.4.3+20070806-debian-1/lib/newsuser.c	1970-01-01 07:00:00.000000000 +0700
+++ inn2-2.4.3+20070806-debian-1-newsuser/lib/newsuser.c	2007-12-12 00:23:37.000000000 +0600
@@ -0,0 +1,99 @@
+/*** newsuser.c --- Ensure running as `news' user / group  -*- C -*- */
+
+/*** Ivan Shmakov, 2007 */
+/** This code is in the public domain */
+
+/*** Code: */
+#include "config.h"
+#include "clibrary.h"
+
+#include <pwd.h>
+#include <grp.h>
+
+#include "inn/innconf.h"
+#include "newsuser.h"
+
+/** Resolve newsuser to a UID and newsgrp to a GID */
+int
+get_news_uid_gid (uid_t *uid, gid_t *gid, int may_die_p)
+{
+  int fail_p = 0;
+  struct passwd *pwd;
+  struct group *grp;
+
+  /* resolve newsuser to a UID */
+  if (uid == 0) {
+    /* do nothing */
+  } else if ((pwd = getpwnam (innconf->newsuser)) != 0) {
+    *uid = pwd->pw_uid;
+  } else if (may_die_p) {
+    die (("can't resolve %s to a UID"
+          " (account doesn't exist?)"), innconf->newsuser);
+  } else {
+    fail_p = 1;
+  }
+
+  /* resolve newsgrp to a GID */
+  if (gid == 0) {
+    /* do nothing */
+  } else if ((grp = getgrnam (innconf->newsgrp)) != 0) {
+    *gid = grp->gr_gid;
+  } else if (may_die_p) {
+    die (("can't resolve %s to a GID"
+          " (group doesn't exist?)"), innconf->newsgrp);
+  } else {
+    fail_p = 1;
+  }
+
+  /* . */
+  return fail_p ? -1 : 0;
+}
+
+/** Ensure running as newsuser */
+void
+ensure_news_user (int may_setuid_p)
+{
+  uid_t uid;
+
+  get_news_uid_gid (&uid, 0, 1);
+  if (geteuid () == 0) {
+    if (! may_setuid_p) {
+      /* NB: mustn't be run as root, unless `may_setuid_p' is true */
+      die ("must be run as %s, not as root", innconf->newsuser);
+    }
+    setuid (uid);
+  }
+  if (geteuid () != uid || getuid () != uid) {
+    die ("must be run as %s", innconf->newsuser);
+  }
+
+  /* . */
+}
+
+/** Ensure running as newsgrp group */
+void
+ensure_news_grp (int may_setgid_p)
+{
+  gid_t gid;
+
+  get_news_uid_gid (0, &gid, 1);
+  if (may_setgid_p && geteuid () == 0) {
+    setgid (gid);
+  }
+  if (getegid () != gid || getgid () != gid) {
+    die ("must be run as %s group", innconf->newsgrp);
+  }
+
+  /* . */
+}
+
+/** Ensure running as newsuser and newsgrp group */
+void
+ensure_news_user_grp (int may_setuid_p, int may_setgid_p)
+{
+  /* NB: changing the group first to lose root privileges last */
+  ensure_news_grp  (may_setgid_p);
+  ensure_news_user (may_setuid_p);
+}
+
+/*** newsuser.c ends here */

	The second one:

diff -drHu inn2-2.4.3+20070806-debian-1/expire/expire.c inn2-2.4.3+20070806-debian-1-newsuser/expire/expire.c
--- inn2-2.4.3+20070806-debian-1/expire/expire.c	2007-08-06 16:07:29.000000000 +0700
+++ inn2-2.4.3+20070806-debian-1-newsuser/expire/expire.c	2007-12-12 00:03:34.000000000 +0600
@@ -7,7 +7,6 @@
 #include "clibrary.h"
 #include <ctype.h>
 #include <errno.h>
-#include <pwd.h>
 #include <sys/stat.h>
 #include <syslog.h>
 #include <time.h>
@@ -17,10 +16,10 @@
 #include "inn/messages.h"
 #include "inndcomm.h"
 #include "libinn.h"
+#include "newsuser.h"
 #include "paths.h"
 #include "storage.h"
 
-
 typedef struct _EXPIRECLASS {
     time_t              Keep;
     time_t              Default;
@@ -488,25 +487,6 @@
 }
 
 
-/*
-**  Change to the news user if possible, and if not, die.  Used for operations
-**  that may create new database files so as not to mess up the ownership.
-*/
-static void
-setuid_news(void)
-{
-    struct passwd *pwd;
-
-    pwd = getpwnam(NEWSUSER);
-    if (pwd == NULL)
-        die("can't resolve %s to a UID (account doesn't exist?)", NEWSUSER);
-    if (getuid() == 0)
-        setuid(pwd->pw_uid);
-    if (getuid() != pwd->pw_uid)
-        die("must be run as %s", NEWSUSER);
-}
-
-
 int
 main(int ac, char *av[])
 {
@@ -627,8 +607,8 @@
     RealNow = Now;
     Now += TimeWarp;
 
-    /* Change users if necessary. */
-    setuid_news();
+    /* Change to the newsuser and newsgrp group if necessary. */
+    ensure_news_user_grp (1, 1);
 
     /* Parse the control file. */
     if (av[0]) {
diff -drHu inn2-2.4.3+20070806-debian-1/expire/expireover.c inn2-2.4.3+20070806-debian-1-newsuser/expire/expireover.c
--- inn2-2.4.3+20070806-debian-1/expire/expireover.c	2007-08-06 16:07:29.000000000 +0700
+++ inn2-2.4.3+20070806-debian-1-newsuser/expire/expireover.c	2007-12-12 00:03:34.000000000 +0600
@@ -11,7 +11,6 @@
 #include "config.h"
 #include "clibrary.h"
 #include <errno.h>
-#include <pwd.h>
 #include <signal.h>
 #include <syslog.h>
 #include <time.h>
@@ -46,25 +45,6 @@
 }
 
 
-/*
-**  Change to the news user if possible, and if not, die.  Used for operations
-**  that may create new database files so as not to mess up the ownership.
-*/
-static void
-setuid_news(void)
-{
-    struct passwd *pwd;
-
-    pwd = getpwnam(NEWSUSER);
-    if (pwd == NULL)
-        die("can't resolve %s to a UID (account doesn't exist?)", NEWSUSER);
-    if (getuid() == 0)
-        setuid(pwd->pw_uid);
-    if (getuid() != pwd->pw_uid)
-        die("must be run as %s", NEWSUSER);
-}
-
-
 int
 main(int argc, char *argv[])
 {
@@ -142,8 +122,8 @@
     if (!innconf_read(NULL))
         exit(1);
 
-    /* Change to the news user if necessary. */
-    setuid_news();
+    /* Change to the newsuser and newsgrp group if necessary. */
+    ensure_news_user_grp (1, 1);
 
     /* Initialize the lowmark file, if one was requested. */
     if (lowmark_path != NULL) {
diff -drHu inn2-2.4.3+20070806-debian-1/expire/makedbz.c inn2-2.4.3+20070806-debian-1-newsuser/expire/makedbz.c
--- inn2-2.4.3+20070806-debian-1/expire/makedbz.c	2007-08-06 16:07:29.000000000 +0700
+++ inn2-2.4.3+20070806-debian-1-newsuser/expire/makedbz.c	2007-12-12 00:03:34.000000000 +0600
@@ -6,7 +6,6 @@
 #include "config.h"
 #include "clibrary.h"
 #include <errno.h>
-#include <pwd.h>
 #include <syslog.h>  
 
 #include "dbz.h"
@@ -14,6 +13,7 @@
 #include "inn/messages.h"
 #include "inn/qio.h"
 #include "libinn.h"
+#include "newsuser.h"
 #include "paths.h"
 #include "storage.h"
 
@@ -230,25 +230,6 @@
 }
 
 
-/*
-**  Change to the news user if possible, and if not, die.  Used for operations
-**  that may create new database files, so as not to mess up the ownership.
-*/
-static void
-setuid_news(void)
-{
-    struct passwd *pwd;
-
-    pwd = getpwnam(NEWSUSER);
-    if (pwd == NULL)
-        die("can't resolve %s to a UID (account doesn't exist?)", NEWSUSER);
-    if (getuid() == 0)
-        setuid(pwd->pw_uid);
-    if (getuid() != pwd->pw_uid)
-        die("must be run as %s", NEWSUSER);
-}
-
-
 int
 main(int argc, char **argv)
 {
@@ -309,8 +290,8 @@
     if (chdir(HistoryDir) < 0)
         sysdie("cannot chdir to %s", HistoryDir);
 
-    /* Change users if necessary. */
-    setuid_news();
+    /* Change to the newsuser and newsgrp group if necessary. */
+    ensure_news_user_grp (1, 1);
 
     Rebuild(size, IgnoreOld, Overwrite);
     closelog();
diff -drHu inn2-2.4.3+20070806-debian-1/expire/makehistory.c inn2-2.4.3+20070806-debian-1-newsuser/expire/makehistory.c
--- inn2-2.4.3+20070806-debian-1/expire/makehistory.c	2007-08-06 16:07:29.000000000 +0700
+++ inn2-2.4.3+20070806-debian-1-newsuser/expire/makehistory.c	2007-12-12 00:03:34.000000000 +0600
@@ -8,7 +8,6 @@
 #include "portable/wait.h"
 #include <assert.h>
 #include <errno.h>
-#include <pwd.h>
 #include <syslog.h>  
 
 #include "inn/buffer.h"
@@ -18,6 +17,7 @@
 #include "inn/qio.h"
 #include "inn/wire.h"
 #include "libinn.h"
+#include "newsuser.h"
 #include "ov.h"
 #include "paths.h"
 #include "storage.h"
@@ -740,25 +740,6 @@
 }
 
 
-/*
-**  Change to the news user if possible, and if not, die.  Used for operations
-**  that may create new database files, so as not to mess up the ownership.
-*/
-static void
-setuid_news(void)
-{
-    struct passwd *pwd;
-
-    pwd = getpwnam(NEWSUSER);
-    if (pwd == NULL)
-        die("can't resolve %s to a UID (account doesn't exist?)", NEWSUSER);
-    if (getuid() == 0)
-        setuid(pwd->pw_uid);
-    if (getuid() != pwd->pw_uid)
-        die("must be run as %s", NEWSUSER);
-}
-
-
 int
 main(int argc, char **argv)
 {
@@ -853,8 +834,8 @@
     if (chdir(HistoryDir) < 0)
         sysdie("cannot chdir to %s", HistoryDir);
 
-    /* Change users if necessary. */
-    setuid_news();
+    /* Change to the newsuser and newsgrp group if necessary. */
+    ensure_news_user_grp (1, 1);
 
     /* Read in the overview schema */
     ARTreadschema(DoOverview);



More information about the inn-workers mailing list