buglwIP - A Lightweight TCP/IP stack - Bugs: bug #50534, TFTP server does not copy...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #50534: TFTP server does not copy terminating null of filename

Submitter:  David Rodgers <drodgers_ls>
Submitted:  Mon 13 Mar 2017 08:04:49 PM UTC
   
 
Category:  apps Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  2.0.2

Mon 13 Mar 2017 08:42:46 PM UTC, comment #2: 


> This is my first bug report for this project, so let me know
> if there's anything else I need to do


In general, it's better to provide a diff/patch against a defined version. This helps us to see what's changed without manually comparing (where we need the version you have for manual diff).

On top of that, a git diff save us the time for a commit (and keeps your name in the git log), but for such simple patches, a simple diff is probably enough :-)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 13 Mar 2017 08:27:16 PM UTC, comment #1: 

Thank you for reporting and providing a patch!! Pushed.

Dirk Ziegelmeier <dziegel>
Group administrator
Mon 13 Mar 2017 08:04:49 PM UTC, original submission:  

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.

David Rodgers <drodgers_ls>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #40000:  tftp_server.c added by drodgers_ls (11KiB - text/plain - Fixed tftp_server.c (copies filename null))

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by drodgers_ls (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-03-13 dziegel StatusNone Fixed
        Open/ClosedOpen Closed
    2017-03-13 drodgers_ls Attached File- Added tftp_server.c, #40000

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code