use-after-free caused by tcp_input_delayed_close

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

use-after-free caused by tcp_input_delayed_close

Michael Zimmermann
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 | f | in | phone: +49-221-677-860-0 | email: [hidden email]

Amtsgericht Köln | HRB  70119 | Geschäftsführer: R. Rottmann, M. Willnow | USt.-IdNr.: DE266333969

_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

Re: use-after-free caused by tcp_input_delayed_close

goldsimon@gmx.de
Am 24.01.2019 um 10:01 schrieb Michael Zimmermann:
> 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)".

Which version of lwIP are you using? If this problem persists with
current git master (maybe it's enough to compare the code?), could you
please file a bug report to ensure this doesn't get lost?

Thanks,
Simon


> 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: [hidden email]
> <mailto:[hidden email]>
>
> Amtsgericht Köln | HRB  70119 | Geschäftsführer: R. Rottmann, M. Willnow
> | USt.-IdNr.: DE266333969
>
> _______________________________________________
> lwip-devel mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/lwip-devel
>


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

Re: use-after-free caused by tcp_input_delayed_close

Michael Zimmermann
I'm using espressif's modified version of LWIP for the esp32 which is based on 2.0.3.
I could however reproduce this issue using the latest master branch of lwip and the linux port.

Also it looks like the code in question was added in 2012 and has never been changed that much.
and the questionable condition is in the following line today:

The problem does seem to go away if I just unconditionally call TCP_EVENT_ERR, but I'm not sure if that is the correct solution or if this would have unwanted side effects.

If you need to be able to reproduce this I'll happily upload sample code for both the client and the server side.

Thanks
Michael Zimmermann

On Thu, Jan 24, 2019 at 8:37 PM [hidden email] <[hidden email]> wrote:
Am 24.01.2019 um 10:01 schrieb Michael Zimmermann:
> 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)".

Which version of lwIP are you using? If this problem persists with
current git master (maybe it's enough to compare the code?), could you
please file a bug report to ensure this doesn't get lost?

Thanks,
Simon


> 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: [hidden email]
> <mailto:[hidden email]>
>
> Amtsgericht Köln | HRB  70119 | Geschäftsführer: R. Rottmann, M. Willnow
> | USt.-IdNr.: DE266333969
>
> _______________________________________________
> lwip-devel mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/lwip-devel
>


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel

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 | f | in | phone: +49-221-677-860-0 | email: [hidden email]

Amtsgericht Köln | HRB  70119 | Geschäftsführer: R. Rottmann, M. Willnow | USt.-IdNr.: DE266333969

_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

Re: use-after-free caused by tcp_input_delayed_close

Douglas
In reply to this post by Michael Zimmermann
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: [hidden email]
> <mailto:[hidden email]>
>
> Amtsgericht Köln | HRB  70119 | Geschäftsführer: R. Rottmann, M. Willnow
> | USt.-IdNr.: DE266333969
>
> _______________________________________________
> lwip-devel mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/lwip-devel
>


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

Re: use-after-free caused by tcp_input_delayed_close

Douglas
In reply to this post by Michael Zimmermann
Hi,

Might the following guard in lwip_netconn_do_close() protect the
shutdown() call? "if ((msg->conn->pcb.tcp != NULL)"

The close() path appears to use lwip_netconn_do_delconn() which has the
following guard "if (msg->conn->pcb.tcp != NULL)"

It looks like err_tcp() is called when the other end closes the
connection, from tcp_input_delayed_close(), and this function sets
conn->pcb.tcp to NULL, and that NULL should protect later calls to
shutdown() which would return ERR_CONN, and to close().

Would you be able to check this, see if these guards are working when
this can be reproduced? Can you get a better idea of the failure path.

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: [hidden email]
> <mailto:[hidden email]>
>
> Amtsgericht Köln | HRB  70119 | Geschäftsführer: R. Rottmann, M. Willnow
> | USt.-IdNr.: DE266333969
>
> _______________________________________________
> lwip-devel mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/lwip-devel
>


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

Re: use-after-free caused by tcp_input_delayed_close

goldsimon@gmx.de
Michal, Douglas,

is this still an issue? If so, would someone care to file a bug in our
bugtracker?

Thanks,
Simon

Am 20.04.2019 um 17:07 schrieb Douglas:

> Hi,
>
> Might the following guard in lwip_netconn_do_close() protect the
> shutdown() call? "if ((msg->conn->pcb.tcp != NULL)"
>
> The close() path appears to use lwip_netconn_do_delconn() which has the
> following guard "if (msg->conn->pcb.tcp != NULL)"
>
> It looks like err_tcp() is called when the other end closes the
> connection, from tcp_input_delayed_close(), and this function sets
> conn->pcb.tcp to NULL, and that NULL should protect later calls to
> shutdown() which would return ERR_CONN, and to close().
>
> Would you be able to check this, see if these guards are working when
> this can be reproduced? Can you get a better idea of the failure path.
>
> 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: [hidden email]
>> <mailto:[hidden email]>
>>
>> Amtsgericht Köln | HRB  70119 | Geschäftsführer: R. Rottmann, M. Willnow
>> | USt.-IdNr.: DE266333969
>>
>> _______________________________________________
>> lwip-devel mailing list
>> [hidden email]
>> https://lists.nongnu.org/mailman/listinfo/lwip-devel
>>
>
>
> _______________________________________________
> lwip-devel mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/lwip-devel
>


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel