BearSSL client: warn user on misconfiguration, allow flash string for fingerprint #4833

d-a-v · 2018-06-21T09:25:49Z

igrr · 2018-06-21T09:58:15Z

libraries/ESP8266WiFi/examples/HTTPSRequest/HTTPSRequest.ino

@@ -19,6 +19,8 @@
 #include <ESP8266WiFi.h>
 #include <WiFiClientSecure.h>

+#define DEPRECATED_SSL_WITH_AXTLS 0  // use 1 for axTLS, 0 for BearSSL


I think if you are making BearSLL the default here, you may also just drop axTLS part of the example entirely. After all, the intent is that axTLS is still supported (for existing sketches), but all new users should try go with BearSSL.

True. What about deferring that to when namespace is by default switched to BearSSL. We will have to revisit all examples and also remove these now useless / misleading verify()/verifyCertChain() api methods.

I guarantee there are bugs in both the BearSSL wrapper and library, so let's hold off talk of deprecating things for a bit, please. Maybe a simple debug message in the BearSSL::verify() method would cover the case discussed here?

devyte · 2018-07-03T03:23:06Z

libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL-fp.cpp

+    return c >= '0' && c <= '9'? c - '0'
+         : c >= 'A' && c <= 'F'? c - 'A' + 10
+         : c >= 'a' && c <= 'f'? c - 'a' + 10
+         : 255;


If this 255 is an error condition....

Actually, given the way this is written 255 is not an error but could be a space (expected to occur every other iteration in the while() lop in the setFP function) or an error.

255 means no hex number. 255 is a separator detector.

In this code 255 doesn't mean "space", it means "none of the above".
My comment is aimed at: what happens if the fingerprint string has a typo, i.e.: an invalid char. The above check is strict in that it checks for specific chars that are allowed. If there is a separator allowed, then maybe that should be explicitly allowed as well? Then, in case of an invalid char, an error is returned, which gets checked below.

devyte · 2018-07-03T03:26:54Z

libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp

