Uninitialized pointer at using_mbedtls.c TLS_FreeRTOS_Connect

Libraries
1

In FreeRTOS/FreeRTOS-Plus/Source/Application-Protocols/network_transport/using_mbedtls/using_mbedtls.c function TLS_FreeRTOS_Connect, the pTlsTransportParams->tcpSocket is left uninitialized if the Sockets_Connect() fails (at least when using implementation from freertos_plus_tcp/sockets_wrapper.c). At the end of that function, this pointer is checked against SOCKETS_INVALID_SOCKET value and potentially Sockets_Disconnect() is called against with a uninitialized pointer. This may cause a crash in free or cause duplicate free if a stale pointer was there (ie when re-connecting).
I’d suggest initializing the pointer value to SOCKETS_INVALID_SOCKET before TLS_FreeRTOS_Connect() to avoid using an uninitialized value.

2

Thanks for taking the time to report this. Setting *pTcpSocket to FREERTOS_INVALID_SOCKET here FreeRTOS/sockets_wrapper.c at 202112.00 · FreeRTOS/FreeRTOS · GitHub will ensure the socket is not left uninitialised for any function calling the demo’s helper function Sockets_Connect(). Would be great if you can create a pull request to do that, or something similar if you think there is a better alternative - otherwise I can create a ticket to get it updated.

3

I politely disagree that the sockets_wrapper is the correct location to fix the bug. Keeping the TLS_FreeRTOS_Connect() function unchanged so that it is expecting a valid pointer even if the Sockets_Connect() function call returns failure makes the code more fragile and hides potential bugs from both the reviewers and analyzers. Also there is plenty of code, which does not modify arguments in case of failure (ie asprintf in glibc) and it is a simple mistake to assume this here. When setting the pointer to SOCKETS_INVALID_SOCKET at TLS_FreeRTOS_Connect() when the Sockets_Connect() fails or before the function call altogether, all cases are covered, whatever the Sockets_Connect() function happens to do in the future. If there are other use cases that have this bug, all of them should be fixed.

I can provide a pull request for this change. Also I have nothing against if FreeRTOS developers want to fix in a way they like.

4

The suggestion looks good to me. Please raise a PR.

5

I agree with your rationale for this, so agreeing with your and disagreeing with me :wink:

1 Like
6

It looks like in the mean time this part of the code has been somewhat reworked on the dev branch. The check against SOCKETS_INVALID_SOCKET is now inside the TCP_Sockets_Disconnect function. That bug is still there in the updated code. There are separate private invalid socket values for FreeRTOS-Plus-TCP and cellular wrappers, so assigning one of them would not be nice anyway. But there is another nice solution for this bug - setting socketStatus = -1 at the beginning and checking if it is 0 (socket got connected) before calling TCP_Sockets_Disconnect.

In addition to that bug, there is a second bug of dereferencing a null pointer of pTlsTransportParams near the end of the function if first checks at the beginning of the function fail.
There is also a third bug of calling sslContextFree when potentially all or some parts of it were not properly initialized yet.

It looks like a much bigger can of worms than I initially thought. I started fixing some of it, but it takes more effort to fix everything. There are three different files that have that bug and need fixing. Also I don’t have the platforms/setups ready to test them. Could someone working on that code and has all development set up take over and fix it themselves?

7

Sure - I will ask my teammate to look at it. Thank you for bringing it to our attention.

8

Hi @antisullin,
Sorry for late response.
I’ve created a pull request to fix this bug.

Here is the summary of the PR:

Initialize the Socket_t pointer to NULL before calling TCP_Sockets_Connect() and reset it to NULL if any error happens during TLS_FreeRTOS_Connect().

Thanks for your help.