Improve memory management of SSL fingerprint data (#1618)

* Improve memory management of SSL fingerprint data.

Add methods to SSLFingerprints structure to manage allocation/de-allocation of fingerprint data.
Create `SSLValidatorList` class for `TCPClient` and move relevant code into methods.

Take care to set `SSLFingerprints` members to `nullptr` when ownership relinquished (see `TcpClient::pinCertificate()`)

bugfix: In `TcpClient::onSslConnected`, the first matching validator causes checking to stop, which means the validator data for any subsequent fingerprints never gets released. This is fixed in the `SSLValidatorList` destructor.

* Revise `Basic_Ssl` sample to use different web-page with fingerprint that doesn't change.

Add length checks to SslFingerprints.
Also change `HardwareSerial` to base on `ReadWriteStream` instead of `Stream`, allows incoming data to be streamed direct.

* Review changes

* Additional checks in `SslFingerprints` assignment operators
* Output debug error if `SslFingerprint::setXXX` fails
* Update fingerprints in `HttpClient` sample and enable checking

* Move `pinCertificate` code into SslValidator

Validator callback still responsible for de-allocating memory, but can be called with ssl = nullptr

* Rename `SSLValidatorList` -> `SslValidatorList`, `SSLValidator` -> `SslValidator`

* Accept invalid length of fingerprint to ensure validation fails

Move SslFingerprint code out of header
Add check for flash memory in `SslFingerprint::setValue`

Sming/SmingCore/Data/Stream/DataSourceStream.h

@@ -44,7 +44,10 @@ class IDataSourceStream : public Stream
      *  @retval StreamType The stream type.
      *  @todo   Return value of IDataSourceStream:getStreamType base class function should be of type StreamType, e.g. eSST_User
      */
-	virtual StreamType getStreamType() const = 0;
+	virtual StreamType getStreamType() const
+	{
+		return eSST_Unknown;
+	}
 
 	/** @brief Determine if the stream object contains valid data
 	 *  @retval bool true if valid, false if invalid

Sming/SmingCore/HardwareSerial.h

@@ -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
@@ -239,11 +239,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)
 	{
 		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

Sming/SmingCore/Network/Http/HttpRequest.cpp

@@ -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';

Sming/SmingCore/Network/Http/HttpRequest.h

@@ -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
 

Sming/SmingCore/Network/HttpClient.cpp

@@ -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];
 	}

Sming/SmingCore/Network/Ssl/SslFingerprints.cpp

@@ -0,0 +1,87 @@
+/****
+ * Sming Framework Project - Open Source framework for high efficiency native ESP8266 development.
+ * Created 2015 by Skurydin Alexey
+ * http://github.com/anakod/Sming
+ * All files of the Sming Core are provided under the LGPL v3 license.
+ ****/
+
+#ifdef ENABLE_SSL
+
+#include "SslFingerprints.h"
+#include <user_config.h>
+#include "flashmem.h"
+
+static inline bool isFlashPtr(const uint8_t* ptr)
+{
+	auto addr = reinterpret_cast<uint32_t>(ptr);
+	return addr >= INTERNAL_FLASH_START_ADDRESS;
+}
+
+static inline void freeValue(const uint8_t*& ptr)
+{
+	delete[] ptr;
+	ptr = nullptr;
+}
+
+void SslFingerprints::free()
+{
+	freeValue(certSha1);
+	freeValue(pkSha256);
+}
+
+bool SslFingerprints::setValue(const uint8_t*& value, unsigned requiredLength, const uint8_t* newValue,
+							   unsigned newLength)
+{
+	if(newValue == nullptr || newLength == 0) {
+		freeValue(value);
+		return true;
+	} else {
+		if(newLength != requiredLength) {
+			debug_w("Warning: Invalid fingerprint length");
+			// Copy data anyway to prevent false positive validation
+		}
+		if(value == nullptr) {
+			value = new uint8_t[requiredLength];
+			if(value == nullptr) {
+				return false;
+			}
+		}
+		// If new value is longer than buffer, copy short
+		unsigned length = std::min(newLength, requiredLength);
+		// Behave properly when source is flash memory and length is wrong or buffers misaligned
+		if(isFlashPtr(newValue)) {
+			memcpy_P(const_cast<uint8_t*>(value), newValue, length);
+		} else {
+			memcpy(const_cast<uint8_t*>(value), newValue, length);
+		}
+		return true;
+	}
+}
+
+SslFingerprints& SslFingerprints::operator=(SslFingerprints& source)
+{
+	if(this != &source) {
+		freeValue(certSha1);
+		certSha1 = source.certSha1;
+		source.certSha1 = nullptr;
+
+		freeValue(pkSha256);
+		pkSha256 = source.pkSha256;
+		source.pkSha256 = nullptr;
+	}
+
+	return *this;
+}
+
+/** @brief Make copy of values from source */
+SslFingerprints& SslFingerprints::operator=(const SslFingerprints& source)
+{
+	if(this != &source) {
+		setSha1(source.certSha1, SHA1_SIZE);
+		setSha256(source.pkSha256, SHA256_SIZE);
+	}
+
+	return *this;
+}
+
+#endif // ENABLE_SSL

Sming/SmingCore/Network/Ssl/SslFingerprints.h

@@ -5,24 +5,96 @@
  * All files of the Sming Core are provided under the LGPL v3 license.
  ****/
 
+#ifndef SMINGCORE_NETWORK_SSLFINGERPRINTS_H_
+#define SMINGCORE_NETWORK_SSLFINGERPRINTS_H_
+
 #include "ssl/ssl.h"
 
+/**
+ * @brief SSL Certificate fingerprint type
+ */
 enum SslFingerprintType {
-	eSFT_CertSha1 = 0, // << Fingerprint based on the SHA1 value of the certificate.
-					   //     Every time a certificate is renewed this value will change.
-	eSFT_PkSha256,	 // << Fingerprint based on the SHA256 value of the public key subject in the certificate.
-					   //    Only when the private key used to generate the certificate is used then that fingerprint
+	/* The SHA1 hash of the entire certificate. This changes on each certificate renewal so needs
+	 * to be updated every time the remote server updates its certificate.
+	 * Advantages: Takes less time to verify than SHA256
+	 * Disadvantages: Likely to change periodically
+	 */
+	eSFT_CertSha1 = 0, ///< Fingerprint based on the SHA1 value of the certificate
+
+	/* For HTTP public key pinning (RFC7469), the SHA-256 hash of the Subject Public Key Info
+	 * (which usually only changes when the public key changes) is used.
+	 * Advantages: Doesn't change frequently
+	 * Disadvantages: Takes more time (in ms) to verify.
+	 */
+	eSFT_PkSha256, // << Fingerprint based on the SHA256 value of the Public Key Subject in the certificate
 };
 
-typedef struct {
-	uint8_t* certSha1 = nullptr; // << certificate SHA1 fingerprint
-	uint8_t* pkSha256 = nullptr; // << public key SHA256 fingerprint
+/** @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 {
+	const uint8_t* certSha1 = nullptr; // << certificate SHA1 fingerprint
+	const uint8_t* pkSha256 = nullptr; // << public key SHA256 fingerprint
+
+	~SslFingerprints()
+	{
+		free();
+	}
+
+	void free();
+
+	/** @brief Set the SHA1 fingerprint
+	 *  @param cert data to copy
+	 *  @param length for checking
+	 *  @retval bool false on length check or allocation failure
+	 */
+	bool setSha1(const uint8_t* cert, unsigned length)
+	{
+		return setValue(certSha1, SHA1_SIZE, cert, length);
+	}
+
+	/** @brief Make copy of SHA1 certificate from data stored in flash */
+	bool setSha1_P(const uint8_t* cert, unsigned length)
+	{
+		return setValue(certSha1, SHA1_SIZE, cert, length);
+	}
+
+	/** @brief Set the SHA256 fingerprint
+	 *  @param cert data to copy
+	 *  @param length for checking
+	 *  @retval bool false on length check or allocation failure
+	 */
+	bool setSha256(const uint8_t* cert, unsigned length)
+	{
+		return setValue(pkSha256, SHA256_SIZE, cert, length);
+	}
 
-	void free()
+	/** @brief Make copy of SHA256 certificate from data stored in flash */
+	bool setSha256_P(const uint8_t* cert, unsigned length)
 	{
-		delete certSha1;
-		certSha1 = nullptr;
-		delete pkSha256;
-		pkSha256 = nullptr;
+		return setValue(pkSha256, SHA256_SIZE, cert, length);
 	}
-} SslFingerprints;
+
+	/** @brief Moves values out of source */
+	SslFingerprints& operator=(SslFingerprints& source);
+
+	/** @brief Make copy of values from source */
+	SslFingerprints& operator=(const SslFingerprints& source);
+
+private:
+	/** @brief Internal method to set a fingerprint
+	 *  @param value Reference to fingerprint value in this structure
+	 *  @param requiredLength Expected length for value
+	 *  @param newValue
+	 *  @param newLength
+	 *  @retval bool true on success, false on invalid length or memory allocation failure
+	 */
+	bool setValue(const uint8_t*& value, unsigned requiredLength, const uint8_t* newValue, unsigned newLength);
+};
+
+#endif // SMINGCORE_NETWORK_SSLFINGERPRINTS_H_