In article <200112121304.AAA07644@avalon.reed.wattle.id.au>,
Darren Reed <darrenr@reed.wattle.id.au> wrote:
>
>After some discussion on icb about problems with using rlogin on home
>directories, mounted with NFS but without root privs., the changes
>below were generally agreed upon as being "a good thing".
>
>Changes they bring in:
>* home directory must be owned by user or root
>* home directory cannot be writable by other (group too??)

Well, it seems to me that the norm these days is to have your home
directory not group writable too [sshd enforces this]. So I'd say
we could make it more strict. If we decide not to, then the error
message should indicate this [i.e. the error message should say
other than owner and group].

>* checks to see if the file changes between lstat & fopen with more
>  vigour

ok.

>* set the "global" rhost error message if .rhost's cannot be opened

ok.

>* don't change the uid/gid until after the fstat() is done in case NFS
>  doesn't/hasn't cache(d) the results

ok.

>* check that .rhosts is a regular file before opening it
>
>Comments from the peanut gallery on this change are welcome.  If someone
>else would like to do a threat analysis on races, etc, please feel free.
>
>The reason for lstat() and preventing .rhosts from being a symbolic link
>isn't clear given that fstat() is used.  If someone else is willing to
>stake their life on stat() being safe and not introducing a security hole,
>it could be used instead of lstat().

No, it is fine the way it is now.

- I'd use snprintf() instead of strncpy() + strncat()
- I'd check the seteuid() and initgroups returns
- I'd use != -1 instead of < 0 for the return values of syscalls.

christos

>Index: rcmd.c
>===================================================================
>RCS file: /cvsroot/basesrc/lib/libc/net/rcmd.c,v
>retrieving revision 1.45
>diff -c -r1.45 rcmd.c
>*** rcmd.c	2001/11/04 13:57:30	1.45
>--- rcmd.c	2001/12/12 12:54:44
>***************
>*** 661,667 ****
>  {
>  	struct sockaddr *sa;
>  	register char *cp;
>! 	struct stat sbuf;
>  	struct passwd *pwd;
>  	FILE *hostf;
>  	uid_t uid;
>--- 661,667 ----
>  {
>  	struct sockaddr *sa;
>  	register char *cp;
>! 	struct stat sbuf, lbuf;
>  	struct passwd *pwd;
>  	FILE *hostf;
>  	uid_t uid;
>***************
>*** 691,698 ****
>  		first = 0;
>  		if ((pwd = getpwnam(luser)) == NULL)
>  			return (-1);
>- 		(void)strncpy(pbuf, pwd->pw_dir, sizeof(pbuf) - 1);
>- 		(void)strncat(pbuf, "/.rhosts", sizeof(pbuf) - strlen(pbuf) - 1);
>  
>  		/*
>  		 * Change effective uid while opening .rhosts.  If root and
>--- 691,696 ----
>***************
>*** 704,734 ****
>  		(void)setegid(pwd->pw_gid);
>  		initgroups(pwd->pw_name, pwd->pw_gid);
>  		(void)seteuid(pwd->pw_uid);
>- 		hostf = fopen(pbuf, "r");
>- 		(void)seteuid(uid);
>- 		(void)setegid(gid);
>  
>! 		if (hostf == NULL)
>! 			return (-1);
>! 		/*
>! 		 * If not a regular file, or is owned by someone other than
>! 		 * user or root or if writeable by anyone but the owner, quit.
>! 		 */
>  		cp = NULL;
>! 		if (lstat(pbuf, &sbuf) < 0)
>  			cp = ".rhosts lstat failed";
>! 		else if (!S_ISREG(sbuf.st_mode))
>! 			cp = ".rhosts not regular file";
>! 		else if (fstat(fileno(hostf), &sbuf) < 0)
>! 			cp = ".rhosts fstat failed";
>! 		else if (sbuf.st_uid && sbuf.st_uid != pwd->pw_uid)
>! 			cp = "bad .rhosts owner";
>! 		else if (sbuf.st_mode & (S_IWGRP|S_IWOTH))
>! 			cp = ".rhosts writeable by other than owner";
>  		/* If there were any problems, quit. */
>  		if (cp) {
>  			__rcmd_errstr = cp;
>! 			(void)fclose(hostf);
>  			return (-1);
>  		}
>  		goto again;
>--- 702,757 ----
>  		(void)setegid(pwd->pw_gid);
>  		initgroups(pwd->pw_name, pwd->pw_gid);
>  		(void)seteuid(pwd->pw_uid);
>  
>! 		(void)strncpy(pbuf, pwd->pw_dir, sizeof(pbuf) - 1);
>! 		(void)strncat(pbuf, "/.rhosts", sizeof(pbuf) - strlen(pbuf)-1);
>! 
>  		cp = NULL;
>! 		hostf = NULL;
>! 		if (stat(pwd->pw_dir, &sbuf) < 0)
>! 			cp = "stat on home directory failed";
>! 		else if (sbuf.st_mode & S_IWOTH)
>! 			cp = "home directory is writable by other than owner";
>! 		else if ((sbuf.st_uid != 0) && (sbuf.st_uid != pwd->pw_uid))
>! 			cp = "bad home directory owner";
>! 		else if (lstat(pbuf, &lbuf) < 0)
>  			cp = ".rhosts lstat failed";
>! 		else if (!S_ISREG(lbuf.st_mode))
>! 			cp = ".rhosts is not a regular file";
>! 
>! 		if (cp == NULL) {
>! 			hostf = fopen(pbuf, "r");
>! 
>! 			if (hostf == NULL)
>! 				cp = "failed to open .rhosts";
>! 			else if (fstat(fileno(hostf), &sbuf) < 0) {
>! 				cp = ".rhosts fstat failed";
>! 			}
>! 		}
>! 
>! 		(void)seteuid(uid);
>! 		(void)setegid(gid);
>! 
>! 		if (cp == NULL) {
>! 			/*
>! 			 * If not a regular file, or is owned by someone other
>! 			 * than user or root or if writeable by anyone but the
>! 			 * owner, quit.
>! 			 */
>! 			if ((sbuf.st_uid != 0) && (sbuf.st_uid != pwd->pw_uid))
>! 				cp = "bad .rhosts owner";
>! 			else if (sbuf.st_mode & (S_IWGRP|S_IWOTH))
>! 				cp = ".rhosts writeable by other than owner";
>! 			else if ((lbuf.st_dev != sbuf.st_dev) ||
>! 				 (lbuf.st_ino != sbuf.st_ino) ||
>! 				 (lbuf.st_rdev != sbuf.st_rdev))
>! 				cp = ".rhosts file changed";
>! 		}
>  		/* If there were any problems, quit. */
>  		if (cp) {
>  			__rcmd_errstr = cp;
>! 			if (hostf != NULL)
>! 				(void)fclose(hostf);
>  			return (-1);
>  		}
>  		goto again;