Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WiFiClientSecure: don't trash unread decrypted data when writing #4024

Merged
merged 2 commits into from
Dec 26, 2017
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 108 additions & 2 deletions libraries/ESP8266WiFi/src/WiFiClientSecure.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ extern "C"
#include "osapi.h"
#include "ets_sys.h"
}
#include <list>
#include <errno.h>
#include "debug.h"
#include "ESP8266WiFi.h"
Expand All @@ -50,6 +51,26 @@ extern "C"
#define SSL_DEBUG_OPTS 0
#endif


typedef struct BufferItem
{
BufferItem(const uint8_t* data_, size_t size_)
: size(size_), data(new uint8_t[size])
{
if (data.get() != nullptr) {
memcpy(data.get(), data_, size);
} else {
DEBUGV(":wcs alloc %d failed\r\n", size_);
size = 0;
}
}

size_t size;
std::unique_ptr<uint8_t[]> data;
} BufferItem;

typedef std::list<BufferItem> BufferList;

class SSLContext
{
public:
Expand Down Expand Up @@ -139,6 +160,10 @@ class SSLContext
_available -= will_copy;
if (_available == 0) {
_read_ptr = nullptr;
/* Send pending outgoing data, if any */
if (_hasWriteBuffers()) {
_writeBuffersSend();
}
}
return will_copy;
}
Expand All @@ -155,10 +180,34 @@ class SSLContext
--_available;
if (_available == 0) {
_read_ptr = nullptr;
/* Send pending outgoing data, if any */
if (_hasWriteBuffers()) {
_writeBuffersSend();
}
}
return result;
}

int write(const uint8_t* src, size_t size)
{
if (!_available) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this check !_available or !hasData()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to check for hasData, as far as i can tell — we only need to make sure that the fragment buffer is empty.

if (_hasWriteBuffers()) {
int rc = _writeBuffersSend();
if (rc < 0) {
return rc;
}
}
return _write(src, size);
}
/* Some received data is still present in the axtls fragment buffer.
We can't call ssl_write now, as that will overwrite the contents of
the fragment buffer, corrupting the received data.
Save a copy of the outgoing data, and call ssl_write when all
recevied data has been consumed by the application.
*/
return _writeBufferAdd(src, size);
}

int peek()
{
if (!_available) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this check !available or !hasData()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. The logic is:

  1. Is there some incoming plaintext data? If not, we'll try to get any received data from the TCP stack, and decrypt it.
  2. If after step 1 there is some plaintext data, return the first byte of it.

Expand Down Expand Up @@ -193,6 +242,12 @@ class SSLContext
return cb;
}

// similar to availble, but doesn't return exact size
bool hasData()
{
return _available > 0 || (s_io_ctx && s_io_ctx->getSize() > 0);
}

bool loadObject(int type, Stream& stream, size_t size)
{
std::unique_ptr<uint8_t[]> buf(new uint8_t[size]);
Expand Down Expand Up @@ -282,12 +337,63 @@ class SSLContext
return _available;
}

int _write(const uint8_t* src, size_t size)
{
if (!_ssl) {
return 0;
}

int rc = ssl_write(_ssl, src, size);
if (rc >= 0) {
return rc;
}
DEBUGV(":wcs write rc=%d\r\n", rc);
return rc;
}

int _writeBufferAdd(const uint8_t* data, size_t size)
{
if (!_ssl) {
return 0;
}

_writeBuffers.emplace_back(data, size);
if (_writeBuffers.back().data.get() == nullptr) {
_writeBuffers.pop_back();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mayeb add a DEBUGV msg here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a DEBUGV into BufferItem constructor, where the memory allocation failure is first detected.

return 0;
}
return size;
}

int _writeBuffersSend()
{
while (!_writeBuffers.empty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a lengthy operation? Are there timing concerns due to the exection time of this loop?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on the amount of data queued for transmission. In most cases, this will be zero. For protocols like MQTT this may send a few packets. There will be yields in ClientContext, so we don't need to worry about starving the WiFi stack here.

auto& first = _writeBuffers.front();
int rc = _write(first.data.get(), first.size);
_writeBuffers.pop_front();
if (rc < 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a DEBUGV msg here for the failure and for the buffers getting dropped (data drop)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added DEBUGV for ssl_write failure into _write and a message about buffers being dropped here.

if (_hasWriteBuffers()) {
DEBUGV(":wcs _writeBuffersSend dropping unsent data\r\n");
_writeBuffers.clear();
}
return rc;
}
}
return 0;
}

bool _hasWriteBuffers()
{
return !_writeBuffers.empty();
}

static SSL_CTX* _ssl_ctx;
static int _ssl_ctx_refcnt;
SSL* _ssl = nullptr;
int _refcnt = 0;
const uint8_t* _read_ptr = nullptr;
size_t _available = 0;
BufferList _writeBuffers;
bool _allowSelfSignedCerts = false;
static ClientContext* s_io_ctx;
};
Expand Down Expand Up @@ -371,7 +477,7 @@ size_t WiFiClientSecure::write(const uint8_t *buf, size_t size)
return 0;
}

int rc = ssl_write(*_ssl, buf, size);
int rc = _ssl->write(buf, size);
if (rc >= 0) {
return rc;
}
Expand Down Expand Up @@ -458,7 +564,7 @@ err x N N
uint8_t WiFiClientSecure::connected()
{
if (_ssl) {
if (_ssl->available()) {
if (_ssl->hasData()) {
return true;
}
if (_client && _client->state() == ESTABLISHED && _ssl->connected()) {
Expand Down