-
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
Signed updates #3105
Signed updates #3105
Conversation
…e last moment, allowing us to ignore the additional signature and certificate
Having a look through some archives, and I've found https://github.com/igrr/axtls-8266 which I assume is what is providing axTLS. I'll look at using the binary blob, rather than re-including my versions of those files. |
I've got some completed changes to support storing updater commands in
flash, which also include a fixed location for finding the updater
commands in flash, grand scheme I'd envisaged that the location of such
certificates would be included - either in the flash-command block itself,
or with the location of the flash command block (in fact I was looking to
put the certificates themselves in the flash-command block since it needs
to be preserved across updates and presumably so does the certificate? Or
does each update always include certificates?)
It's waiting on my finishing a script to generate boards.txt (as an
inherent part of putting commands in flash is ensuring things don't
overwrite them, which is simplest if we adjust the areas considered
available for code and SPIFFs, which implies updates to the Makefiles, and
since the info is also in boards.txt one script will generate both). I've
been a bit short on time recently, unfortunately :(
…On Mon, Apr 3, 2017 at 8:51 AM, Myles Eftos ***@***.***> wrote:
Having a look through some archives, and I've found
https://github.com/igrr/axtls-8266 which I assume is what is providing
axTLS. I'll look at using the binary blob, rather than re-including my
versions of those files.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3105 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAN_A313x3MTAsKZLXS6-f-INADplZhqks5rsApTgaJpZM4MwuCh>
.
|
@davisonja I've been thinking about the certificate storage issue. I think the certificate used to decrypt the signature should be part of the payload - as they usually have a shortish life (Mine expire after 12 months), and rotating them would be painful, as it would require manual reflashing. They are public anyway, so there isn't really any harm having them as part of the payload. The CA certificate - I'm torn. Having them in code would effectively update them every update. I'm not sure if this is a good idea or bad idea. It's good from the point of view that you can update them when they expire (though that would probably be a long time in the future). It's bad in that they could tampered with a bit more easily. Having them in flash also means you can get them out of source control, which is probably a good thing too. What is involved in getting them into flash? |
I think the first interesting question is how do you update the
certificates.
There's no technical barrier to the firmware itself updating the
certificate (since it's running on the chip it can read/write the flash),
and if your system ensure only legitimate code is running then that's an
okay thing to do. It shifts the responsibility to the firmware, and could
be integrated into the Updater code itself (when checking if new firmware
is available, also check if new certificates are available).
Is the entire update binary encrypted?
Putting things in flash is simple enough, do you have a ballpark size?
If you're just appending data files to the binary you might be able to use
the ESP tool which currently prepends the bootloader binary to the compiled
sketch.
…On Mon, Apr 3, 2017 at 9:51 AM, Myles Eftos ***@***.***> wrote:
@davisonja <https://github.com/davisonja> I've been thinking about the
certificate storage issue.
I think the certificate used to decrypt the signature should be part of
the payload - as they usually have a shortish life (Mine expire after 12
months), and rotating them would be painful, as it would require manual
reflashing. They are public anyway, so there isn't really any harm having
them as part of the payload.
The CA certificate - I'm torn. Having them in code would effectively
update them every update. I'm not sure if this is a good idea or bad idea.
It's good from the point of view that you can update them when they expire
(though that would probably be a long time in the future).
It's bad in that they could tampered with a bit more easily.
Having them in flash also means you can get them out of source control,
which is probably a good thing too.
What is involved in getting them into flash?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3105 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAN_A4nc6wZD91aHisv6YdZUhedtER8dks5rsBh8gaJpZM4MwuCh>
.
|
The CA cert I've been testing with (just a root, no chaining) is 991 bytes. The signing certificate is 925 bytes. (My original POC with test certs is here: https://github.com/madpilot/ESP8266FileSigning) I still like passing the signing certificate with the payload - no need to check for a new one - the newest one is always there. |
The updates to eboot for storing updater commands in flash (which allows it
to survive a power loss for more of the process) has to reserve a flash
page, which is mostly 4k. By default it's using around 1k of that, so
there's 3k 'spare' which I had hoped could be used for persistent
certificate/key storage.
…On Mon, Apr 3, 2017 at 10:07 AM, Myles Eftos ***@***.***> wrote:
The CA cert I've been testing with (just a root, no chaining) is 991
bytes. The signing certificate is 925 bytes. (My original POC with test
certs is here: https://github.com/madpilot/ESP8266FileSigning)
I still like passing the signing certificate with the payload - no need to
check for a new one - the newest one is always there.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3105 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAN_A4WcQifT9_Fp8IZOvPnEeqhWklHMks5rsBw6gaJpZM4MwuCh>
.
|
3k should be plenty. All I know about the memory layout is this: https://github.com/esp8266/Arduino/blob/master/doc/filesystem.md#flash-layout I assume you are talking about the OTA section? |
…to do certificate validation
I've removed the copied axtls files. By copying the headers files from the https://github.com/igrr/axtls-8266 we can use the binary blob. We can probably remove the ssl.h file in ESP8266WiFi/src/ |
@martinayotte suggested using EEPROM which sounds like a good solution - it's persistent, and it can be written to from the command line, making the initial setup fairly easy. Though it made me think about giving the develooer choices. I think I'll add a helper function that reads the very from EEPROM and another that will read it from SPIFFS, while leaving the existing addCA function. That way there is a sane default (EEPROM) but the developer can decide to store it elsewhere. |
When you say EEPROM, do you mean external hardware that's attached, or the
one that's stored in flash?
Leaving it up to the user seems like a good idea.
…On Thu, Apr 6, 2017 at 10:35 AM, Myles Eftos ***@***.***> wrote:
@martinayotte <https://github.com/martinayotte> suggested using EEPROM
which sounds like a good solution - it's persistent, and it can be written
to from the command line, making the initial setup fairly easy.
Though it made me think about giving the develooer choices. I think I'll
add a helper function that reads the very from EEPROM and another that will
read it from SPIFFS, while leaving the existing addCA function.
That way there is a sane default (EEPROM) but the developer can decide to
store it elsewhere.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3105 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAN_Az-gQvCKp8IJywKJrZ8M2m25iVGnks5rtBcXgaJpZM4MwuCh>
.
|
@davisonj the one that is stored in flash. |
This change looks very good, but i don't think that using a compile-time define ( Arduino way of implementing such an optional feature (whether anyone likes this or not) is to enable it at run time via some method call. Note that it is still possible to benefit from compile-time code size savings if certificate verification is not used. |
@@ -141,18 +148,35 @@ class UpdaterClass { | |||
return written; | |||
} | |||
|
|||
#ifdef VERIFY_SIGNATURE | |||
int addCA(const uint8_t *cert, int *len); |
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.
Similar to WiFiClientSecure
, suggest exposing a helper template method which loads CA certificate from a file (Stream).
#include <Arduino.h> | ||
#include <flash_utils.h> | ||
#include <MD5Builder.h> | ||
|
||
#ifdef VERIFY_SIGNATURE | ||
#include <ssl/crypto_misc.h> | ||
#include <crypto/crypto.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.
Do we need these headers in user-facing header file?
@@ -1,10 +1,17 @@ | |||
#ifndef ESP8266UPDATER_H | |||
#define ESP8266UPDATER_H | |||
|
|||
#define VERIFY_SIGNATURE 1 |
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.
Posted a note about using preprocessor macro as a feature switch above.
private: | ||
void _reset(); | ||
bool _writeBuffer(); | ||
|
||
bool _verifyHeader(uint8_t data); | ||
bool _verifyEnd(); | ||
|
||
#ifdef VERIFY_SIGNATURE | ||
CA_CERT_CTX *_ca_ctx; |
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.
Ok, i see now why the headers were needed. If you go for the approach with VerificationTraits
interface (call it some other way if you like), you will not have to expose these internals to the users, introducing axTLS names into the sketch global namespace.
cores/esp8266/md5.h
Outdated
|
||
extern void MD5Init (md5_context_t *); | ||
extern void MD5Update (md5_context_t *, uint8_t *, uint16_t); | ||
extern void MD5Final (uint8_t [16], md5_context_t *); |
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.
Suggest keeping this file — it links to the functions defined in ESP8266 ROM code. MD5 functions which run from ROM code run much faster than the axTLS defined ones which run from flash.
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.
Agreed
@@ -0,0 +1,99 @@ | |||
/* |
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.
Do you think there is a way to submodule these instead? If anything, i have one more use case for using axTLS as a submodule, to enable root certificate store (igrr/axtls-8266#44).
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.
A submodule or sub tree makes a lot of sense here
@@ -11,12 +11,12 @@ uint8_t hex_char_to_byte(uint8_t c) | |||
void MD5Builder::begin(void) | |||
{ | |||
memset(_buf, 0x00, 16); | |||
MD5Init(&_ctx); | |||
MD5_Init(&_ctx); |
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.
Likewise, suggest keeping ROM functions for performance reasons.
@@ -23,6 +23,9 @@ UpdaterClass::UpdaterClass() | |||
, _currentAddress(0) | |||
, _command(U_FLASH) | |||
{ | |||
#ifdef VERIFY_SIGNATURE | |||
_ca_ctx = (CA_CERT_CTX *)malloc(sizeof(CA_CERT_CTX)); |
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.
Suggest allocating this only when the feature gets actually used.
Thanks for the feedback @igrr - that should all be doable. I'm thinking some sort of delegator so the developer can drop in different validation schemes. I'll look at making the changes in the coming weeks (though we are just about to have a baby so progress will be slow!) |
ThumbsUp Looking forward for a release of this feature... |
We just had a baby, so progress has slowed somewhat. |
@madpilot congratulations! hot times ahead! in every sense ;) hope you 3(?) enjoy life between changing diapers.. :) |
Hi, guys! Thanks for your work, any updates? |
Hi Guys... |
Hi, this look amazing!!!! |
Adds the ability to verify the signature of an OTA update.
The developer certificate and signature are bundled at the end of the binary, with two additional uint32_t variables added to the very end to let the updater know where to look for them.
It works, but there is a few rough edges to clean up:
NOTE: The is a feature flag:
Iy's turned on by default in this version. Final version will allow it to be set as a build paramater.
Quick and Dirty documentation for those that know what they are doing
First step is to create a CA, and reference in the source of the sketch, by converting it to a binary header file using xxd:
Include the file in the Sketch, and using the new addCA function to reference it.
Compile and upload the sketch using Serial.
To do an update:
Create a developer certificate:
The library definately suppports sha256. It should support sha1 and md5 too.
Generate the signature for the new binary using openssl:
The mash them together using this C program (I'll convert it to Go or Python shortly)