Subject: proposed change to ruserok()
To: None <tech-userlevel@netbsd.org>
From: Darren Reed <darrenr@reed.wattle.id.au>
List: tech-userlevel
Date: 12/13/2001 00:04:54
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??)
* checks to see if the file changes between lstat & fopen with more
  vigour
* set the "global" rhost error message if .rhost's cannot be opened
* don't change the uid/gid until after the fstat() is done in case NFS
  doesn't/hasn't cache(d) the results
* 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().

Darren

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;