WIP: Add a SHA256Builder #3223

comino · 2017-05-09T20:52:12Z

A secure hashing algorithm is a crucial part of a secure signature.
At the moment only MD5 is part of the core, thus I propose to add the SHA256Builder.

Can then be implemented with: https://github.com/esp8266/Arduino/pull/3105/files

ToDo

Implement a "HashBuilder" base class
Implement a seperate unit test for HashBuilder
Check performance impact of new implementation
(opt) Implement RSA/AES builder

This is a preperation for firmware file signature validation. In order to check, that a binary comes from a trusted source the signature needs to be validated. A secure hashing algorithm is a cruicial part of a secure signature. At the moment only MD5 is part of the core. I propose to add a SHA256Builder as a fundament for the firmware validation.

igrr · 2017-05-10T12:45:38Z

As discussed in #3105, i think the way forward is to add axTLS source as a submodule (or a subtree) to the Arduino core. This way we can use hashing, HMAC, x509 function without having to duplicate them.

igrr · 2017-05-10T12:48:19Z

The other note about this: it seems to me that a lot of code in MD5Builder and SHA256Builder is very similar, with difference coming only in actual hashing function calls, hash context structures, and hash sizes. I do like the addition of SHA256Builder, but please consider folding implementation of these two hashes into one, both to reduce code size overhead (if both are used) and to reduce source code duplication.

comino · 2017-05-11T08:03:58Z

Thanks for that constructive feedback!

To not introduce breaking changes I can do a "hashLib" baseclass and inherit it in MD5Builder and SHA256Builder? Or drop this and think of a serious axTLS submodule?

igrr · 2017-05-12T06:05:03Z

I think if you make a base class for hash builders and temporary introduce (duplicate) SHA256 code this will be okay. Once someone figures out how to add axTLS as a submodule, this SHA256 code can be removed.

…er, added seperate unit test for hashBuilder

codecov-io · 2017-05-23T13:06:46Z

Codecov Report

Merging #3223 into master will increase coverage by 0.74%.
The diff coverage is 80.76%.

@@            Coverage Diff             @@
##           master    #3223      +/-   ##
==========================================
+ Coverage    27.6%   28.35%   +0.74%     
==========================================
  Files          20       21       +1     
  Lines        3655     3707      +52     
  Branches      678      686       +8     
==========================================
+ Hits         1009     1051      +42     
- Misses       2468     2474       +6     
- Partials      178      182       +4

Impacted Files	Coverage Δ
cores/esp8266/SHA256Builder.cpp	`80.76% <80.76%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b623613...dd98ddc. Read the comment docs.

comino · 2017-05-23T13:26:58Z

The Crypto Submodule you mentioned - is there a concrete plan somewhere for this?
I have my own implementation for a "CryptoBuilder" very similar to my proposed HashBuilder.
If wanted I could implement it into the core (if wanted) with a structure like:

/core
/core/crypto/md5.h
/core/crypto/sha256.h
/core/crypto/aes.h
/core/crypto/rsa.h
/core/CryptoBuilder.h (like the current HashBuilder)
/core/RSABuilder.h /.cpp
/core/MD5Builder.h /.cpp
/core/SHA256Builder.h /.cpp
/core/AESBuilder.h /.cpp

The drawBack is, that the core will be filled with more classes.

Alternatively, we might collect all functionalities (sha256, md5, rsa and aes) in a single class. The implementation I currently use in form of a library looks like this:
`
class CryptoClass {
public:
bool rsaInit(const String &certPath, const String &privKeyPath );
String rsaEncrypt(const String &payload);
String rsaDecrypt(const String &cypher);
String rsaSha256Sign(const String &payload);
bool rsaSha256Verify(Stream &stream, const String &signature);
String rsaMD5Sign(const String &payload);
bool rsaMD5Verify(Stream &stream, const String &signature);

String
String aesEncrypt(const String &payload);
String aesDecrypt(const String &cypher);

String sha256(const String &payload);
String sha256FromStream(Stream &stream, size_t length);

String MD5(const String &payload);
String MD5FromStream(Stream &stream, size_t length);
}
`

Humancell · 2017-05-23T14:16:44Z

Does this include HMAC SHA256?

comino · 2017-05-23T14:36:41Z

Not yet, but I have a dirty implementation for signature checks with SHA256 if we could agree on a submodule structure.

igrr

Overall looks good to me!

Suggest finalizing this and merging, then discussing RSA/AES as a separate step.

Another possible to-do thing (after this MR) is to update the Updater and all the dependent Updaters (HTTP, webserver, Arduino) to allow SHA256 to be used for hashes, with the goal of removing the use of MD5 after a while.

igrr · 2017-05-24T15:39:33Z

cores/esp8266/HashBuilder.cpp

+  resString.reserve(res.size()*2);
+  static constexpr char hex[] = "0123456789abcdef";
+  for (uint8_t & c : res){
+      resString += String(hex[c / 16]);


I guess I can't be upset with the indentation given that we don't have specific rules in this project (which is a shame)... but would be great if indentation was at least consistent at file scope.

igrr · 2017-05-24T15:42:09Z

cores/esp8266/HashBuilder.cpp

+void HashBuilder::getChars(char * output){
+  auto res = _result();
+  for(uint8_t i = 0; i < res.size(); i++){
+    sprintf(output + (i * 2), "%02x", res[i]);


This does nearly the same thing as _byteVecToString but uses sprintf instead of the hex array. Would it make sense to use same method here?

igrr · 2017-05-24T15:43:20Z

cores/esp8266/HashBuilder.h

+
+    // deprecated
+    void getBytes(uint8_t * output );
+    void getChars(char * output);


suggest expressing deprecation with __attribute__((deprecated)) Why are these deprecated, by the way?

igrr · 2017-05-24T15:45:23Z

cores/esp8266/HashBuilder.h

+
+  protected:
+    virtual void _init(void) = 0;
+    virtual void _update(uint8_t * data, uint16_t len) = 0;


I know this is carried over from the original code, but since you are improving things, would you mind replacing this uint16_t with size_t? It doesn't serve any purpose and only adds extra extui instructions to truncate 32-bit int to a 16-bit one.

igrr · 2017-05-24T15:48:55Z

cores/esp8266/HashBuilder.h

+
+// might be renamed to cryptoBuilder, cause same baseclass
+// might be reused in CrpytoSubmodule
+typedef HashBuilder CryptoBuilder;


I don't think that the current HashBuilder interface is a good fit for AES/RSA. There may be common operations such as conversions from/to byte/hex arrays, but this doesn't mean that these features should share a common interface, given how different the use cases are. If we happen to need the byte/hex transforms for crypto classes, my suggestion is to move these transforms into helper functions/classes shared between HashBuilder and the future crypto classes.

igrr · 2017-05-24T15:50:05Z

cores/esp8266/MD5Builder.cpp

    MD5Update(&_ctx, data, len);
+    return;


no need for explicit return here

igrr · 2017-05-24T15:50:18Z

cores/esp8266/MD5Builder.cpp

+void MD5Builder::_init(void){
+  _resultVector = {};
+  MD5Init(&_ctx);
+  return;


igrr · 2017-05-24T15:50:32Z

cores/esp8266/MD5Builder.cpp

+void MD5Builder::_final(void){
+    _resultVector.resize(HEX_STR_LEN);
+    MD5Final(_resultVector.data(), &_ctx);
+    return;


igrr · 2017-05-24T15:51:57Z

cores/esp8266/SHA256Builder.cpp

+void SHA256Builder::_final(void){
+    _resultVector.resize(HEX_STR_LEN);
+    SHA256_Final(_resultVector.data(), &_ctx);
+    return;


returns, indentation

igrr · 2017-05-24T15:52:17Z

cores/esp8266/SHA256Builder.h

@@ -0,0 +1,43 @@
+/*
+  MD5Builder.h - exposed md5 ROM functions for esp8266


the ever present comment line :)

igrr · 2017-05-24T16:01:03Z

Regarding importing axTLS code — let me give it a try...

comino · 2017-05-24T19:51:28Z

Thanks a lot @igrr for taking the time for constructive feedback!
Will digest them and polish asap!

EDIT:
I spend quite a while with Axtls RSA/AES for my personal crypto implementation. Would be very glad to help to bring a crypto submodule to the core. :). Is there a thread where we could have a discussion?

devyte · 2018-03-24T16:16:19Z

CC @earlephilhower
How does this fit in with #4273 ? I'd love to have a standalone crypto module with all the crypto algorithms usable from anywhere, and completely separate from axtls or bearssl.

earlephilhower · 2018-03-25T16:31:57Z

Without a hardware module to accelerate things, either axtls or bearssl would be doable here. But so would any standard C/Arduino library, and it may end up importing less code than either one. There are hundreds of implementations out there, most optimized for 32 or 64 bit processors, so if this is really important I'd suggest looking there and not using either SSL lib as a base.

The way BearSSL is implemented the hash functions are written as pseudo-C++ classes in straight C. It's got MD5, SHA 1, 224, 256, 384, and 512 implemented IIRC. It's not rocket science pulling them out and doing a wrapper, but I have not done any benchmarking to compare their performance.

earlephilhower · 2019-09-29T04:38:48Z

Given this is a WIP with 2 years w/o update, intended for use on signed updates (which are already in there using something similar but using BearSSL), I'm closing. If someone wants to do an abstract wrapper for hashes around the BSSL implementation (honestly, they're simple enough to use directly w/o a wrapper), a new PR would be the best way forward.

comino added 2 commits May 23, 2017 14:59

WIP: Introduced HashBuilder base class for MD5Builder and Sha256Build…

1cfb45b

…er, added seperate unit test for hashBuilder

removed added tmp files

65db990

comino changed the title ~~Add a SHA256Builder~~ WIP: Add a SHA256Builder May 23, 2017

Removed more unwanted files

dd98ddc

igrr requested changes May 24, 2017

View reviewed changes

This was referenced Jul 15, 2017

Subtree axtls #3423

Closed

Subrepo axtls #3424

Closed

earlephilhower closed this Sep 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add a SHA256Builder #3223

WIP: Add a SHA256Builder #3223

comino commented May 9, 2017 •

edited

Loading

igrr commented May 10, 2017

igrr commented May 10, 2017

comino commented May 11, 2017

igrr commented May 12, 2017

codecov-io commented May 23, 2017 •

edited

Loading

comino commented May 23, 2017 •

edited

Loading

Humancell commented May 23, 2017

comino commented May 23, 2017 •

edited

Loading

igrr left a comment

igrr May 24, 2017

igrr May 24, 2017

igrr May 24, 2017

igrr May 24, 2017

igrr May 24, 2017

igrr May 24, 2017

igrr May 24, 2017

igrr May 24, 2017

igrr May 24, 2017

igrr May 24, 2017

igrr commented May 24, 2017

comino commented May 24, 2017 •

edited

Loading

devyte commented Mar 24, 2018

earlephilhower commented Mar 25, 2018

earlephilhower commented Sep 29, 2019

		@@ -0,0 +1,43 @@
		/*
		MD5Builder.h - exposed md5 ROM functions for esp8266

WIP: Add a SHA256Builder #3223

WIP: Add a SHA256Builder #3223

Conversation

comino commented May 9, 2017 • edited Loading

igrr commented May 10, 2017

igrr commented May 10, 2017

comino commented May 11, 2017

igrr commented May 12, 2017

codecov-io commented May 23, 2017 • edited Loading

Codecov Report

comino commented May 23, 2017 • edited Loading

Humancell commented May 23, 2017

comino commented May 23, 2017 • edited Loading

igrr 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

igrr commented May 24, 2017

comino commented May 24, 2017 • edited Loading

devyte commented Mar 24, 2018

earlephilhower commented Mar 25, 2018

earlephilhower commented Sep 29, 2019

comino commented May 9, 2017 •

edited

Loading

codecov-io commented May 23, 2017 •

edited

Loading

comino commented May 23, 2017 •

edited

Loading

comino commented May 23, 2017 •

edited

Loading

comino commented May 24, 2017 •

edited

Loading