feat(hash): Add hashing library and new algorithms #11676
+1,894
−73
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
👋 Hello lucasssvaz, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
Test Results 76 files 76 suites 13m 12s ⏱️ Results for commit a07d589. ♻️ This comment has been updated with latest results. |
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
lucasssvaz
requested review from
SuGlider,
me-no-dev,
P-R-O-C-H-Y and
a team
as code owners
lucasssvaz
force-pushed
the
feat/hashing
branch
from
02a2cf0 to
129c1cd
Compare
lucasssvaz
force-pushed
the
feat/hashing
branch
3 times, most recently
from
e10fe12 to
1038921
Compare
lucasssvaz
changed the title
|
Since you renamed |
Now that I see the full PR I think I should revert this renaming to keep it consistent with the others. |
lucasssvaz
force-pushed
the
feat/hashing
branch
from
1038921 to
a07d589
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Hash library containing secure cryptographic algorithms and refactors the existing hash infrastructure. The primary purpose is to move hashing functionality into a dedicated library while adding support for modern secure algorithms (SHA2, SHA3, PBKDF2_HMAC) to address security vulnerabilities.
Key changes:
- Refactored HashBuilder base class to provide common functionality
- Moved SHA1Builder to the Hash library while keeping core files intact
- Added new secure hash algorithms: SHA2 (224/256/384/512), SHA3 (224/256/384/512), and PBKDF2_HMAC
Reviewed Changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| libraries/Hash/src/SHA3Builder.h | Header file defining SHA3 variants and implementation class |
| libraries/Hash/src/SHA3Builder.cpp | Implementation of SHA3 Keccak algorithm with proper padding |
| libraries/Hash/src/SHA2Builder.h | Header file defining SHA2 variants and implementation class |
| libraries/Hash/src/SHA2Builder.cpp | Implementation of SHA2 family algorithms (SHA-224/256/384/512) |
| libraries/Hash/src/SHA1Builder.h | Updated SHA1 header to match new HashBuilder interface |
| libraries/Hash/src/SHA1Builder.cpp | Updated SHA1 implementation with license changes |
| libraries/Hash/src/PBKDF2_HMACBuilder.h | Header for password-based key derivation function |
| libraries/Hash/src/PBKDF2_HMACBuilder.cpp | Implementation of PBKDF2-HMAC algorithm |
| libraries/Hash/library.properties | Library metadata and configuration |
| libraries/Hash/keywords.txt | Arduino IDE syntax highlighting definitions |
| libraries/Hash/examples/ | Example sketches demonstrating usage of new algorithms |
| cores/esp32/HashBuilder.h | Refactored base class with common hash functionality |
| cores/esp32/HashBuilder.cpp | Implementation of common HashBuilder methods |
| cores/esp32/MD5Builder.h | Updated MD5 header to match new interface |
| cores/esp32/MD5Builder.cpp | Updated MD5 implementation removing duplicate code |
| cores/esp32/HEXBuilder.h | Added documentation comment |
| cores/esp32/HEXBuilder.cpp | Cleaned up includes |
| CMakeLists.txt | Updated build configuration for new Hash library |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| // Pad with zeros to make room for the length | ||
| while (buffer_size + 8 < block_size) { |
There was a problem hiding this comment.
The condition should use <= instead of < to ensure proper padding space. The current condition may leave insufficient space for the length field in edge cases.
Suggested change
| while (buffer_size + 8 < block_size) { | |
| while (buffer_size + 8 <= block_size) { |
Copilot uses AI. Check for mistakes.
Comment on lines
+151
to
+153
| for (size_t i = 0; i < derivedKeyLen; i++) { | ||
| output[i] = (char)derivedKey[i]; | ||
| } |
There was a problem hiding this comment.
The getChars method is casting bytes to chars, which can result in data loss for non-printable bytes. This method should use hex encoding like other hash builders or be documented as returning binary data.
Suggested change
| for (size_t i = 0; i < derivedKeyLen; i++) { | |
| output[i] = (char)derivedKey[i]; | |
| } | |
| // Hex-encode the derived key | |
| const char hexDigits[] = "0123456789abcdef"; | |
| for (size_t i = 0; i < derivedKeyLen; i++) { | |
| output[i * 2] = hexDigits[(derivedKey[i] >> 4) & 0xF]; | |
| output[i * 2 + 1] = hexDigits[derivedKey[i] & 0xF]; | |
| } | |
| // Optionally null-terminate if used as a C string | |
| output[derivedKeyLen * 2] = '\0'; |
Copilot uses AI. Check for mistakes.
Comment on lines
+173
to
+181
| void PBKDF2_HMACBuilder::setPassword(const uint8_t* password, size_t len) { | ||
| if (this->password != nullptr) { | ||
| delete[] this->password; | ||
| } | ||
| this->password = new uint8_t[len]; | ||
| memcpy(this->password, password, len); | ||
| this->passwordLen = len; | ||
| calculated = false; | ||
| } |
There was a problem hiding this comment.
Password data should be securely cleared from memory before deallocation to prevent sensitive data from remaining in memory. Consider using memset_s or explicit_bzero before delete[].
Copilot uses AI. Check for mistakes.
Comment on lines
+191
to
+199
| void PBKDF2_HMACBuilder::setSalt(const uint8_t* salt, size_t len) { | ||
| if (this->salt != nullptr) { | ||
| delete[] this->salt; | ||
| } | ||
| this->salt = new uint8_t[len]; | ||
| memcpy(this->salt, salt, len); | ||
| this->saltLen = len; | ||
| calculated = false; | ||
| } |
There was a problem hiding this comment.
Salt data should be securely cleared from memory before deallocation to prevent sensitive data from remaining in memory. Consider using memset_s or explicit_bzero before delete[].
Copilot uses AI. Check for mistakes.
Comment on lines
+48
to
+55
| void PBKDF2_HMACBuilder::clearData() { | ||
| if (derivedKey != nullptr) { | ||
| delete[] derivedKey; | ||
| derivedKey = nullptr; | ||
| } | ||
| derivedKeyLen = 0; | ||
| calculated = false; | ||
| } |
There was a problem hiding this comment.
Derived key data should be securely cleared from memory before deallocation. Consider using memset_s or explicit_bzero before delete[] to prevent sensitive key material from remaining in memory.
Copilot uses AI. Check for mistakes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Change
Move hashing algorithms and examples into a
Hashlibrary. Some files are required to stay in the core as they are used by core files. The inner structure of the base class was also revised.Also, all the available algorithms in the repo have some flaw and are not considered secure. To address that, this PR also adds support for the SHA2, SHA3 and PBKDF2_HMAC algorithms.
This will be required to fix some vulnerabilities found by CodeQL.
Tests scenarios
Tested locally