Subject: Re: Setreuid in perl-4.036
To: None <kenh@wrl.EPI.COM>
From: Mark P. Gooderum <mark@aggregate.com>
List: current-users
Date: 07/21/1994 13:00:14
> Last time I looked, there was some support for this; If I remember correctly,
> you had to use "option SUIDSCRIPTS" or something similar to get it to work.
> I forget what kernel module it was in, or whether or not it's ready for
> prime time.

Hmmm.  I looked into this and found an oddity and a minor bug.
The kernel module in question is ...src/sys/kerne/exec_script.c.

There are two defines, FDSCRIPTS and SUIDSCRIPTS.  FDSCRIPTS effectively
says to try to use /dev/fd to exec a script that isn't readable or is
setuid, the check is about line 162:

#ifdef FDSCRIPTS
	/*
	 * if the script isn't readable, or it's set-id, then we've
	 * gotta supply a "/dev/fd/..." for the shell to read.
	 * Note that stupid shells (csh) do the wrong thing, and
	 * close all open fd's when the start.  That kills this
	 * method of implementing "safe" set-id and x-only scripts.
	 */
	if (VOP_ACCESS(epp->ep_vp, VREAD, p->p_ucred, p) == EACCES ||
	    script_sbits) {
		struct file *fp;
		extern struct fileops vnops;

At the top of the module, if SUIDSCRIPTS is set and FDSCRIPTS is not,
set FDSCRIPTS.  The reverse is not true.

Near the top of exec_script(), some variables are defined if SETUIDSCRIPTS
is set:

#ifdef SETUIDSCRIPTS
	uid_t script_uid;
	gid_t script_gid;
	u_short script_sbits;
#endif

Then if SETUIDSCRIPTS is set, script_bits is set (along with script_uid and
script_gid):

#ifdef SETUIDSCRIPTS
	/*
	 * MNT_NOSUID and STRC are already taken care of by check_exec,
	 * so we don't need to worry about them now or later.
	 */
	script_sbits = epp->ep_vap->va_mode & (VSUID | VSGID);
	if (script_sbits != 0) {
		script_uid = epp->ep_vap->va_uid;
		script_gid = epp->ep_vap->va_gid;
	}
#endif

But if FDSCRIPTS is defined and SETUIDSCRIPTS isn't, script_bits isn't
defined and you get a compile error in the FDSCRIPTS if given first.
Finally, script_uid and script_gid are only used to assign back into the
same fields farther down:

#ifdef SETUIDSCRIPTS
		/*
		 * set thing up so that set-id scripts will be
		 * handled appropriately
		 */
		epp->ep_vap->va_mode |= script_sbits;
		if (script_sbits & VSUID)
			epp->ep_vap->va_uid = script_uid;
		if (script_sbits & VSGID)
			epp->ep_vap->va_gid = script_gid;
#endif

These fields aren't modified in between, so the variables xxxid aren't needed
and the resetting of va_mode isn't required.  One possible patch follows.
Note that none of these problems are run time bugs, but ones a compile bug
and the other is some confusing and effectively dead code.
The netbsd csh source does appear to have the problem mentioned in the
comments about closing everything (before the scripts are opened, 
initdesc() is called which calls closem() which closes everything except the
std stuff.  I'm not sure how hard a fix would be...special casing it to
the /dev/fd case would probably be easy.  I haven't looked at tcsh yet.

My editor munged some tabs, so this is a diff -w, it may take a patch -l
to get it to apply.

-Mark
--------------------------------------------------------

--- exec_script.c	Wed Jun 29 05:28:54 1994
+++ exec_script.c.patched	Thu Jul 21 12:48:44 1994
@@ -72,8 +72,6 @@
 	struct vnode *scriptvp;
 #ifdef SETUIDSCRIPTS
 	uid_t script_uid;
-	gid_t script_gid;
-	u_short script_sbits;
 #endif
 
 	/*
@@ -146,10 +144,6 @@
 	 * so we don't need to worry about them now or later.
 	 */
 	script_sbits = epp->ep_vap->va_mode & (VSUID | VSGID);
-	if (script_sbits != 0) {
-		script_uid = epp->ep_vap->va_uid;
-		script_gid = epp->ep_vap->va_gid;
-	}
 #endif
 #ifdef FDSCRIPTS
 	/*
@@ -159,8 +153,12 @@
 	 * close all open fd's when the start.  That kills this
 	 * method of implementing "safe" set-id and x-only scripts.
 	 */
-	if (VOP_ACCESS(epp->ep_vp, VREAD, p->p_ucred, p) == EACCES ||
-	    script_sbits) {
+    if (VOP_ACCESS(epp->ep_vp, VREAD, p->p_ucred, p) == EACCES 
+#ifdef SETUIDSCRIPTS
+||
+        script_sbits
+#endif
+) {
 		struct file *fp;
 		extern struct fileops vnops;
 
@@ -243,17 +241,6 @@
 
 		epp->ep_flags |= (EXEC_HASARGL | EXEC_SKIPARG);
 		epp->ep_fa = shellargp;
-#ifdef SETUIDSCRIPTS
-		/*
-		 * set thing up so that set-id scripts will be
-		 * handled appropriately
-		 */
-		epp->ep_vap->va_mode |= script_sbits;
-		if (script_sbits & VSUID)
-			epp->ep_vap->va_uid = script_uid;
-		if (script_sbits & VSGID)
-			epp->ep_vap->va_gid = script_gid;
-#endif
 		return (0);
 	}
 




------------------------------------------------------------------------------