-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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.
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this 255 is an error condition.... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 255 means no hex number. 255 is a separator detector. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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". |
||
} | ||
|
||
void WiFiClientSecure::setFingerprint(const String& fingerprint) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does a C string ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. The advantage of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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++)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming we're sscanf'ing manually here, can we use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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):
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this code fingerprint must be in PROGMEM right ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, maybe I was too influenced by AVR. Also confused by |
||
// skip separator | ||
_fingerprint--; | ||
continue; | ||
} | ||
fp[idx++] = (c << 4) + d; | ||
} | ||
|
||
if (idx == 20) | ||
setFingerprint(fp); | ||
} | ||
|
||
} // namespace |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,6 +67,7 @@ class WiFiClientSecure : public WiFiClient { | |
_knownkey_usages = usages; | ||
} | ||
// Only check SHA1 fingerprint of certificate | ||
void setFingerprint(const String& fingerprint); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above about making this eat |
||
void setFingerprint(const uint8_t fingerprint[20]) { | ||
_use_fingerprint = true; | ||
memcpy_P(_fingerprint, fingerprint, 20); | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @earlephilhower I'm a bit confused. My question was for |
||
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); | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting way to cause an error on compile. |
||
}; | ||
|
||
}; | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?