From dc0ef089eea8e04a4088b0563f4a417960b9d4fa Mon Sep 17 00:00:00 2001 From: Slavey Karadzhov Date: Tue, 11 Jun 2024 17:13:08 +0200 Subject: [PATCH 1/8] Fix to StartTLS command. --- Sming/Components/Network/src/Network/SmtpClient.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Sming/Components/Network/src/Network/SmtpClient.cpp b/Sming/Components/Network/src/Network/SmtpClient.cpp index a604133bd1..fc6cc9f5ea 100644 --- a/Sming/Components/Network/src/Network/SmtpClient.cpp +++ b/Sming/Components/Network/src/Network/SmtpClient.cpp @@ -390,8 +390,13 @@ int SmtpClient::smtpParse(char* buffer, size_t len) RETURN_ON_ERROR(SMTP_CODE_SERVICE_READY); if(!useSsl && (options & SMTP_OPT_STARTTLS)) { - useSsl = true; - TcpConnection::internalOnConnected(ERR_OK); + if(!TcpConnection::sslCreateSession()) { + bitClear(options, SMTP_OPT_STARTTLS); + } else { + TcpConnection::ssl->hostName = url.Host; + useSsl = true; + TcpConnection::internalOnConnected(ERR_OK); + } } sendString(F("EHLO ") + url.Host + "\r\n"); From e59fe3678ea9384cf42d02ffb862cba357c150cc Mon Sep 17 00:00:00 2001 From: Slavey Karadzhov Date: Tue, 11 Jun 2024 17:19:10 +0200 Subject: [PATCH 2/8] Fix to the Url stringification with username. --- Sming/Components/Network/src/Network/Url.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Sming/Components/Network/src/Network/Url.cpp b/Sming/Components/Network/src/Network/Url.cpp index b23df5aaac..722ec71648 100644 --- a/Sming/Components/Network/src/Network/Url.cpp +++ b/Sming/Components/Network/src/Network/Url.cpp @@ -86,6 +86,8 @@ String Url::toString() const result += ':'; result += Password; } + + result += '@'; } result += getHostWithPort(); From f98da806f2064f099dac430455fb2eadcbd5a218 Mon Sep 17 00:00:00 2001 From: Slavey Karadzhov Date: Tue, 11 Jun 2024 18:21:31 +0200 Subject: [PATCH 3/8] Fix memory leaks reported bin MailMessage. --- Sming/Components/Network/src/Network/MailMessage.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Sming/Components/Network/src/Network/MailMessage.h b/Sming/Components/Network/src/Network/MailMessage.h index f5357ebc9e..7f31f73cf3 100644 --- a/Sming/Components/Network/src/Network/MailMessage.h +++ b/Sming/Components/Network/src/Network/MailMessage.h @@ -38,6 +38,15 @@ class MailMessage String subject; String cc; + ~MailMessage() + { + delete stream; + for(auto attachment: attachments) { + delete attachment.headers; + delete attachment.stream; + } + } + /** * @brief Set a header value * @param name From 1b577cca4e33e418b953b190b7d07d51262f07eb Mon Sep 17 00:00:00 2001 From: Slavey Karadzhov Date: Wed, 12 Jun 2024 10:59:03 +0200 Subject: [PATCH 4/8] Extract the logic for activating SSL to a separate method in TcpConnection. If SSL is not compiled don't bail out and try to communicate via plain-text tcp. --- .../Network/src/Network/MailMessage.h | 2 +- .../Network/src/Network/SmtpClient.cpp | 27 +++++++++++++------ .../Network/src/Network/TcpConnection.cpp | 23 ++++++++++++++++ .../Network/src/Network/TcpConnection.h | 7 +++++ samples/SmtpClient/app/application.cpp | 2 +- 5 files changed, 51 insertions(+), 10 deletions(-) diff --git a/Sming/Components/Network/src/Network/MailMessage.h b/Sming/Components/Network/src/Network/MailMessage.h index 7f31f73cf3..7ee8d42ccf 100644 --- a/Sming/Components/Network/src/Network/MailMessage.h +++ b/Sming/Components/Network/src/Network/MailMessage.h @@ -41,7 +41,7 @@ class MailMessage ~MailMessage() { delete stream; - for(auto attachment: attachments) { + for(auto attachment : attachments) { delete attachment.headers; delete attachment.stream; } diff --git a/Sming/Components/Network/src/Network/SmtpClient.cpp b/Sming/Components/Network/src/Network/SmtpClient.cpp index fc6cc9f5ea..1a4f44a777 100644 --- a/Sming/Components/Network/src/Network/SmtpClient.cpp +++ b/Sming/Components/Network/src/Network/SmtpClient.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #define ADVANCE \ { \ @@ -390,12 +391,14 @@ int SmtpClient::smtpParse(char* buffer, size_t len) RETURN_ON_ERROR(SMTP_CODE_SERVICE_READY); if(!useSsl && (options & SMTP_OPT_STARTTLS)) { - if(!TcpConnection::sslCreateSession()) { - bitClear(options, SMTP_OPT_STARTTLS); - } else { - TcpConnection::ssl->hostName = url.Host; - useSsl = true; - TcpConnection::internalOnConnected(ERR_OK); + if(!enableSsl(url.Host)) { + /* + * Excerpt from RFC 3207: If, + * after having issued the STARTTLS command, the client finds out that + * some failure prevents it from actually starting a TLS handshake, then + * it SHOULD abort the connection. + */ + return 0; } } @@ -425,8 +428,16 @@ int SmtpClient::smtpParse(char* buffer, size_t len) if(isLastLine) { state = eSMTP_Ready; if(!useSsl && (options & SMTP_OPT_STARTTLS)) { - state = eSMTP_StartTLS; - } else if(url.User && authMethods.count()) { + if(Ssl::factory != nullptr) { + state = eSMTP_StartTLS; + break; + } + + bitClear(options, SMTP_OPT_STARTTLS); + debug_w("[SMTP] SSL required, no factory. Continue plain-text communication."); + } + + if(url.User && authMethods.count()) { state = eSMTP_SendAuth; } } diff --git a/Sming/Components/Network/src/Network/TcpConnection.cpp b/Sming/Components/Network/src/Network/TcpConnection.cpp index 0aae89fc2e..04c41f2738 100644 --- a/Sming/Components/Network/src/Network/TcpConnection.cpp +++ b/Sming/Components/Network/src/Network/TcpConnection.cpp @@ -423,6 +423,29 @@ err_t TcpConnection::internalOnConnected(err_t err) return res; } +bool TcpConnection::enableSsl(const String& hostName) +{ + if(tcp == nullptr) { + return false; + } + + if(tcp->state != ESTABLISHED) { + return false; + } + + if(!sslCreateSession()) { + return false; + } + + if(hostName) { + ssl->hostName = hostName; + } + + useSsl = true; + useSsl = (internalOnConnected(ERR_OK) == ERR_OK); + return useSsl; +} + err_t TcpConnection::internalOnReceive(pbuf* p, err_t err) { sleep = 0; diff --git a/Sming/Components/Network/src/Network/TcpConnection.h b/Sming/Components/Network/src/Network/TcpConnection.h index 3ea5367418..e61e8715e8 100644 --- a/Sming/Components/Network/src/Network/TcpConnection.h +++ b/Sming/Components/Network/src/Network/TcpConnection.h @@ -154,6 +154,13 @@ class TcpConnection : public IpConnection return ssl; } + /** + * @brief Enables Secure Socket Layer on the current connection + * @param hostName + * @retval true on success, false otherwise + */ + bool enableSsl(const String& hostName = nullptr); + protected: void initialize(tcp_pcb* pcb); bool internalConnect(IpAddress addr, uint16_t port); diff --git a/samples/SmtpClient/app/application.cpp b/samples/SmtpClient/app/application.cpp index 1b6083cc6c..411150add1 100644 --- a/samples/SmtpClient/app/application.cpp +++ b/samples/SmtpClient/app/application.cpp @@ -24,7 +24,7 @@ int onServerError(SmtpClient& client, int code, char* status) { debugf("Status: %s", status); - return 0; // return non-zero value to abort the connection + return -1; // return non-zero value to abort the connection } int onMailSent(SmtpClient& client, int code, char* status) From 329462d00a5d8097b640899ed3626c4b4a363350 Mon Sep 17 00:00:00 2001 From: Slavey Karadzhov Date: Wed, 12 Jun 2024 13:15:40 +0200 Subject: [PATCH 5/8] Implement review recommendation. More to follow. --- Sming/Components/Network/src/Network/TcpConnection.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Sming/Components/Network/src/Network/TcpConnection.cpp b/Sming/Components/Network/src/Network/TcpConnection.cpp index 04c41f2738..f2b1199c44 100644 --- a/Sming/Components/Network/src/Network/TcpConnection.cpp +++ b/Sming/Components/Network/src/Network/TcpConnection.cpp @@ -437,9 +437,7 @@ bool TcpConnection::enableSsl(const String& hostName) return false; } - if(hostName) { - ssl->hostName = hostName; - } + ssl->hostName = hostName; useSsl = true; useSsl = (internalOnConnected(ERR_OK) == ERR_OK); From 6228d3475e20ef3b37c858215fa2eddfe3ec4f98 Mon Sep 17 00:00:00 2001 From: Slavey Karadzhov Date: Wed, 12 Jun 2024 15:03:31 +0200 Subject: [PATCH 6/8] Replace stream pointer with std::unique_ptr. --- Sming/Components/Network/src/Network/MailMessage.cpp | 6 ++---- Sming/Components/Network/src/Network/MailMessage.h | 5 ++--- Sming/Components/Network/src/Network/SmtpClient.cpp | 9 ++++----- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/Sming/Components/Network/src/Network/MailMessage.cpp b/Sming/Components/Network/src/Network/MailMessage.cpp index 19e33f92cb..6fdb8666de 100644 --- a/Sming/Components/Network/src/Network/MailMessage.cpp +++ b/Sming/Components/Network/src/Network/MailMessage.cpp @@ -48,13 +48,11 @@ MailMessage& MailMessage::setBody(String&& body, MimeType mime) noexcept MailMessage& MailMessage::setBody(IDataSourceStream* stream, MimeType mime) { - if(this->stream != nullptr) { + if(this->stream) { debug_e("MailMessage::setBody: Discarding already set stream!"); - delete this->stream; - this->stream = nullptr; } - this->stream = stream; + this->stream.reset(stream); headers[HTTP_HEADER_CONTENT_TYPE] = toString(mime); return *this; diff --git a/Sming/Components/Network/src/Network/MailMessage.h b/Sming/Components/Network/src/Network/MailMessage.h index 7ee8d42ccf..5d6f6eb745 100644 --- a/Sming/Components/Network/src/Network/MailMessage.h +++ b/Sming/Components/Network/src/Network/MailMessage.h @@ -40,8 +40,7 @@ class MailMessage ~MailMessage() { - delete stream; - for(auto attachment : attachments) { + for(auto& attachment : attachments) { delete attachment.headers; delete attachment.stream; } @@ -115,7 +114,7 @@ class MailMessage MailMessage& addAttachment(IDataSourceStream* stream, const String& mime, const String& filename = ""); private: - IDataSourceStream* stream = nullptr; + std::unique_ptr stream = nullptr; HttpHeaders headers; Vector attachments; }; diff --git a/Sming/Components/Network/src/Network/SmtpClient.cpp b/Sming/Components/Network/src/Network/SmtpClient.cpp index 1a4f44a777..68d886b013 100644 --- a/Sming/Components/Network/src/Network/SmtpClient.cpp +++ b/Sming/Components/Network/src/Network/SmtpClient.cpp @@ -289,7 +289,7 @@ void SmtpClient::sendMailHeaders(MailMessage* mail) if(!mail->headers.contains(HTTP_HEADER_CONTENT_TRANSFER_ENCODING)) { mail->headers[HTTP_HEADER_CONTENT_TRANSFER_ENCODING] = _F("quoted-printable"); - mail->stream = new QuotedPrintableOutputStream(mail->stream); + mail->stream.reset(new QuotedPrintableOutputStream(mail->stream.release())); } if(!mail->attachments.isEmpty()) { @@ -298,13 +298,13 @@ void SmtpClient::sendMailHeaders(MailMessage* mail) text.headers = new HttpHeaders(); (*text.headers)[HTTP_HEADER_CONTENT_TYPE] = mail->headers[HTTP_HEADER_CONTENT_TYPE]; (*text.headers)[HTTP_HEADER_CONTENT_TRANSFER_ENCODING] = mail->headers[HTTP_HEADER_CONTENT_TRANSFER_ENCODING]; - text.stream = mail->stream; + text.stream = mail->stream.release(); mail->attachments.insertElementAt(text, 0); mail->headers.remove(HTTP_HEADER_CONTENT_TRANSFER_ENCODING); mail->headers[HTTP_HEADER_CONTENT_TYPE] = F("multipart/mixed; boundary=") + mStream->getBoundary(); - mail->stream = mStream; + mail->stream.reset(mStream); } for(auto hdr : mail->headers) { @@ -320,8 +320,7 @@ bool SmtpClient::sendMailBody(MailMessage* mail) } delete stream; - stream = mail->stream; // avoid intermediate buffers - mail->stream = nullptr; + stream = mail->stream.release(); // avoid intermediate buffers return false; } From 30c0838904effe12571367c1a11e6f11b0639874 Mon Sep 17 00:00:00 2001 From: Slavey Karadzhov Date: Wed, 12 Jun 2024 15:26:41 +0200 Subject: [PATCH 7/8] Apply suggested review change. --- Sming/Components/Network/src/Network/TcpConnection.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Sming/Components/Network/src/Network/TcpConnection.cpp b/Sming/Components/Network/src/Network/TcpConnection.cpp index f2b1199c44..5f442a7c30 100644 --- a/Sming/Components/Network/src/Network/TcpConnection.cpp +++ b/Sming/Components/Network/src/Network/TcpConnection.cpp @@ -440,7 +440,9 @@ bool TcpConnection::enableSsl(const String& hostName) ssl->hostName = hostName; useSsl = true; - useSsl = (internalOnConnected(ERR_OK) == ERR_OK); + if(internalOnConnected(ERR_OK) != ERR_OK) { + useSsl = false; + } return useSsl; } From 75ede356f40e66a8053c72d47e3858a3c81f5d2d Mon Sep 17 00:00:00 2001 From: Slavey Karadzhov Date: Thu, 13 Jun 2024 16:58:52 +0200 Subject: [PATCH 8/8] Further review recommendations. --- Sming/Components/Network/src/Network/SmtpClient.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sming/Components/Network/src/Network/SmtpClient.cpp b/Sming/Components/Network/src/Network/SmtpClient.cpp index 68d886b013..eb42a9fe53 100644 --- a/Sming/Components/Network/src/Network/SmtpClient.cpp +++ b/Sming/Components/Network/src/Network/SmtpClient.cpp @@ -289,7 +289,7 @@ void SmtpClient::sendMailHeaders(MailMessage* mail) if(!mail->headers.contains(HTTP_HEADER_CONTENT_TRANSFER_ENCODING)) { mail->headers[HTTP_HEADER_CONTENT_TRANSFER_ENCODING] = _F("quoted-printable"); - mail->stream.reset(new QuotedPrintableOutputStream(mail->stream.release())); + mail->stream = std::make_unique(mail->stream.release()); } if(!mail->attachments.isEmpty()) {