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

Improve memory management of SSL fingerprint data #1618

Merged
merged 6 commits into from
Feb 14, 2019
Merged
Changes from 2 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
1 change: 1 addition & 0 deletions Sming/SmingCore/Data/Stream/DataSourceStream.h
Original file line number Diff line number Diff line change
@@ -19,6 +19,7 @@
enum StreamType {
eSST_Invalid, ///< Stream content not valid
eSST_Memory, ///< Memory data stream
eSST_Serial, ///< Serial port
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... Do we need those specific types or more general ones would be better?

Copy link
Contributor Author

@mikee47 mikee47 Feb 13, 2019

Choose a reason for hiding this comment

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

I do wonder if we need StreamType at all? In the framework it's barely used:

IDataSourceStream::isValid()

Inherited classes can just override this method directly, rather than returning eSST_Invalid from getStreamType()

HttpRequest::getBody() and HttpResponse::getBody()

Both of these fail if the stream type isn't eSST_Memory, but I'm not sure that check is even necessary.

So we could fix these points and deprecate the getStreamType() method completely. We change IDataSourceStream::getStreamType() to return eSST_Unknown and remove it from HardwareSerial.

Copy link
Contributor

Choose a reason for hiding this comment

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

So we could fix these points and deprecate the getStreamType() method completely. We change IDataSourceStream::getStreamType() to return eSST_Unknown and can then omit it from HardwareSerial completely.

That sounds good to me. Just make sure that it is in a separate PR.

eSST_File, ///< File data stream
eSST_Template, ///< Template data stream
eSST_JsonObject, ///< JSON object data stream
21 changes: 18 additions & 3 deletions Sming/SmingCore/HardwareSerial.h
Original file line number Diff line number Diff line change
@@ -14,7 +14,7 @@
#define _HARDWARESERIAL_H_

#include "WiringFrameworkDependencies.h"
#include "Stream.h"
#include "Data/Stream/ReadWriteStream.h"
#include "Delegate.h"
#include "espinc/uart.h"

@@ -88,7 +88,7 @@ enum SerialMode { SERIAL_FULL = UART_FULL, SERIAL_RX_ONLY = UART_RX_ONLY, SERIAL
#endif

/// Hardware serial class
class HardwareSerial : public Stream
class HardwareSerial : public ReadWriteStream
{
public:
/** @brief Create instance of a hardware serial port object
@@ -101,6 +101,11 @@ class HardwareSerial : public Stream

~HardwareSerial();

virtual StreamType getStreamType() const
{
return eSST_Serial;
}

void setPort(int uartPort)
{
end();
@@ -239,11 +244,21 @@ class HardwareSerial : public Stream
* @note Although this shares the same name as the method in IDataSourceStream,
* behaviour is different because in effect the 'seek' position is changed by this call.
*/
virtual uint16_t readMemoryBlock(char* buf, size_t max_len)
virtual uint16_t readMemoryBlock(char* buf, int max_len)
mikee47 marked this conversation as resolved.
Show resolved Hide resolved
{
return uart_read(uart, buf, max_len);
}

virtual bool seek(int len)
{
return false;
}

virtual bool isFinished()
{
return false;
}

/** @brief Read a character from serial port without removing from input buffer
* @retval int Character read from serial port or -1 if buffer empty
* @note The character remains in serial port input buffer
6 changes: 3 additions & 3 deletions Sming/SmingCore/Network/Http/HttpRequest.cpp
Original file line number Diff line number Diff line change
@@ -28,7 +28,7 @@ HttpRequest::HttpRequest(const HttpRequest& value) : uri(value.uri)

#ifdef ENABLE_SSL
sslOptions = value.sslOptions;
sslFingerprint = value.sslFingerprint;
sslFingerprints = value.sslFingerprints;
sslKeyCertPair = value.sslKeyCertPair;
#endif
}
@@ -163,9 +163,9 @@ String HttpRequest::toString()
#ifdef ENABLE_SSL
content += F("> SSL options: ") + String(sslOptions) + '\n';
content +=
F("> SSL Cert Fingerprint Length: ") + String((sslFingerprint.certSha1 == nullptr) ? 0 : SHA1_SIZE) + '\n';
F("> SSL Cert Fingerprint Length: ") + String((sslFingerprints.certSha1 == nullptr) ? 0 : SHA1_SIZE) + '\n';
content +=
F("> SSL PK Fingerprint Length: ") + String((sslFingerprint.pkSha256 == nullptr) ? 0 : SHA256_SIZE) + '\n';
F("> SSL PK Fingerprint Length: ") + String((sslFingerprints.pkSha256 == nullptr) ? 0 : SHA256_SIZE) + '\n';
content += F("> SSL ClientCert Length: ") + String(sslKeyCertPair.getCertificateLength()) + '\n';
content += F("> SSL ClientCert PK Length: ") + String(sslKeyCertPair.getKeyLength()) + '\n';
content += '\n';
6 changes: 3 additions & 3 deletions Sming/SmingCore/Network/Http/HttpRequest.h
Original file line number Diff line number Diff line change
@@ -232,9 +232,9 @@ class HttpRequest
*
* @return bool true of success, false or failure
*/
HttpRequest* pinCertificate(const SslFingerprints& fingerprints)
HttpRequest* pinCertificate(SslFingerprints& fingerprints)
{
sslFingerprint = fingerprints;
sslFingerprints = fingerprints;
return this;
}

@@ -284,7 +284,7 @@ class HttpRequest

#ifdef ENABLE_SSL
uint32_t sslOptions = 0;
SslFingerprints sslFingerprint;
SslFingerprints sslFingerprints;
SslKeyCertPair sslKeyCertPair;
#endif

2 changes: 1 addition & 1 deletion Sming/SmingCore/Network/HttpClient.cpp
Original file line number Diff line number Diff line change
@@ -52,7 +52,7 @@ bool HttpClient::send(HttpRequest* request)
sslSessionIdPool[cacheKey] = new SslSessionId;
}
httpConnectionPool[cacheKey]->addSslOptions(request->getSslOptions());
httpConnectionPool[cacheKey]->pinCertificate(request->sslFingerprint);
httpConnectionPool[cacheKey]->pinCertificate(request->sslFingerprints);
httpConnectionPool[cacheKey]->setSslKeyCert(request->sslKeyCertPair);
httpConnectionPool[cacheKey]->sslSessionId = sslSessionIdPool[cacheKey];
}
100 changes: 96 additions & 4 deletions Sming/SmingCore/Network/Ssl/SslFingerprints.h
Original file line number Diff line number Diff line change
@@ -14,15 +14,107 @@ enum SslFingerprintType {
// Only when the private key used to generate the certificate is used then that fingerprint
};

typedef struct {
/** @brief Contains SSL fingerprint data
* @note Lifetime as follows:
* - Constructed by application, using appropriate setXXX method;
* - Passed into HttpRequest by application, using pinCertificate method - request is then queued;
* - Passed into HttpConnection (TcpClient descendant) by HttpClient, using pinCertificate method
* - When certificate validated, memory is released
*
*/
struct SslFingerprints {
uint8_t* certSha1 = nullptr; // << certificate SHA1 fingerprint
uint8_t* pkSha256 = nullptr; // << public key SHA256 fingerprint

~SslFingerprints()
{
free();
}

void free()
{
delete certSha1;
delete[] certSha1;
certSha1 = nullptr;
delete pkSha256;
delete[] pkSha256;
pkSha256 = nullptr;
}
} SslFingerprints;

bool setSha1(const uint8_t* cert, unsigned length)
{
return setValue(certSha1, SHA1_SIZE, cert, length);
}

bool setSha256(const uint8_t* cert, unsigned length)
{
return setValue(pkSha256, SHA256_SIZE, cert, length);
}

/** @brief Make copy of SHA1 certificate from data stored in flash
* @param cert
*/
bool setSha1_P(const uint8_t* cert, unsigned length)
{
// Word-aligned and sized buffers don't need special handling
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the word-aligning planned to be added here? If not then this method is identical to the non _P versions and my question is why is that needed?

Copy link
Contributor Author

@mikee47 mikee47 Feb 13, 2019

Choose a reason for hiding this comment

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

It's a matter of semantics; it also means we could add an optimisation internally which avoids copying the fingerprint data but instead takes a reference to it. I can do that if you want to stick with this model, but I'd also change to a class and make the certificate data private, which would break existing user's code...

Copy link
Contributor

Choose a reason for hiding this comment

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

but I'd also change to a class and make the certificate data private, which would break existing user's code...

Let's leave this change for now and schedule it for the next version.

return setSha1(cert, length);
}

/** @brief Make copy of SHA256 certificate from data stored in flash
* @param cert
*/
bool setSha256_P(const uint8_t* cert, unsigned length)
{
// Word-aligned and sized buffers don't need special handling
return setSha256(cert, length);
}

/** @brief Moves values out of source */
SslFingerprints& operator=(SslFingerprints& source)
{
delete[] certSha1;
mikee47 marked this conversation as resolved.
Show resolved Hide resolved
certSha1 = source.certSha1;
source.certSha1 = nullptr;

delete[] pkSha256;
pkSha256 = source.pkSha256;
source.pkSha256 = nullptr;

return *this;
}

/** @brief Make copy of values from source */
SslFingerprints& operator=(const SslFingerprints& source)
{
setSha1(source.certSha1, SHA1_SIZE);
mikee47 marked this conversation as resolved.
Show resolved Hide resolved
setSha256(source.pkSha256, SHA256_SIZE);

return *this;
}

private:
/** @brief Internal method to set a fingerprint
* @param value Reference to fingerprint value in this structure
* @param length Required length for value
* @param newValue
* @param newLength
* @retval bool true on success, false on invalid length or memory allocation failure
*/
bool setValue(uint8_t*& value, unsigned length, const uint8_t* newValue, unsigned newLength)
mikee47 marked this conversation as resolved.
Show resolved Hide resolved
{
if(newValue == nullptr || newLength == 0) {
delete[] value;
value = nullptr;
return true;
} else if(newLength != length) {
return false;
} else {
if(value == nullptr) {
value = new uint8_t[length];
if(value == nullptr) {
return false;
}
}
memcpy(value, newValue, length);
return true;
}
}
};
51 changes: 43 additions & 8 deletions Sming/SmingCore/Network/Ssl/SslValidator.cpp
Original file line number Diff line number Diff line change
@@ -8,29 +8,64 @@
*
****/

#include "SslValidator.h"

#ifdef ENABLE_SSL

#include "SslValidator.h"

bool sslValidateCertificateSha1(SSL* ssl, void* data)
{
uint8_t* hash = (uint8_t*)data;
uint8_t* hash = static_cast<uint8_t*>(data);
bool success = false;
if(hash != NULL) {
if(hash != nullptr) {
success = (ssl_match_fingerprint(ssl, hash) == 0);
delete[] hash;
}

return success;
}

bool sslValidatePublicKeySha256(SSL* ssl, void* data)
{
uint8_t* hash = (uint8_t*)data;
uint8_t* hash = static_cast<uint8_t*>(data);
bool success = false;
if(hash != NULL) {
if(hash != nullptr) {
success = (ssl_match_spki_sha256(ssl, hash) == 0);
delete[] hash;
}

return success;
}

/* SSLValidatorList */

void SSLValidatorList::clear()
{
// Ensure any remaining fingerprint data is released
for(unsigned i = 0; i < count(); ++i) {
delete[] static_cast<uint8_t*>(elementAt(i).data);
}
Vector::clear();
}

bool SSLValidatorList::validate(SSL* ssl)
{
if(count() == 0) {
// No validators specified, always succeed
debug_d("SSL Validator: list empty, allow connection");
return true;
}

// Need a match against a fingerprint
bool success = false;
for(unsigned i = 0; i < count(); i++) {
auto& validator = elementAt(i);
if(validator.callback(ssl, validator.data)) {
debug_d("SSL validator: positive match");
success = true;
break;
}
}

if(!success) {
debug_d("SSL validator: NO match");
}

return success;
26 changes: 23 additions & 3 deletions Sming/SmingCore/Network/Ssl/SslValidator.h
Original file line number Diff line number Diff line change
@@ -16,20 +16,40 @@
#ifndef _SMING_CORE_SSLVALIDATOR_H_
#define _SMING_CORE_SSLVALIDATOR_H_

#ifdef ENABLE_SSL

#include "ssl/ssl.h"
#include "ssl/tls1.h"

#include <functional>
#include "WVector.h"

typedef std::function<bool(SSL* ssl, void* data)> SslValidatorCallback;

bool sslValidateCertificateSha1(SSL* ssl, void* data);

bool sslValidatePublicKeySha256(SSL* ssl, void* data);

#endif /* ENABLE_SSL */
struct SSLValidator {
SslValidatorCallback callback;
mikee47 marked this conversation as resolved.
Show resolved Hide resolved
void* data; ///< Fingerprint
};

class SSLValidatorList : private Vector<SSLValidator>
mikee47 marked this conversation as resolved.
Show resolved Hide resolved
{
public:
~SSLValidatorList()
{
clear();
}

void add(SslValidatorCallback callback, void* data)
{
Vector::add(SSLValidator{callback, data});
}

bool validate(SSL* ssl);

void clear();
};

/** @} */
#endif /* _SMING_CORE_SSLVALIDATOR_H_ */
24 changes: 7 additions & 17 deletions Sming/SmingCore/Network/TcpClient.cpp
Original file line number Diff line number Diff line change
@@ -199,21 +199,9 @@ void TcpClient::onFinished(TcpClientState finishState)
#ifdef ENABLE_SSL
err_t TcpClient::onSslConnected(SSL* ssl)
{
bool hasSuccess = (sslValidators.count() == 0);
for(unsigned i = 0; i < sslValidators.count(); i++) {
if(sslValidators[i](ssl, sslValidatorsData[i])) {
hasSuccess = true;
break;
}
}

return hasSuccess ? ERR_OK : ERR_ABRT;
}

void TcpClient::addSslValidator(SslValidatorCallback callback, void* data)
{
sslValidators.addElement(callback);
sslValidatorsData.addElement(data);
err_t result = sslValidators.validate(ssl) ? ERR_OK : ERR_ABRT;
sslValidators.clear();
return result;
}

bool TcpClient::pinCertificate(const uint8_t* fingerprint, SslFingerprintType type)
@@ -235,20 +223,22 @@ bool TcpClient::pinCertificate(const uint8_t* fingerprint, SslFingerprintType ty
return false;
}

addSslValidator(callback, (void*)fingerprint);
addSslValidator(callback, const_cast<uint8_t*>(fingerprint));

return true;
}

bool TcpClient::pinCertificate(const SslFingerprints& fingerprints)
bool TcpClient::pinCertificate(SslFingerprints& fingerprints)
{
bool success = false;
if(fingerprints.certSha1 != nullptr) {
success = pinCertificate(fingerprints.certSha1, eSFT_CertSha1);
fingerprints.certSha1 = nullptr;
}

if(fingerprints.pkSha256 != nullptr) {
success = pinCertificate(fingerprints.pkSha256, eSFT_PkSha256);
fingerprints.pkSha256 = nullptr;
}

return success;
Loading