Improve memory management of SSL fingerprint data #1618

mikee47 · 2019-02-12T16:32:37Z

Uses may now use methods of SslLFingerprints to provide certificate data. I've updated the Basic_Ssl and HttpClient sample applications accordingly.

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 - previous code TcpClient::pinCertificate().
bugfix: In TcpClient::onSslConnected, the first matching validator causes checking to stop, which means the validator dataj for any subsequent fingerprints never gets released. This is fixed in the new SslValidatorList implementation.
Revise Basic_Ssl sample to use different web-page with fingerprint that doesn't change.
Also change HardwareSerial to base on ReadWriteStream instead of Stream, allows incoming data to be streamed direct.

mikee47 · 2019-02-12T17:30:40Z

This is more-or-less just be a re-factoring of existing code without changing the validation mechanism. However, it feels to me like an overly complex way to crack what should be a fairly simple problem.

The user's application passes their fingerprints to HttpRequest, which keeps them as an SslFingerprints structure. They are then passed into TcpClient, which pulls the values out into a hashmap. Validation eventually happens in TcpClient::onSslConnected. This is essentially a 'one-time fingerprint cache'; the mechanism could be more simply implemented wrapping everything in the SslValdation class, then passing that around.

Alternatively, we could bypass all of this by just using a callback (to the user's application instead...

Thoughts / comments?

Sming/SmingCore/HardwareSerial.h

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 dataj for any subsequent fingerprints never gets released. This is fixed in the `SSLValidatorList` destructor.

…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.

mikee47 · 2019-02-13T07:59:46Z

@slaff As I've started using __attribute__((deprecated)) I can prepare a PR for all the remaining items which only have @deprecated comment.

Another similar general change was use of override.

Both of these I think should be introduced sooner rather than later...

Sming/SmingCore/Network/Ssl/SslFingerprints.h

Sming/SmingCore/Network/Ssl/SslValidator.h

slaff · 2019-02-13T08:53:52Z

@slaff As I've started using __attribute__((deprecated)) I can prepare a PR for all the remaining items which only have @deprecated comment.
Another similar general change was use of override.

Yes, please. Make sure also to modify the https://github.com/SmingHub/Sming/wiki/Coding-Style-Rules wiki page to reflect those changes.

slaff · 2019-02-13T09:06:34Z

samples/HttpClient/app/application.cpp

-	//	request->->pinCertificate(sha1Fingerprint, eSFT_CertSha1)
+	fingerprints.setSha1_P(sha1Fingerprint, sizeof(sha1Fingerprint));
+
+	// request->pinCertificate(fingerprints);


Maybe I am missing something but where are the fingerprints attached to the request?

In the commented-out line - so behaviour isn't changed. Can un-comment if you prefer, or re-comment fingerprints.setXXX lines?

Uncomment and change the comments text if needed.

OK, I'll also update the fingerprints (they've changed)

slaff · 2019-02-13T09:07:38Z

Sming/SmingCore/Network/TcpClient.h

 	 * @param	fingerprints - passes the certificate fingerprints by reference.
 	 *
-	 * @retval	bool  true of success, false or failure
+	 * @retval bool  true of success, false or failure


Can you change this also to 'bool true on success, false on failure' ?

Sming/SmingCore/Network/Ssl/SslValidator.h

slaff · 2019-02-13T09:13:45Z

Sming/SmingCore/Network/Ssl/SslFingerprints.h

