ETR01SDK-459: Enhance deinitialization on errors#408
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request enhances error handling and cleanup in the libtropic library by adding missing secure memory zeroing, improving deinitialization logic, and ensuring proper cleanup on error paths. The changes focus on security improvements and more robust error handling throughout the codebase.
- Added secure zeroing of sensitive cryptographic material in key derivation functions
- Implemented proper cleanup with goto-based error handling in initialization functions
- Added missing session status checks and test cleanup procedures
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libtropic.c | Added cleanup error handling in lt_init() to properly deinitialize resources on failure |
| src/libtropic_l3.c | Enhanced lt_in__session_start() with secure zeroing and improved cleanup flow; added session status check in lt_in__ecc_key_generate() |
| src/lt_hkdf.c | Added secure memory zeroing and goto-based cleanup pattern for sensitive intermediate values |
| src/lt_l3_process.c | Removed redundant session status checks from lt_l3_encrypt_request() and lt_l3_decrypt_response() |
| tests/functional/lt_test_rev_startup_req.c | Added lt_deinit() call in cleanup function for proper resource cleanup |
| tests/functional/lt_test_rev_handshake_req.c | Added lt_deinit() call at end of test for proper resource cleanup |
| src/lt_crypto_common.h | Added warning documentation about context initialization requirements |
| src/lt_aesgcm.h | Added warning documentation about deinit function assumptions |
| CHANGELOG.md | Updated to document all changes in this PR |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot Do you think there are some other places where secure memory zeroing should be done? |
|
lt_in__session_start: having labels scattered around the function is not ideal. I think we can actually use one single label at the end, if we do a few modifications: Variables: We can always zero all variables if we declare them at the beginning. |
The labels are not scattered randomly, they are always under a self-contained block of code, so the cleanup is done in parts. I just didn't want to zero out a ton of variables if only some of them were actually used. I will try to refactor it. |
|
@copilot I pushed some commits today, refactoring the goto cleanups. Can you check if I didn't do some mistake? |
|
@andreondra , I refactored the cleanups:
Please, take a look at it. |
82ed292 to
e799604
Compare
…it at the end or in the cleanup
…tart_aesgcm_error
… and lt_ecc_key_read
… and lt_init cleanup
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
e799604 to
def4f1c
Compare
Description
lt_handle_t.l3.session_statusinlt_in__ecc_key_generate().lt_init().lt_handle_t.l3.session_statusinlt_l3_encrypt_request()andlt_l3_decrypt_response().lt_deinit()in lt_test_rev_handshake_req and lt_test_rev_startup_req.Type of Change
Select the type(s) that best describe your change:
Checklist
Before submitting, please confirm that you have completed the following: