[lwip-devel] [bug #50534] TFTP server does not copy terminating null of filename

[David Rodgers]

URL:
  <http://savannah.nongnu.org/bugs/?50534>

                 Summary: TFTP server does not copy terminating null of
filename
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: drodgers_ls
            Submitted on: Mon 13 Mar 2017 08:04:49 PM UTC
                Category: apps
                Severity: 3 - Normal
              Item Group: Faulty Behaviour
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None
            lwIP version: 2.0.2

    _______________________________________________________

Details:

I am currently developing a TCP and UDP application on NXP Kinetis using lwIP
2.0.0.  NXP does not provide any of the apps, so I copied
lwip/src/apps/tftp/tftp_server.c into my application.  Everything seemed to
work OK, except after some extended testing, I noticed I was sometimes getting
trailing garbage in my filenames:


(attempting to read non-existent "fwlogic.pkglksdjlk")
tftp: file "fwlogic.pkglksdjlkøÿl" not recognized
tftp: file "fwlogic.pkglksdjlkøÿ\" not recognized
tftp: file "fwlogic.pkglksdjlkøÿÌ" not recognized
(attempting to read non-existent "fwlogic.abcde")
tftp: file "fwlogic.abcde¨
8" not recognized


I ran Wireshark and verified that my TFTP client (Microsoft command line) was
not generating bad filenames.  I traced into my application where I was
generating the above messages.  I then went down the stack one level into
tftp_server.c, static void recv().

Long story short, the math for calculating the size of the filename data to
copy, which must include the terminating null, is off by one.  I'll include an
example, by a read request for file "wibble", mode "octet".


01234567890123456789
**wibble.octet.


A TFTP read request consists of a 2-byte opcode, followed by a null-terminated
filename, followed by a null-terminated mode.  The opcode always starts the
filename at offset 2.  So in this call:

filename_end_offset = pbuf_memfind(p, &tftp_null, sizeof(tftp_null), 2);

filename_end_offset will for the above example be set to 8 (opcode is 2 bytes,
"wibble" is six bytes long).  The code then uses the expression
(filename_end_offset-2) in two places: checking it does not exceed
sizeof(filename), and setting the size of data to copy to filename[].  For the
above example, the expression equals 6.  But that's not the value you want...
you need to copy the trailing null as well.

You actually want ((filename_end_offset-2)+1), or (filename_end_offset-1),
which in this example is 7.  You want to change the expression in both places;
you need to make sure that the string PLUS null does not exceed the filename[]
buffer, and you need to copy the string PLUS null to the buffer.

Now, I also checked the code for the mode string, and that is correct;
filename_end_offset+1 is indeed the offset of the start of the mode string,
and (mode_end_offset-filename_end_offset) evaluates correctly in describing
the length of the mode string plus null.  Also, the declarations

char filename[TFTP_MAX_FILENAME_LEN];
char mode[TFTP_MAX_MODE_LEN];

are not semantically correct, since those buffers need to contain a null; I
suggest adding "+1" to each [] expression.  Thus, if TFTP_MAX_FILENAME_LEN is
20, then you can indeed supply a 20-character filename, and
filename[TFTP_MAX_FILENAME_LEN+1] can hold 20 characters plus a null.

I've tagged this bug against 2.0.2, because doing a diff shows that
tftp_server.c has not changed from 2.0.0 to 2.0.2.  I've attached a fixed copy
of tftp_server.c that runs correctly on my target.  This is my first bug
report for this project, so let me know if there's anything else I need to do,
thanks.



    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Mon 13 Mar 2017 08:04:49 PM UTC  Name: tftp_server.c  Size: 11kB   By:
drodgers_ls
Fixed tftp_server.c (copies filename null)
<http://savannah.nongnu.org/bugs/download.php?file_id=40000>

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?50534>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/