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

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

+update HTTPSRequest.ino example

edit:

I'm not sure we should keep BearSSL's ::verify(fingerprint,host).

using it was always failing almost silently
now does not build with a linker error
since the API must change anyway, then why not removing it ?

… fingerprint +update HTTPSRequest.ino example

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:24:01Z

libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL-fp.cpp

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


... then the return should likely be checked here, in case the user-provided fingerprint is bad. Also, the return for setFingerprint could be a bool to indicate success/failure.

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

In the right direction, a few things needed IMO to clean it up.

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

The purpose of this PR was to "shoot in the can of worm" :)

axTLS and BearSSL's api are not the same.

What I suggest is:

Keep axTLS for 2.4.2 (that was acquired, we all agree)
BearSSL shall not try to be compatible with axTLS
All examples using axTLS should be updated to work with both libs (like I tried to show in this PR)
BearSSL by default should be used.
fingerprint presented as a string is as easy to use as copy-paste is, and this may be proposed to the users (like with axTLS. with PROGMEM if that's cheap)
BearSSL should be verbosely failing in debug mode when misconfigured

Since @earlephilhower is going to update BearSSL in #4882, I'm OK to close this PR and let his magic do the job. I can take care of the examples.ino.

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

Agree w/ you, @d-a-v, except for

BearSSL should not try to be compatible w/axTLS

I'm fine with doing non-heroic things (already implemented easy ones), so maybe we can go for "should try to be compatible, except when it becomes too onerous?"

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

maybe we can go for "should try to be compatible, except when it becomes too onerous?"

Agreed !

@d-a-v

Add a method allowing a user to send in a character string for the fingerprint, like axTLS supported. Implements part of PR esp8266#4833 from @d-a-v with changes requested in discussion.

@d-a-v

Print a warning when in debug mode when a BearSSL connection tries to connect without having any defined authentication methods, since it will fail. Completely remove the empty axTLS compatibilty method "::verify(char *fp, char *name)" because it can't be done w/BearSSL w/o code changes, and always failed. Better to have a compile failure when we know at compile time the app won't do what is expected. Completes the changes started by @d-a-v in PR esp8266#4833

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

Closing this in lieu of #5204 and #5205 to take some of the load off of @d-a-v.

@d-a-v

Add a method allowing a user to send in a character string for the fingerprint, like axTLS supported. Implements part of PR esp8266#4833 from @d-a-v with changes requested in discussion.

@d-a-v

Print a warning when in debug mode when a BearSSL connection tries to connect without having any defined authentication methods, since it will fail. Completely remove the empty axTLS compatibilty method "::verify(char *fp, char *name)" because it can't be done w/BearSSL w/o user code changes, and always failed. Better to have a compile failure when we know at compile time the app won't do what is expected. Completes the changes started by @d-a-v in PR #4833

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

d-a-v commented Jun 21, 2018 •

edited

Loading

igrr Jun 21, 2018

d-a-v Jun 21, 2018

earlephilhower Jul 3, 2018

devyte Jul 3, 2018

earlephilhower Jul 3, 2018

d-a-v Jul 3, 2018

devyte Jul 4, 2018

devyte Jul 3, 2018

devyte Jul 3, 2018

earlephilhower Jul 3, 2018

devyte Jul 3, 2018

earlephilhower Jul 3, 2018

devyte Jul 4, 2018

earlephilhower left a comment

earlephilhower Jul 3, 2018

earlephilhower Jul 3, 2018

d-a-v Jul 3, 2018

devyte Jul 4, 2018

earlephilhower Jul 3, 2018

d-a-v Jul 3, 2018

earlephilhower Jul 3, 2018

earlephilhower Jul 3, 2018

earlephilhower Jul 3, 2018

earlephilhower Jul 3, 2018

d-a-v Jul 3, 2018

earlephilhower Jul 3, 2018

d-a-v Jul 3, 2018

earlephilhower Jul 3, 2018

earlephilhower Jul 3, 2018

d-a-v commented Jul 3, 2018

earlephilhower commented Jul 3, 2018 •

edited

Loading

d-a-v commented Jul 3, 2018

earlephilhower commented 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

Conversation

d-a-v commented Jun 21, 2018 • edited Loading

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

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

Choose a reason for hiding this comment

earlephilhower left a 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

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

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

d-a-v commented Jul 3, 2018

earlephilhower commented Jul 3, 2018 • edited Loading

d-a-v commented Jul 3, 2018

earlephilhower commented Oct 3, 2018

d-a-v commented Jun 21, 2018 •

edited

Loading

earlephilhower commented Jul 3, 2018 •

edited

Loading