Subject: Re: check resource limits with exec(3)?
To: None <tech-userlevel@netbsd.org, tech-kern@netbsd.org,>
From: Jeremy C. Reed <reed@reedmedia.net>
List: tech-kern
Date: 06/13/2006 22:56:31
I added tech-kern to this as I have a patch for kernel below related to 
this. (Also included are parts of a few emails below for some context.)
Below are two patches (one for kernel) and replies to a couple emails.
I also added tech-security for discussion on the actual security 
issues this involves.

On Tue, 6 Jun 2006, Iain Hibbert wrote to tech-userlevel:

> > Or is it acceptable for programs to go over (ignore) the defined maxproc?
> 
> If it is actually a problem, then I would say that the correct place to
> check it would be under setuid() somewhere (probably do_setresuid() in
> sys/kern/kern_prot.c?), and return EPROCLIM if the operation would exceed
> limits.  I couldnt say if that would have other implications though..

On Thu, 8 Jun 2006, Daniel Carosone wrote to tech-userlevel:

> On Wed, Jun 07, 2006 at 09:31:12PM +0100, David Laight wrote:
> > All the places where those calls are assumed to succeed because there
> > is no way they can fail...
> 
> This discussion comes about in part because of a discovery, on another
> platform, of a way they can fail.

Exactly. I was testing some security issues found on Linux caused by 
setuid failing (where a user could easily get root). I couldn't reproduce 
same issue on NetBSD but came across a different issue.

Here is what I have been using:

Index: sys/kern/kern_prot.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_prot.c,v
retrieving revision 1.89
diff -u -r1.89 kern_prot.c
--- sys/kern/kern_prot.c	14 May 2006 21:15:11 -0000	1.89
+++ sys/kern/kern_prot.c	13 Jun 2006 20:04:42 -0000
@@ -353,8 +353,12 @@
 	/* The pcred structure is not actually shared... */
 	if (r != -1 && r != kauth_cred_getuid(cred)) {
 		/* Update count of processes for this user */
+		/* but don't allow user running more than their limit. */
+		if (chgproccnt(r, 1) > p->p_rlimit[RLIMIT_NPROC].rlim_cur) {
+			(void)chgproccnt(r, -1);
+			return (EAGAIN);
+		}
 		(void)chgproccnt(kauth_cred_getuid(cred), -1);
-		(void)chgproccnt(r, 1);
 		kauth_cred_setuid(cred, r);
 	}
 	if (sv != -1)


I return EAGAIN instead of suggested EPROCLIM since I saw its use in other 
similar code.

I hope it is okay to change the order of adding and remove the process 
count above.


On Wed, 7 Jun 2006, Greg A. Woods wrote:

> > (NetBSD includes a couple in the default install that allow normal users
> > to start more processes than they are allowed.)
> 
> Concrete examples?

I emailed off-list but didn't have an MX for my hostname so your mail 
server bounced me.

Few examples: login, su and cron. For example, a user can login and run 
their "shell" even if they have passed their maxproc. (Of course their new 
shell can't fork() anything, but they could use a custom shell.)

With my patch above, login fails (like I want) with:

/var/log/authlog:Jun 13 12:16:13 glacier login: setuid(1004): Resource 
temporarily unavailable
/var/log/authlog:Jun 13 12:16:13 glacier login: setusercontext failed

Very easy to abuse cron in particular by a regular user to override their 
login.conf defined limits. I have done this many times on NetBSD 2.1 and 
-current.

cron did only set resources for root which makes no sense. My patch below 
fixes problem with going over maxproc -- in combination with kernel patch 
above. For example (with patches):

Jun 13 22:28:00 glacier cron[4058]: setuid(1004): Resource temporarily unavailable
Jun 13 22:28:00 glacier cron[4058]: setusercontext failed

Index: usr.sbin/cron/do_command.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/cron/do_command.c,v
retrieving revision 1.22
diff -u -r1.22 do_command.c
--- usr.sbin/cron/do_command.c	5 Jun 2006 16:41:34 -0000	1.22
+++ usr.sbin/cron/do_command.c	13 Jun 2006 22:49:11 -0000
@@ -90,6 +90,10 @@
 	(void) &mailto;
 	(void) &children;
 #endif
+#ifdef LOGIN_CAP
+	login_cap_t	*lc;
+	struct passwd	*pwd;
+#endif
 	Debug(DPROC, ("[%d] child_process('%s')\n", getpid(), e->cmd))
 
 	/* note we handle a job */
@@ -197,9 +201,6 @@
 		if (setsid() == -1)
 			syslog(LOG_ERR, "setsid() failure: %m");
 
-		if (setlogin(usernm) < 0)
-			syslog(LOG_ERR, "setlogin() failure: %m");
-
 		/* close the pipe ends that we won't use.  this doesn't affect
 		 * the parent, who has to read and write them; it keeps the
 		 * kernel from recording us as a potential client TWICE --
@@ -228,14 +229,26 @@
 		do_univ(u);
 
 #ifdef LOGIN_CAP
-		if (setusercontext(NULL, getpwuid(e->uid), e->uid,
-		    LOGIN_SETRESOURCES|LOGIN_SETPRIORITY|
-		    LOGIN_SETUMASK) != 0) {
+		if ((pwd = getpwuid(e->uid)) == NULL) {
+		   syslog(LOG_ERR, "getpwuid failed");
+		   _exit(ERROR_EXIT);
+		}
+
+		if ((lc = login_getclass(pwd ? pwd->pw_class : NULL)) == NULL) {
+		   syslog(LOG_ERR, "unable to get login class for %s\n",
+			u->name);
+		   _exit(ERROR_EXIT);
+		}
+		/* Does the setgid, initgroups, setlogin and setuid */
+		if (setusercontext(lc, pwd, e->uid,
+		    LOGIN_SETALL) != 0) {
 			syslog(LOG_ERR, "setusercontext failed");
 			_exit(ERROR_EXIT);
 		}
-#endif /* LOGIN_CAP */
-		/* set our directory, uid and gid.  Set gid first, since once
+
+		login_close(lc);
+#else
+		/* Set gid first, since once
 		 * we set uid, we've lost root privledges.
 		 */
 		if (setgid(e->gid) != 0) {
@@ -248,10 +261,14 @@
 		   _exit(ERROR_EXIT);
 		}
 # endif
+		if (setlogin(usernm) < 0)
+			syslog(LOG_ERR, "setlogin() failure: %m");
+
 		if (setuid(e->uid) != 0) {
 		   syslog(LOG_ERR, "setuid failed");
 		   _exit(ERROR_EXIT);
 		}
+#endif /* LOGIN_CAP */
 		/* we aren't root after this... */
 		chdir(env_get("HOME", e->envp));
 
Index: usr.sbin/cron/popen.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/cron/popen.c,v
retrieving revision 1.10
diff -u -r1.10 popen.c
--- usr.sbin/cron/popen.c	31 Jul 2005 17:52:01 -0000	1.10
+++ usr.sbin/cron/popen.c	13 Jun 2006 22:49:11 -0000
@@ -53,6 +53,7 @@
 
 #define MAX_ARGS 1024
 
+/* should this honor LOGIN_CAP also ? */
 FILE *
 cron_popen(char *program, const char *type)
 {


I am not sure about other resources (other than maxproc) as I didn't full 
test them all yet.


 Jeremy C. Reed

p.s. On FreeBSD, I had different bad behaviour with cron -- it would not 
run the cron job but then had continuous repeated sendmail processes that 
hung.