[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[lwip-devel] [bug #50534] TFTP server does not copy terminating null of
From: |
David Rodgers |
Subject: |
[lwip-devel] [bug #50534] TFTP server does not copy terminating null of filename |
Date: |
Mon, 13 Mar 2017 16:04:50 -0400 (EDT) |
User-agent: |
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0 |
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/
- [lwip-devel] [bug #50534] TFTP server does not copy terminating null of filename,
David Rodgers <=
- [lwip-devel] [bug #50534] TFTP server does not copy terminating null of filename, Dirk Ziegelmeier, 2017/03/13
- [lwip-devel] [bug #50534] TFTP server does not copy terminating null of filename, Simon Goldschmidt, 2017/03/13
- Re: [lwip-devel] [bug #50534] TFTP server does not copy terminating null of filename, Axel Lin, 2017/03/13
- Re: [lwip-devel] [bug #50534] TFTP server does not copy terminating null of filename, Dirk Ziegelmeier, 2017/03/14
- Re: [lwip-devel] [bug #50534] TFTP server does not copy terminating null of filename, Axel Lin, 2017/03/14
- Re: [lwip-devel] [bug #50534] TFTP server does not copy terminating null of filename, Axel Lin, 2017/03/14
- Re: [lwip-devel] [bug #50534] TFTP server does not copy terminating null of filename, Dirk Ziegelmeier, 2017/03/14
- Re: [lwip-devel] [bug #50534] TFTP server does not copy terminating null of filename, Axel Lin, 2017/03/14
- Re: [lwip-devel] [bug #50534] TFTP server does not copy terminating null of filename, Sylvain Rochet, 2017/03/14