Subject: Re: proposed change to ruserok()
To: None <tech-userlevel@netbsd.org>
From: Christos Zoulas <christos@zoulas.com>
List: tech-userlevel
Date: 12/12/2001 16:37:13
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;