[Bug 1001522] Array index out of bounds in tftp_server.c

[bugzilla-daemon at bugs dot ecos dot sourceware dot org]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001522

--- Comment #3 from Grant Edwards <grant.b.edwards@gmail.com> 2012-03-09 15:55:07 GMT ---
(In reply to comment #2)

> Yes there's a bug here, but it's not quite that. That inner loop
> _should_ go and close both the ipv4 and ipv6 sockets, because once
> another thread wakes up to be the tftp server, it will want to open
> new ones of each. We'd leak sockets if we didn't close both. So that
> bit's ok.
>
> However, if the client sends an unsupported opcode, then this bit runs:
>   tftpd_send_error(server->s[i],hdr,TFTP_EBADOP,&from_addr,from_len);
> the problem here being that we've just closed server->s[i].

But I don't think we did just close server->s[i].

At that point, i == CYGNUM_NET_MAX_INET_PROTOS

We closed server->s[0] through server->s[CYGNUM_NET_MAX_INET_PROTOS-1]

server->s[i] is past the end of array server->s[].

> So that's broken.  Either it should make a new socket and use that
> for sending, or send the error before it closes this server
> socket. A cheaty solution is probably something like:
>
>               int server_sock = -1; // at top of function
>               // ...
>               for (i=0; i < CYGNUM_NET_MAX_INET_PROTOS; i++) {
>                 if (server->s[i]) {
>                   server_sock = server->s[i];
>                   server->s[i] = 0;
>                 }
>               }
> 
> and then use server_sock later on, and close it after the end of the 'switch'
> block.

Wouldn't that leak sockets if more than one is open?  The loop would
set all of them to 0 [which is also a bug, see below], but it only
remembers the last one for closing later.

Using the integer value 0 as a sentinal value meaning "no socket" is a
problem.  If the tftp server is the first task to create a socket,
it's going to get fd 0.  Using -1 to mean "no socket" is fine.  [I've
been burned by the 0 == no-socket bug a couple times times in the past
few years in our eCos application code.]

> It's all rather crufty code - there's a window where the server socket is
> closed and hasn't been reopened (which will cause ICMP port unreachable
> messages to be sent, not merely have the TFTP request be ignored). It shouldn't
> be closed at all really, but that would require other changes too. And I
> dislike the server handle being a cast to an int of the server address. The
> whole setup just isn't very good.
> 
> What do you think?

I still think the "close loop" shouldn't be using i as an index
variable since it's inside an outer loop that's using i as an index
variable.  That's causing the call to tftpd_send_error() to access
past the end of access server->s[].

If that were fixed by creating a seperate index for the "close loop",
then the bug you're describing would occur: sending data out using
descriptor 0 because we just closed the server->s[i] and put a "0"
sentinal value in s[].

Why not move the "close loop" to below the switch statement that calls
tftpd_send_error()?

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.