[Added tech-userlevel]

On Fri, Jan 29, 1999 at 04:38:04PM -0500, Alexis Rosen wrote:
> While trying to move a tftp server to NetBSD from SunOS, I ran into a problem:
> My ciscos could download files but not upload.
> 
> Reading the source made it clear: NetBSD's tftpd demands (in validate_access())
> that "full path name must be given as we have no login directory", according
> to a comment (line 459 of tftpd.c).
> 
> This turns out to be PR 4637. The fix recommended there is to search in all
> of the paths listed in the invocation. I disagree- it should simply try to
> use / or the / of the chroot as a default. The other alternative violates
> the Principle of Least Surprise.

I've discussed this with Alexis a bit in private email (since I gather he
wants me to commit the change), and I said I disagreed with him about his
change vs. the first reccommended fix vis-a-vis "the Principle of Least
Surprise."  Thing is, I think that the file being written to could quite
often be different than the file being read, under his scheme.  (e.g., if
we ran tftpd as
/usr/libexec/tftpd /tftpboot
and wrote a file without an absolute path, it'd go into /, but any
relative file we read will come from /tftpboot.  (I'm pretty green to
tftpd, so I could be completely bonkers here, but I think I follow.)

Now, the original proposed fix wants to walk the relative-dirs passed on
the command-line, just like tftpd does for reading files..  The way the
relative-reading code works is it just iterates over the directories
passed on the command line, stat()ing for the path of the file based in
each directory, finishing only when it finds a world-readable version of
the filename.  Thing is, we'd have to check for world-writable if we were
doing relative-writes, and this once again means that we could conceivably
end up talking to two different files (one readable but not writable, and
one writable but not readable).

Now, there's only one clean solution that I see to this problem, but that
changes existing behaviour that was presumably put in there for a reason..
That solution is to stop checking files for access as soon as we find one
that might potentially be used, a la the attached patch.  But,
unfortunately, that'll mean that if we've got two files that might be
read, one with S_IROTH set and one without, if we run into the one without
S_IROTH set we won't fall back to the one we could read.  I've got no clue
as to specifics of how people use tftp, so I don't know how big of a
problem that is.

Anyways, the proposed, completely untested patch:
--- tftpd.c.orig      Wed Feb  3 01:20:52 1999
+++ tftpd.c     Wed Feb  3 01:29:46 1999
@@ -512,7 +512,7 @@
 		 * restrictions.
 		 */
 
-		if (mode != RRQ || !strncmp(filename, "../", 3))
+		if (!strncmp(filename, "../", 3))
 			return (EACCESS);
 
 		/*
@@ -527,13 +527,15 @@
 				    dirp->name, filename);
 				if (stat(pathname, &stbuf) == 0 &&
 				    (stbuf.st_mode & S_IFMT) == S_IFREG) {
-					if ((stbuf.st_mode & S_IROTH) != 0)
-						break;
-					err = EACCESS;
+					break;
 				}
 			}
 			if (dirp->name == NULL)
 				return (err);
+			if (mode == RRQ && !(stbuf.st_mode & S_IROTH))
+				return (EACCESS);
+			else if (!stbuf.st_mode & S_IWOTH)
+				return (EACCESS);
 			*filep = filename = pathname;
 		} else
 			*filep = filename;

Does this seem reasonable?  (after testing, of course..)
--aidan