tabascoeye / lwip Goto Github PK

When an ICMP Echo request with either NO payload (data) or an arbirtary amount of 0x00 Bytes as payload reaches LWIP, it responds with an ICMP packet with a wrong checksum (0x0000 instead of 0xffff).

This is a comparison error in icmp.c

The socket API is missing a function to find out the last error that happened to a socket.
if you don't have an "errno" mechanism in your system, this might be a good alternative to find out what specific error happened last on a socket.

The routing function ip_route() is a bit too simple for the scenario that multiple netifs are on the same subnet.

For outbound packets, the function is called with the destination IP as parameter.
That destination is then matched against each netif's subnet (IP+Netmask) and the first match "wins" the packet.
This means that with two netifs on the same subnet, all outgoing packets will always run via the first one even packets from sockets which are bound to the IP of the second netif.
The netifs are not taken out of the routing list when they loose their link.
This means that a connection that you supposedly have established with the second netif will go down when you unplug the first netif.

A workaaround for that last problem is to have a link callback function and set the netif to down state, so that it is taken out of the routing list.

This will still create a bit of waiting time when the first cable is unplugged because the ARP tables need to be updated.

==> so:

1. we need to enhance the routing algorithm to take the "src" IP into account and choose the netif according to that criteria also
2. we might consider checking for link status and for another netif matching the destination subnet if the src IP is INADDR_ANY, so that the packet still goes out into the subnet even when one of the two interfaces is down

The loopback interface is not set "link up" when it is started.
Imho, it should because it always has a link and if you check your netifs for link up (e.g. before routing something via them) then the loopback will never be used.

==> add the line
netif_set_link_up(&loop_netif);
to netif.c in function netif_init(void)

Many feature requests and ideas for this stack have been denied with the argument of it then no longer being "light-weight".
I would love to counter that argument by:

• making the stack even more configurable with opt.h
• having Travis auto-build several versions of the stack configured differently, checking the sizes and reporting them for trackability (dashboard anyone?)
• maybe even strip out the configurable parts into separate .c files instead of having a minefield of #ifdefs in the code

When a socket is used from different tasks at the same time, the stack behavior is basically undefined.

To slowly work towards the goal of having a thread-safe stack that is still "light weight", we'll try to do the following bits:

• first allow another Task to close a socket that is blocking in accept(), connect() or select()
• analyze what would be necessary to make it fully independent:
  • the api_message interface is waiting for a op_completed semaphore and it cannot be determined what operation has completed when multiple ones where started
  • check for possible raceb conditions in the stack

When a new established connection is returned from lwip_accept(), the new socket does not inherit the socket options which were set for the listening socket.
(This is somewhat legitimate since many sockopts do not even work on a listening socket)

might want to check if this should be an option and if so do it via opt.h defines

then, for all listening cases, use only the listening pcb.
Also, there should be some handling for setsockopt() for listening sockets.

When a netconn_free() is called a second time on an already free'd netconn, the sys_sem_free(&conn->op_completed); call is being executed without first checking if the sem is still valid.
On my sys_arch implementation, calling sys_sem_free() on an already freed semaphore leads to an assertion.

I propose in the attached patch that a check for validity of the semaphore should be done before freeing and invalidating it (again).

When a UDP broadcast is received (destination IP 255.255.255.255), the udp input function checks:

1. is it a broadcast ==> yes
   AND
2. is the local pcb bound to INADDR_ANY?
   OR
3. does the destination IP match into the local IP/netmask?

So when you have bound you udp socket to a specific IP instead of INADDR_ANY, it will never receive UDP broadcasts because 2) and 3) will eval to false.

==> a third check is needed

the tcp_input() function is checking for multicast or broadcast packets and drops them because they shouldn't be processed there.

This should be extended to also drop the following potentially malicious packets:

• suspected TCP LAND attacks (src ip == dst ip == local IP of host)
• packets fro IP src 0.0.0.0

The TCP processing accepts RST packets which roughly fall into the ACK window instead of only accepting an RST packet if the seqno. is an exact match.

This is not a too big issue because an attacker would still have to guess around before hitting into the window, but it is still not too nice.

Some big players like most Linux distros still did not fix this issue because it is mainly bad for long lasting connections (e.g.BGP routes).

Still, fixing this in LwIP shouldn't be complicated (see the FreeBSD solution) and feasable

the socket API yields wrong Error codes for connect().
The usage of EALREADY and EISCONN are not handled as expected by the BSD socket API.

• in err.h, introduce new LWIP internal error code ERR_ALREADY
• in sockets.c add mapping of EISCONN to the err_to_errno_table array
• in api_msg in function do_connect() give out correct errors ERR_ISCONN and ERR_ALREADY in the correct cases

This is because:
lwip_close() -> netconn_delete() -> API_MSG -> do_delconn() -> do_close_internal() -> tcp_close() -> tcp_close_shutdown() -> tcp_send_fin() -> tcp_enqueue_flags()

That last function checks the TCP send queue and returns with ERR_MEM when the cable is unplugged because the send queue is full.
Thus, that error is given up to do_close_internal() which does NOT sys_sem_signal() the op_completed semaphore and in turn, the API_MSG is still being waited on.
Do_close_internal() sets poll_tcp and sent_tcp callbacks which re-call do_close_interal() until it works (when the cable is re-plugged and the send queue can be worked on).

When multiple netifs are on the same local subnet multiple problems arise:

• ip_route() is called with only destination IP as parameter. Thus the netif that is found as matching is always the first one on this subnet and not the one where the socket is bound to
• when the link goes down on that first netif, the packet is still sent its way and gets lost even though there is a perfectly fine netif on the same subnet ready to take over.
• the "default_netif" must match to the interface that has the configured "default gateway" because it is always the last chance netif when no route is found ==> when there are two netifs on the same subnet and the default gateway is on that net, the netif going link down must give its "default status" to the other netif on the same subnet

phew...

• I suggest starting with adding "src" to the parameters of ip_route() and choosing the netif also according to the source (if not INADDR_ANY etc.)
• we need to include the check for link_down in ip_route() as well so that other interfaces on a matching subnet have a chance to sent the packet. This also means that during the first run of the for loop, the correct netif (matching src ip) might be down so we have to either re-run the for loop, now taking other netifs on that subent into account or find another way to check for another netif on same subnet.
• the default_netif must always match the one netif that is a) up+link and b) can reach the default gateway.

The UDP sockets are not re-bound to the new IP address when it gets changed on a netif.