WiFiClientSecure half-duplex contract. #3019
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The ssl.h header was updated to follow a newer axtls-8266 version, which supports querying for availability. An additional ax_port_pending() function has been added to support this new availability querying capability. It returns the number of bytes available in the internal ClientContext without decrypting. This is similar to what SSL_pending() would do, but since it does not decrypt first, its result may not be exactly the number of readable bytes because of padding. For many operations where only the presence of new information is interesting, this is enough. Compared to using the currently present available() method, counting the bytes prior to decrypting does not require touching the axTLS buffer, which is shared between receiver and transmitter. So, using such a method reduces the chances of read/write collisions. As a side effect of the update, the ssl_client_new() call was modified, since the API has changed to use an SSL_EXTENSIONS structure instead of a direct string for the hostName.
Building on top of the previous axTLS update, the WiFiClientSecure
interface has been modified to avoid invalid memory access, which
could lead to any sort of application bugs (due to garbage input).
Previously, the WiFiClientSecure write() operation would directly
write to the SSL socket, disregarding the context of the object.
In particular, were there a read operation underway (i.e. a read
operation that had not finished consuming the entire axTLS's shared
receiver/transmitter buffer), the write() call would simply reuse
such buffer for its own encryption purposes, leaving any further
attempts to continue reading the buffer to fail because the read
data would be gone.
The first step to mitigate this bug risk is to make the
WiFiClientSecure explicitly state its half-duplex contract. So,
either writes to the socket should wait until any read operations are
finished, or the client should explicitly mark the read pointer as
invalid as soon as it starts writing.
In this patch, write() fails with a new SSL_ERROR_READ_LOCK error
code if there are unread bytes in the buffer, as a signal to the
calling code to try later when the buffer has been completely read.
This is the better alternative, since it is much less destructive
('write block' instead of lost data) and throwing away the data
is trivial to implement (just read until there's nothing available).
Indeed, a discardCache() method has been added to do just that.
Using discardCache() is more explicit so the user understands
the implications of their actions.
Besides, any code calling write() should already be dealing with
error values such as from connection errors.
The biggest offender in misusing the buffer is the connected()
call, which calls available(), which itself reads the whole
ssl input into axTLS's buffer. So, just checking that the client
is connected may imply any subsequent writes will fail. Instead,
a new wantRead() method has been added, which indicates whether
there's any ssl input left without reading it into axTLS's buffer,
i.e. without blocking. This heavily reduces buffer access collisions.
In summary, code that currently works with WiFiClientSecure will
continue working, possibly with more write errors if they were
using the client unsafely. Getting the previous behaviour (but
safely) just requires calling discardCache() before writing.
Use wantRead() instead of available() != 0. Unsafe situations
have been mitigated by not reading when it's not needed.
When you read, read until there's nothing left in the buffer,
otherwise explicitly discard the rest.
Other alternative solutions may be explored, such as expanding
WiFiClientSecure to be full duplex, but that might require much
more memory.
This was referenced Mar 3, 2017
|
+1 Also fixes the call to ssl_client_new and passes the hostname in the correct way. You can't connect to a host with multiple certificates using TLS extension SNI otherwise. |
|
Thanks Diegopx, I was having issues with getting kicked periodically from my MQTT connection (using pubsubclient), and this + your pubsubclient changes seemed to have fixed it. |
|
@igrr could you please take a look at this? |
|
@vshymanskyy could you add a reference to the issue you are facing? |
|
@igrr Do you have any sense of when this might be resolved? I'm encountering the same issue. I'd like to pull in the changes in the interim, but the associated axtls binaries are pretty old compared and I'm not sure I want to dive down the rabbit hole to check the changes and re-compile if it might be resolved soon. |
|
Closing in favor of #4024 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The internal axTLS library is half duplex, but WiFiClientSecure acts as if it was full duplex. This results in subtle bugs due to erroneous memory access. This branch fixes these bugs and establishes the half-duplex contract in the WiFiClientSecure interface.