@@ -768,6 +768,11 @@ bool WiFiClientSecure::_installClientX509Validator() {
 bool WiFiClientSecure::_connectSSL(const char* hostName) {
  _freeSSL();
  _oom_err = false;
+#ifdef DEBUG_ESP_SSL
+  if (!_use_insecure && !_use_fingerprint && !_use_self_signed && !_knownkey && !_sk) {


What happens with this condition in the normal use case, i.e.: a certificate signed by an authority, which has been verified?

It'll fail. Needs to check that there is no CertStore and no CA list in the object as well.

devyte · 2018-07-03T03:29:06Z

libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h

@@ -104,9 +105,14 @@ class WiFiClientSecure : public WiFiClient {
    static bool probeMaxFragmentLength(const char *hostname, uint16_t port, uint16_t len);
    static bool probeMaxFragmentLength(const String host, uint16_t port, uint16_t len);

-    // AXTLS compatbile wrappers
-    bool verify(const char* fingerprint, const char* domain_name) { (void) fingerprint; (void) domain_name; return false; } // Can't handle this case, need app code changes
+    // AXTLS compatible wrappers
    bool verifyCertChain(const char* domain_name) { (void)domain_name; return connected(); } // If we're connected, the cert passed validation during handshake


Does it make sense to verify a certificate chain before connection? How does verification work with axTLS?
My question is aimed at whether a connection should be initiated here in case it is not established.

No, it doesn't make sense before a connection. You get a cert chain as part of a connection handshake. In axTLS this is just stuffed away and not examined unless you call .verify() at some later point. In BearSSL it's verified as part of the connection setup and if it fails there is no connection. If you connected, verified, then disconnected, there is no guarantee you'd connect to the same server on the next real .connect().

The only reason verify() was there in the initial BearSSL was for compatibility with axTLS code (at least compilation...it would fail when run if there was no prior setFingerprint() type call. If you want to rename it to something else, I'd say drop it as it's always going to be a no-op in BearSSL. Suggest dropping this or using original method code with a return connected() instead of return false.

@earlephilhower I'm a bit confused. My question was for verifyCertChain(), which already returns connected().

earlephilhower

earlephilhower · 2018-07-03T05:25:22Z

libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL-fp.cpp

@@ -0,0 +1,36 @@
+


Can we please just put these couple methods inside the main WiFiClientSecureBearSSL.cpp file? I don't see a good reason for these to live outside that file.

earlephilhower · 2018-07-03T05:27:09Z

libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL-fp.cpp

+         : 255;
+}
+
+void WiFiClientSecure::setFingerprint(const String& fingerprint) {


Does a C string ("12 34 56 ...") silently promote to a String&? Maybe using a char * would be better WRT memory.

Yes. The advantage of const String& is that it will take a progmem or ram char*. It will be released right after setFingerprint(fp); which does not any allocation, so ram won't be holed.

I think that in this case a C string promotes to a temp String object, which is then passed in by reference.

earlephilhower · 2018-07-03T05:31:08Z

libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL-fp.cpp

+    int idx = 0;
+    const char* _fingerprint = fingerprint.c_str();
+
+    while (idx < 20 && (c = *_fingerprint++) && (d = *_fingerprint++)) {


Assuming we're sscanf'ing manually here, can we use c=pgm_read_byte(_fingerprint++) && d=pgm_read_byte... instead? Everything else in BearSSL silently "just works" with PROGMEM strings/arrays (except for write() which I really should have just use memcpy_p and be done with it).

I don't think we can always assume fingerprint is in progmem. We would have to have two functions{,_P}. String is already here, takes care of progmem, and is released when we are done.

As below, no assumption is needed. RAM and flash can be read w/pgm_read*. It's slower than plain pointer reads, true, but when compared to the actual SSL connection it's absolutely not noticeable (and this is only run one time before connection, not in a loop for every step of the process).

earlephilhower · 2018-07-03T05:35:03Z

libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL-fp.cpp

+    return c >= '0' && c <= '9'? c - '0'
+         : c >= 'A' && c <= 'F'? c - 'A' + 10
+         : c >= 'a' && c <= 'f'? c - 'a' + 10
+         : 255;


Actually, given the way this is written 255 is not an error but could be a space (expected to occur every other iteration in the while() lop in the setFP function) or an error.

earlephilhower · 2018-07-03T05:37:17Z

libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp

@@ -768,6 +768,11 @@ bool WiFiClientSecure::_installClientX509Validator() {
 bool WiFiClientSecure::_connectSSL(const char* hostName) {
  _freeSSL();
  _oom_err = false;
+#ifdef DEBUG_ESP_SSL
+  if (!_use_insecure && !_use_fingerprint && !_use_self_signed && !_knownkey && !_sk) {


It'll fail. Needs to check that there is no CertStore and no CA list in the object as well.

earlephilhower · 2018-07-03T05:46:12Z

libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL-fp.cpp

+    while (idx < 20 && (c = *_fingerprint++) && (d = *_fingerprint++)) {
+        c = htoi(c);
+        d = htoi(d);
+        if (c > 15 || d > 15) {


Suggest the following to be more pedantic about verification (forgive no indenting, doing this in the review window in proportional font):

while (idx < 20 && (c = pgm_read_byte(_fingerprint++)) && (d = pgm_read_byte(_fingerprint++))) { c = htoi(c); d = htoi(d); // Skip 0 or more spaces for all but last entry (could drop while and only check for 1 ' ' as well) while ((idx < 19) && pgm_read_byte(_fingerprint) && (pgm_read_byte(_fingerprint)==' ')) { _fingerprint++; } if ((c>15) || (d>15)) { return false; } fp[idx++] = (c<<4)|d; } if ((idx != 20) || pgm_read_byte(_fingerprint)) { return false; } // Some garbage left over at end of line return setFingerprint(fp);

With this code fingerprint must be in PROGMEM right ?

Actually, no. That would be true on the AVR which is Harvard architecture (completely separate program and RAM) and you need special instructions to read flash. ESP8266 has von Neumann, unified address space, but has alignment requirements in certain regions.

pgm_read_*() is always safe and just ensures reads are done on a 32-bit boundary (by shifting/masking after the fact).

Well, maybe I was too influenced by AVR. Also confused by __FlashStringHelper and FPSTR that only String can understand. Thanks for the details!
Then in the general case when performance is not in the loop we can always use pgm_*.
I should have read memcpy_P()'s code before :)

earlephilhower · 2018-07-03T05:48:05Z

libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h

@@ -67,6 +67,7 @@ class WiFiClientSecure : public WiFiClient {
      _knownkey_usages = usages;
    }
    // Only check SHA1 fingerprint of certificate
+    void setFingerprint(const String& fingerprint);


Same comment as above about making this eat char * instead. Strings on this machine are evil and beat up the heap like mad. IMO they need to be rewritten to use handles like old MacOS to be easier w/o MMU, but that's another can of worms.

earlephilhower · 2018-07-03T05:50:15Z

libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h

@@ -208,6 +214,8 @@ class WiFiClientSecure : public WiFiClient {
    static std::shared_ptr<uint8_t> _bearssl_stack;
    // The local copy, only used to enable a reference count
    std::shared_ptr<uint8_t> _local_bearssl_stack;
+
+    void WiFiClientSecure_verify__is_unavailable_with_BearSSL__use_setFingerprint_instead (void);


Interesting way to cause an error on compile.

d-a-v · 2018-07-03T10:18:07Z

earlephilhower · 2018-07-03T14:18:42Z

d-a-v · 2018-07-03T14:57:07Z

@d-a-v

@d-a-v

earlephilhower · 2018-10-03T21:29:47Z

@d-a-v

@d-a-v

BearSSL client: warn user on misconfiguration, allow flash string for…

09cdacd

… fingerprint +update HTTPSRequest.ino example

d-a-v requested review from earlephilhower, igrr and devyte June 21, 2018 09:25

igrr approved these changes Jun 21, 2018

View reviewed changes

devyte reviewed Jul 3, 2018

View reviewed changes

earlephilhower reviewed Jul 3, 2018

View reviewed changes

earlephilhower added 2 commits July 16, 2018 11:19

Merge branch 'master' into bearssl-client-update+example

a2d6c2c

Merge branch 'master' into bearssl-client-update+example

34b55e8

earlephilhower self-assigned this Oct 3, 2018

earlephilhower mentioned this pull request Oct 3, 2018

Add BearSSL::setFingerprint(char* fp) #5204

Merged

earlephilhower mentioned this pull request Oct 3, 2018

Add warning when no authenticator, drop verify() #5205

Merged

earlephilhower closed this Oct 3, 2018

BearSSL client: warn user on misconfiguration, allow flash string for fingerprint #4833

BearSSL client: warn user on misconfiguration, allow flash string for fingerprint #4833

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!