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.

Sign up for GitHub

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

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
09cdacd
BearSSL client: warn user on misconfiguration, allow flash string for…
d-a-v Jun 21, 2018
a2d6c2c
Merge branch 'master' into bearssl-client-update+example
earlephilhower Jul 16, 2018
34b55e8
Merge branch 'master' into bearssl-client-update+example
earlephilhower Oct 3, 2018
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
46 changes: 45 additions & 1 deletion libraries/ESP8266WiFi/examples/HTTPSRequest/HTTPSRequest.ino
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
#include <ESP8266WiFi.h>
#include <WiFiClientSecure.h>

#define DEPRECATED_SSL_WITH_AXTLS 0 // use 1 for axTLS, 0 for BearSSL
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?


const char* ssid = "........";
const char* password = "........";

Expand All @@ -27,8 +29,23 @@ const int httpsPort = 443;

// Use web browser to view and copy
// SHA1 fingerprint of the certificate

#if DEPRECATED_SSL_WITH_AXTLS

// no separator, or space, or ":", in iram:
const char* fingerprint = "5F F1 60 31 09 04 3E F2 90 D2 B0 8A 50 38 04 E8 37 9F BC 76";

#else // with BearSSL

// compatible with axTLS:
//const char* fingerprint = "35 85 74 EF 67 35 A7 CE 40 69 50 F3 C0 F6 80 CF 80 3B 2E 19";
// flash is allowed:
//#define fingerprint FPSTR("358574EF67:35:A7:CE:40:69:50:F3-C0-F6-80-CF-80-3B 2E 19")
// or an array in iram:
const uint8_t fingerprint [] = { 0x35, 0x85, 0x74, 0xEF, 0x67, 0x35, 0xA7, 0xCE, 0x40, 0x69, 0x50, 0xF3, 0xC0, 0xF6, 0x80, 0xCF, 0x80, 0x3B, 0x2E, 0x19 };

#endif

void setup() {
Serial.begin(115200);
Serial.println();
Expand All @@ -46,9 +63,13 @@ void setup() {
Serial.println(WiFi.localIP());

// Use WiFiClientSecure class to create TLS connection
WiFiClientSecure client;
Serial.print("connecting to ");
Serial.println(host);

////////////////////////// axTLS (soon deprecated)
#if DEPRECATED_SSL_WITH_AXTLS

axTLS::WiFiClientSecure client;
if (!client.connect(host, httpsPort)) {
Serial.println("connection failed");
return;
Expand All @@ -60,6 +81,29 @@ void setup() {
Serial.println("certificate doesn't match");
}

////////////////////////// BearSSL
#else // new SSL api with BearSSL

BearSSL::WiFiClientSecure client;

// WARNING:
// - axTLS was insecure by default,
// and fingerprint could be checked after a successful connection
// - BearSSL is not:
// "If there are no CAs or insecure options specified, BearSSL will not connect."
// Check the BearSSL_Validation.ino example for more details
// Insecure connection is available with `client.setInsecure()`
// Semi-secure checking using fingerprint:
client.setFingerprint(fingerprint);

if (!client.connect(host, httpsPort)) {
Serial.println("connection failed");
return;
}

#endif
//////////////////////////

String url = "/repos/esp8266/Arduino/commits/master/status";
Serial.print("requesting URL: ");
Serial.println(url);
Expand Down
36 changes: 36 additions & 0 deletions libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL-fp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

#include <WiFiClientSecureBearSSL.h>

namespace BearSSL {

static uint8_t htoi (unsigned char c)
{
return c >= '0' && c <= '9'? c - '0'
: c >= 'A' && c <= 'F'? c - 'A' + 10
: c >= 'a' && c <= 'f'? c - 'a' + 10
: 255;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this 255 is an error condition....

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}

void WiFiClientSecure::setFingerprint(const String& fingerprint) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


uint8_t fp [20];
uint8_t c, d;
int idx = 0;
const char* _fingerprint = fingerprint.c_str();

while (idx < 20 && (c = *_fingerprint++) && (d = *_fingerprint++)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

c = htoi(c);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

d = htoi(d);
if (c > 15 || d > 15) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With this code fingerprint must be in PROGMEM right ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 :)

// skip separator
_fingerprint--;
continue;
}
fp[idx++] = (c << 4) + d;
}

if (idx == 20)
setFingerprint(fp);
}

} // namespace
5 changes: 5 additions & 0 deletions libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,6 +846,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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

DEBUG_ESP_PORT.println(FPSTR("BearSSL: connection *will* fail, no authentication method is setup"));
}
#endif

_sc = std::make_shared<br_ssl_client_context>();
_eng = &_sc->eng; // Allocation/deallocation taken care of by the _sc shared_ptr
Expand Down
10 changes: 9 additions & 1 deletion libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ class WiFiClientSecure : public WiFiClient {
_knownkey_usages = usages;
}
// Only check SHA1 fingerprint of certificate
void setFingerprint(const String& fingerprint);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

void setFingerprint(const uint8_t fingerprint[20]) {
_clearAuthenticationSettings();
_use_fingerprint = true;
Expand Down Expand Up @@ -120,8 +121,13 @@ class WiFiClientSecure : public WiFiClient {
static bool probeMaxFragmentLength(const String host, uint16_t port, uint16_t len);

// AXTLS compatible 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
bool verifyCertChain(const char* domain_name) { (void)domain_name; return connected(); } // If we're connected, the cert passed validation during handshake
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

bool verify(const String& fingerprint, const String& domain_name) {
(void) fingerprint;
(void) domain_name;
WiFiClientSecure_verify__is_unavailable_with_BearSSL__use_setFingerprint_instead();
return false;
}

bool setCACert(const uint8_t* pk, size_t size);
bool setCertificate(const uint8_t* pk, size_t size);
Expand Down Expand Up @@ -232,6 +238,8 @@ class WiFiClientSecure : public WiFiClient {
void _ensureStackAvailable(); // Allocate the stack if necessary
// 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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting way to cause an error on compile.

};

};
Expand Down