+	 */
+	bool setSha1_P(const uint8_t* cert, unsigned length)
+	{
+		// Word-aligned and sized buffers don't need special handling


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?

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

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.

slaff · 2019-02-13T09:17:53Z

Sming/SmingCore/Data/Stream/DataSourceStream.h

@@ -19,6 +19,7 @@
 enum StreamType {
 	eSST_Invalid,	///< Stream content not valid
 	eSST_Memory,	 ///< Memory data stream
+	eSST_Serial,	 ///< Serial port


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

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.

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.

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

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

…slValidator`

Sming/SmingCore/Network/Ssl/SslFingerprints.h

Sming/SmingCore/Network/Ssl/SslValidator.h

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

mikee47 · 2019-02-13T21:45:42Z

OK, I've tested this last commit with various dodgy keys and behaves predictably. Users should be able to throw any old crap in there and it'll play nice.

* SmingHub#1618 (comment) * Change `ensureCapacity` to adjust new capacity by `increment`, thus simplifying code which uses it * Add `bool` return value to: `addElement()` - so that `add()` return value isn't meaningless `ensureCapacity()` `setSize()` `insertElementAt()` `setElementAt()`

* Add additional checks to `Vector` class methods * #1618 (comment) * Change `ensureCapacity` to adjust new capacity by `increment`, thus simplifying code which uses it * Add `bool` return value to: `addElement()` - so that `add()` return value isn't meaningless `ensureCapacity()` `setSize()` `insertElementAt()` `setElementAt()` * Apply coding style and fix use of parentheses

mikee47 commented Feb 12, 2019

View reviewed changes

Sming/SmingCore/HardwareSerial.h Show resolved Hide resolved

mikee47 added 2 commits February 12, 2019 18:09

Revise Basic_Ssl sample to use different web-page with fingerprint …

6d992fe

…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.

mikee47 force-pushed the fix/SSL_Fingerprints branch from 5c5d885 to 6d992fe Compare February 12, 2019 18:14

mikee47 commented Feb 13, 2019

View reviewed changes

slaff reviewed Feb 13, 2019

View reviewed changes

slaff added this to the 3.7.2 milestone Feb 13, 2019

slaff added the 3 - Review label Feb 13, 2019

mikee47 added 2 commits February 13, 2019 12:23

Review changes

9989ef9

* 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

16732c4

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

mikee47 force-pushed the fix/SSL_Fingerprints branch 4 times, most recently from 44f5848 to 16732c4 Compare February 13, 2019 14:15

Rename SSLValidatorList -> SslValidatorList, SSLValidator -> `S…

fd0f801

…slValidator`

mikee47 commented Feb 13, 2019

View reviewed changes

Sming/SmingCore/Network/Ssl/SslFingerprints.h Outdated Show resolved Hide resolved

mikee47 commented Feb 13, 2019

View reviewed changes

Sming/SmingCore/Network/Ssl/SslValidator.h Show resolved Hide resolved

Accept invalid length of fingerprint to ensure validation fails

ed1ce28

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

mikee47 force-pushed the fix/SSL_Fingerprints branch from 33e3780 to ed1ce28 Compare February 13, 2019 21:45

slaff removed the 3 - Review label Feb 14, 2019

slaff merged commit cbed639 into SmingHub:develop Feb 14, 2019

mikee47 deleted the fix/SSL_Fingerprints branch February 14, 2019 14:03

slaff mentioned this pull request Feb 27, 2019

Preparation for Release 3.8.0 #1581

Closed

4 tasks

mikee47 mentioned this pull request Jun 12, 2019

Add additional checks to Vector class methods #1721

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve memory management of SSL fingerprint data #1618

Improve memory management of SSL fingerprint data #1618

mikee47 commented Feb 12, 2019 •

edited

Loading

mikee47 commented Feb 12, 2019

mikee47 commented Feb 13, 2019 •

edited

Loading

slaff commented Feb 13, 2019

slaff Feb 13, 2019

mikee47 Feb 13, 2019

slaff Feb 13, 2019

mikee47 Feb 13, 2019

slaff Feb 13, 2019

slaff Feb 13, 2019

mikee47 Feb 13, 2019 •

edited

Loading

slaff Feb 13, 2019

slaff Feb 13, 2019

mikee47 Feb 13, 2019 •

edited

Loading

slaff Feb 13, 2019

mikee47 commented Feb 13, 2019

Improve memory management of SSL fingerprint data #1618

Improve memory management of SSL fingerprint data #1618

Conversation

mikee47 commented Feb 12, 2019 • edited Loading

mikee47 commented Feb 12, 2019

mikee47 commented Feb 13, 2019 • edited Loading

slaff commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikee47 Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikee47 Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikee47 commented Feb 13, 2019

mikee47 commented Feb 12, 2019 •

edited

Loading

mikee47 commented Feb 13, 2019 •

edited

Loading

mikee47 Feb 13, 2019 •

edited

Loading

mikee47 Feb 13, 2019 •

edited

Loading