-
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
Add BearSSL client and server, support true bidir, lower memory, modern SSL #4273
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
65cc9f8
to
9c088ba
Compare
This comment has been minimized.
This comment has been minimized.
013a1c5
to
1ef3bd1
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Great work @earlephilhower ! I am adopting your bearssl port in my AsyncTCP fork, and have similar observations on stack usages -- starting at 33KB of free heap space before making any connection, it quickly drains down to 2KB before a x509 decoder can be created. (the default 16KB recv buffer is the main culprit). But I have an interesting observation during debugging:
Is it possible to repurpose the free sys stack to satisfy the 4.5KB heap needs of bearssl? |
Very interesting idea, @Adam5Wu! First of all, you can/should set a smaller transmit buffer size. There's no reason to allow a full 16KB transmit buffer unless you really need the highest bandwdth SSL transmit. Try the ~900 bytes transmit size I default to, it works but will break up >512b unencrypted packets into multiple 512b encrypted ones. On receive the same logic applies. If you're not expecting, say, >2KB POSTs from a web client, then set server receive buffers smaller as well. SSL clients, though, you're kind of stuck unless you can only generate HTTP RANGE GETs..OTW 16KB receive buffers are required. Those will save you tons of heap, but none of the stack which still needs many KB to work (not a dig on BearSSL, the encryption algorithms themselves have large temporary memory needs). To do what you're suggesting correctly we'd need to get to the SYS context for bearssl calls, and I don't know how to do that. Do you? To do it incorrectly (as in super-hackish) all you need to be able to do is get the end of the SYS stack. Pass that address -4.5KB in to the synthetic stack function, and it'll "just work" assuming the last 4.5KB of SYS stack aren't used. Again, I'm not sure how to grab the SYS context info as I've never thought about it...if we check before calling a BearSSL API that the space we say is available it should be fine... |
Checkout libraries/ESP8266WiFi/src/include/ClientContext.h
You should see something like 3ffffcXX, and it seems this stack can go as deep as 3fffe000. I have tried to modified my local copy of your bearssl port, effectively undo the alternative heap changes, and it seems I am able to complete a connection under AsyncTCP, with 4.5KB more free heap. :) I think it is possible to enjoy the benefit of extra sys stack without having to fully embrace AsyncTCP. |
The system stack is spelled out, not explicitly, in the app.ld file. There's a line after the dram0_0_seg that's
So that's legit, but what we need to make it safe to use outside of the sys context in appspace is the location they stuff the stack pointer on a context switch. Since we're going to a new SDK, even, the comments on the espressif board may not hold true any longer. :( Also, FWIW, the elliptic curve crypto takes more stack space than the x509 decode. IT's actually not the x509 portion, but the RSA 4Kbit signing/checking part of the certificate validation that eats up space early in the process. You can pull down the stack space a little bit (~1.5KB IIRC) by disabling all the elliptic curve support, but honestly I'd rather have a "works and can validate with darn near anything" than the extra 1.5KB. In case you didn't find it already, to disable the stack stuff just go to src/inner.h, go down to the bottom of the file, and replace the STACK_PROXY_ALLOC() from ESP8266 with the non-ESP8266 one. That'll just define variables in the function like standard GCC. |
ECDSA support is the main reason I pursue bearssl, so I am not going to give that up! 😄 I may be wrong, but to my understanding, RSA or ECDSA, they ONLY happen at handshake stage, when certificates are validated and shared secrets are exchanged. After that point, everything else only involves symmetric key block/stream cipher. So my point is, during the short timespan of "after TCP connected" --> "handshake done", hook into ClientContext and perform all bearssl "heavy crypto" in the callback context, let it use the sys context stack, which can go up to 8KB. (By "use" I mean "natrual" use, not through the proxy) Once handshake completes, the rest of bearssl operations can happen in cont context (as they do now). Because only "simple crypto" will be invoked (block/stream cipher, no RSA/ECC), the stack usage will not be high, so again, we can let it use the cont context stack natrually without proxy. And thanks for the hint about stack proxy macros, that is just what I did. 😃 |
ta->flags |= BR_X509_TA_CA; | ||
} | ||
|
||
delete dc; // Done with x509 decoder |
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.
At this point pk still holds reference to structure inside dc, so it cannot be freed here.
Issue became visible when add some debug prints after this line corrupted the public key.
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.
Thanks for finding this! I'll throw a new commit up soon, simple fix (you've probably already done it in your own copy).
About the 2ndary stack, there's talk of using this for other things in the Arduino side as well on the maintainers gitter. We may get some official (or at least better) info on how much can safely be used. I'll need to see how the ctx switching is handled to see if we can "easily" pop just the certificate validation in the SYS stack. The easiest way, though, would be to use the setup as-is and point that 4.5K buffer to SYS_STACKEND-4,.5KB and call it a day. Assuming that stack space is always available anyway (because the SYS stack is overallocated), that's just as safe as handling things while in SYS. I think you're correct on the stack usage differences. Seems like only the key exchange is done using ECC or RSA, everything else is almost trivial AES or other simple block codes. |
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.
Was leaving some comments and realized that the PR is already merged. Feel free to ignore...
bool begin(String host, uint16_t port, String uri, String httpsFingerprint); | ||
// Use BearSSL for secure HTTPS connection | ||
bool begin(String url, const uint8_t httpsFingerprint[20]); |
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.
Why can't we pass the fingerprint as String
? If the intention is to disambiguate between two implementations of TLS, then it's a really non-obvious way to do this. Introducing a function with different name (beginBearSSL?) and keeping the argument type might be an option.
@@ -19,43 +19,5 @@ | |||
Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA | |||
*/ |
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.
The copyright header can now be removed from this file.
Serial.printf("Connected!\n-------\n"); | ||
client->write("GET "); | ||
client->write(path); | ||
client->write(" HTTP/1.0\r\nHost: "); |
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.
In a fair few issues users have been advised to use HTTPClient library instead of rolling their own HTTP request code. Does it make sense to update the example to do the same?
void loop() { | ||
Serial.printf("\n\n\n\n\n"); | ||
|
||
yield(); |
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.
Is this yield
really needed?
#ifndef _BEARSSLHELPERS_H | ||
#define _BEARSSLHELPERS_H | ||
|
||
#include <bearssl/bearssl.h> |
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.
It would be great to avoid pulling in third party libraries into sketch namespace. This can be done is a way similar to the way LwIP and axTLS are currently hidden from users.
path += "/"; | ||
} | ||
|
||
String tblName = path + "ca_tbl.bin"; |
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.
Why is the name hardcoded?
#include <Arduino.h> | ||
#include <bearssl/bearssl.h> | ||
|
||
// Virtual base class for the certificate stores, which allow use |
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.
If this is a base class for certificate stores, why is it specific to BearSSL? If we add a similar class for AxTLS, would it be derived from BearSSL one?
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.
hello:
have a stable version with this changes?
So a newbie who has been using:
can simply switch to bearSSL exactly how...? |
hello every one I want to use setBufferSizes for BearSSL Secure Server Class which is mentioned here: |
-edit- No longer WIP, seems stable enough for review and testing use outside my own lab -edit-
As discussed in #3490, axTLS has some issues with not checking for OOM, is unidirectional (i.e. some hacks required to support bidir MQTT protocol/etc.), and uses lots of memory.
This patch takes BearSSL, ports it to the ESP8266, and creates two new classes to replace the WiFiServerSecure and WiFiClientSecure classes. BearSSL memory requirements are much lower than axTLS when doing bidirectional (only ~24KB for a client, ~5.5KB for a server) connections since the receive and transmit buffer sizes are independently configurable, allowing for both a bidir SSL client and bidir SSL server to be run at the same time with memory to spare for your own application. It also does not do any memory allocations at operation time, so you're not likely to get as many random crashes as axTLS when tight on memory. Finally, BearSSL supports a much wider variety of modern ciphers (and does not do the completely broken RC4, for example) and certificate types.
BearSSL is MIT licensed by the author.
While the patch here does work and can be used as both a SSL server and Client, it's still a WIP as I expect the API to change and grow, as well as more advanced testing by real users.
The Server/Client interface is the same as the existing classes, but the SSL-specific bits are different. This applies to the way certificates are validated (they are ALWAYS checked against a trust anchor/hardcoded cert you supply), and how SSL parameters are set amongst other things, so it's not as simple as just replacing WiFiClientSecure w/WiFiClientBearSSL.
The new classes are called (for now) WiFiClientBearSSL/WiFiServerBearSSL.
See the new example bearssl/bearssl.ino for a quick example. See https://bearssl.org for more BearSSL info as well as the tools to generate your own trust anchor structures for your own apps.
I'm including a precompiled bearssl.a library for now, but the changes to the original sources are quite trivial and will be put up at least in my own repo if not integrated w/the main BearSSL code if they're acceptable to the author.
https://github.com/earlephilhower/bearssl-esp8266 is the port sources.
Thanks very much to the BearSSL author @pornin for his assistance in the original issue.