Subject: tftpd
To: None <tech-userlevel@netbsd.org>
From: Alex <xela@MIT.EDU>
List: tech-userlevel
Date: 02/25/2003 00:24:16
Some Intel PXE ROMs have a bug that results in the bootfile name
being sent to tftpd with an 0xff appended to the end of the
filename.  This results in the machine not booting because the
boot file couldn't be found, and a fair bit of wasted sysadmin 
time.

I'd like to add a workaround for this bug to tftpd.  The
workaround checks whether a filename that fails to stat ends
with an 0xff, and if so, lops the last character off the
filename, and tries again with the new name.  

The function where these tests happen, validate_access, is written
as two nested if statements, and each of the three cases has code
for performing a series of tests on the filename, including 
stat().  Two of these are identical blocks of code, the other
one is functionally identical.  

Rather than insert my workaround three times, I think it makes
more sense to abstract those tests out into their own function,
which I call validate_file, and put the workaround in it.

Does this sound ok to people?

---Alex

P.S.  If anyone cares, here's more detail, in pseudocode to preserve
some illusion of brevity:  

validate_access tests the filename passed by the client for three
cases with regard to the optional list of directories tftpd is 
permitted to serve files out of:

    A:   if (filename begins with "/" &&
              (/ is in the list of allowed directories || 
                 there is no such list)
            perform file validity tests
    B1:  if (filename does not begin with a "/" &&
             the optional directory list exists)
           loop through the list attempting to stat filename in each;
             on success, break out of the loop and perform the rest
             of the file validity tests
    B2:  if (filename does not begin with a "/" &&
             the optional directory list does not exist)
           perform file validity tests

The code for the validity tests in A is identical to that in B2;
that's the code I've broken out into validate_file.  In B1, all
the same tests are performed, but coded differently, and the
stat test is performed inside the for loop.

In my version, validate_file is called in place of the file
validity tests in A and B2, and in place of the stat() in B1's
for loop.  This means the behavior of my code is potentially
different in case B1:  in the current code, if a file stat()s but
fails one of the other tests, validate_access fails.  In my
version, if a file stats but fails another test, we're still in
the for loop, and go on to the next directory --- potentially
booting the client from a file that does pass all the tests, in
a directory later in the list.  This is a pretty unlikely
circumstance, but I believe what my code does is more correct.

While I'm at it, there's one other thing I'd like to change in
validate_access.  There are two security tests:
strstr(filename, "/../") occurs before A; 
!strncmp(filename, "../", 3) occurs at the beginning of B1.
I'd like to combine the two of them before A.

Please let me know if you have any comments.