Fixing a potential Integer Overflow in sendCommand() and improving styling and inline protocol explanation

[gili-yankovitch]

There was a potential integer overflow @ sendCommand() where length could be larger than the protocol overhead, mainly causing erroneous CRC calculation but might cause other types of errors such as a buffer overflow over the packet_to_CRC buffer. Additionally, refactored the source code to pass variables to the top of functions, correct function exits, self-explanatory protocol while eliminating magic-numbers in the code (mostly around the main protocol).

[lewispg228]

Hey @gili-yankovitch, Wow! Thanks for your work on this! I was able to look through it quickly just now and you've done some great stuff in here. Adding in the SHA256 implementation is awesome!

I should be able to verify all of this on some hardware early next week, and then I will be happy to accept your PR into the repo.

Thanks again and I'll keep you posted. -Pete

[lewispg228]

Hey @gili-yankovitch , I was able to look through this more closely this afternoon and upload onto some hardware. It all looks like good improvements. I had a couple questions for you before we move forward with merging it into master.

When I first tried to compile it, there were a few errors that I was able to work around by commenting/removing. Arduino couldn't find the am_util_debug.h file. What was your reasoning behind using this for debug?

Is uprintf() better in some way than using _debugSerial->print() ?

Also, Arduino didn't want to compile other things, and I wasn't sure what you were doing here:

_i2cPort->write_byte

...

_i2cPort->available2()

...

i2cPort->read2()

...

_i2cPort->write2(total_transmission, total_transmission_length)

If I changed these back to the previous names (i.e. write, available, read, write) then it compiled and worked.

Let me know and thanks again! -Pete

[gili-yankovitch]

Hi, I was about to amend this and push another commit. For some weird reason when using VTables and overrides (well, vtables) over my Apollo3 Nano, things would crash (still have no idea why and didn't find the time to look into it) so I had to change this to avoid using virtual functions. I will fix this and commit it again (along with other improvements). Thanks on your feedback!

[gili-yankovitch]

Hi @lewispg228, I've amended all my weird changes. I've also added some macros that interface with SlotConfig and KeyConfig addressing and some more cool stuff. Regarding uprintf(): This is just an alias I had for am_util_debug_printf() which is a (relatively) direct way to interface nicely with the UART, in C, based only on the HAL for Ambiq boards. I've removed it as this library can be used with any kind of Arduino, not just Ambiq ones. All-in-all, I'm continuing to work with the chip and might upgrade the code in the future even further and will ask for another pull request once I have something more to contribute. If you have any questions, feel free to ask. Cheers, Gili.

[oclyke]

@gili-yankovitch Thanks a ton! We really appreciate your contribution and will look forward to anything else you might add. Regards, Owen

[lewispg228]

@gili-yankovitch Thanks again for your work on this! I'm hoping to try this out on some hardware later this week.

Also, I spoke with our engineering group about this pull request, and we discussed the use of "GOTO" in our libraries. We have never used these in any of our code previously, so we'd like to stay consistent. The preferred code for SparkFun libraries is to use "return true" or "return false" when exiting out of a function.

If you're up for it, please change these "GOTO"s on your PR, but if not, I'd be happy to accept your PR and then switch them back as a separate commit.

Let us know and thanks again! -Pete

PS I'll report back once a get a chance to try this out on some hardware (hoping later this week).

[gili-yankovitch]

Hi, Thanks for the comments and your kind responses! :-) I personally thinks "GOTO"s makes the code much cleaner, easier to maintain and prevents erroneous implementations and bugs regarding return-values, I will amend this soon and will let you know.

[gili-yankovitch]

Done, though I didn't have time to test it yet. I think it should work as it involved merely replacing the 'goto's to 'return false's and 'return true' at the end of functions. Hope that helps. Cheers!

[lewispg228]

Thanks @gili-yankovitch !

I just verified this on some hardware. Every thing seems to compile fine. I will merge this in shortly. Wahoo! We really do appreciate you tweaking those gotos back to our standard style of returns. It really helps keep our libraries and examples consistent.

I have not yet tried out the SHA256 feature, but I'm looking forward to using that in a future project with longer chunks of data to sign. Most of my project ideas involve embedded systems with not much data, so I'm not sure when I'll need to sign more than 32 bytes. But who knows, it will be handy to be able to create hashes and sign those!

Thanks again and if you're willing, I'd love to hear about what project (or projects) you are using this hardware/library for. While testing out your PR, I just added an extra layer of security to my garage door opener.

Cheers!

[lewispg228]

Hey @gili-yankovitch , I got to playing around with the sha256 today! Was working great, except I noticed a small bug when trying to hash exactly 64 bytes. Turns out the ATECC508A can only accept 63 bytes on the final END command to the SHA sequence. I think I added a proper fix (it seemed to be working for me on a few different data sizes).

I also wrote a small example for trying it out. I hope this helps others with their first time playing around with hashes.

If you have a sec, I'd appreciate you taking a look at my fix for the slight bug, and maybe even trying out example 7 on your hardware.

I also added some attribution in the header on the library and example files. I hope this is okay with you.

Let me know and thanks again!

[gili-yankovitch]

Seems legit :-) Good example and thanks for the attribution!! :D I'm working on an open source Bitcoin hardware wallet which initially will use only the Artemis Redboard Nano but will support storing the keys on ATECC chips as well. You can see the source code on my GitHub page (+ The Android App which is almost complete). All it takes is a simple make upload command. I've also designed a 3D printable case, so everything will be finished soon and I'll publish something on it somewhere... On a different note, If you have any additional cool hardware needs porting to or something other, we can chat. Cheers, Gili.