Re: [lwip-devel] use-after-free caused by tcp_input_delayed_close

[Douglas]

Hi,

Took at look at this, trying to understand the code.

There is a guard in lwip_shutdown() which checks if sock->conn is NULL
and then returns ENOTCONN. Would that be the correct path in this case,
is ENOTCONN the expected response from the SHUT_RDWR if there has
already been a SHUT_WR and the other end has also closed?

I can't see anywhere that actually would set sock->conn to NULL if the
other end has closed? See this only in free_socket_locked() which is
caller from close(), but then the socket is also freed but that is not
the case for shutdown().

There is a similar guard in lwip_close(), but in that case it seems to
be guarded by get_socket() (seems that the only way sock->conn is set to
NULL is if the socket is also freed?)

Can you see how the errf callback that your patch enables actually works
to prevent the double free?

Perhaps socket specific handlers are needed for shutdown and close on
the api_msg side that guard against the case of a shudown and a closed
connection, where the pcb has been freed. Rather than just calling into
generic netconn function which appear to handle shutdown and closing
differently?

There seems to be some merit in the current code in
tcp_input_delayed_close(), that an error would not be appropriate if the
other end has simply closed after a shutdown on this end.

Douglas

On 1/24/19 8:01 PM, Michael Zimmermann wrote:
> Hi,
> 
> I'm running a tcp server using LWIP and upon termination of the
> connection, both sides do a "shutdown(sock, SHUT_WR)", wait for recv to
> return 0, call "shutdown(sock, SHUT_RDWR)", call "close(sock)".
> 
> The bug occurs in form of a race condition:
> - the lwip server calls SHUT_WR
> - the client calls SHUT_WR, once lwip saw this, it sets TF_RXCLOSED in
> "pcb->flags"
> - the client closes the connection, lwip sees this, adds TF_CLOSED to
> recv_flags, and then deletes the pcb within "tcp_input_delayed_close".
> 
> The problem here is that "tcp_input_delayed_close" only calls the
> "pcb->errf" callback on this condition:
> "if (!(pcb->flags & TF_RXCLOSED))"
> 
> I don't really know why that was done in first place, but because of
> this, the pcb gets freed without notifying the user(which would set
> conn->pcb.tcp to NULL) in case the RX side was closed already.
> 
> On the next call to shutdown or close, this results in use-after-free.
> 
> I'm posting this to the mailing list first instead of the bug tracker to
> discuss the intention behind the condition and to come up with a proper
> solution.
> 
> Thanks
> Michael Zimmermann
> 
> IOTΛ Data Marketplace Member · MS Azure IoT Gold Partner · Apple MFi
> Developer · Bluetooth SIG · zigbee Alliance · LoRa Alliance · Thread Group
> 
> grandcentrix GmbH · Holzmarkt 1 · 50676 Köln · Deutschland
> | t <https://twitter.com/grandcentrix> | f
> <https://www.facebook.com/GrandCentrix/> | in
> <https://www.linkedin.com/company/grandcentrix> | phone:
> +49-221-677-860-0 | email: address@hidden
> <mailto:address@hidden>
> 
> Amtsgericht Köln | HRB  70119 | Geschäftsführer: R. Rottmann, M. Willnow
> | USt.-IdNr.: DE266333969
> 
> _______________________________________________
> lwip-devel mailing list
> address@hidden
> https://lists.nongnu.org/mailman/listinfo/lwip-devel
>