The second cipher-update failure destroys the volatile symmetric key but left its identifier cached in session state. Clear it immediately so sec1_close_session() cannot try to destroy the same key again.
Constraint: Follow-up to maintainer review on espressif/esp-idf#18813
Confidence: high
Scope-risk: narrow
Tested: security1.c cross-compiled for ESP32 with Xtensa GCC 14.2.0; test_security1.c compiled with the test app flags; git diff --check
Not-tested: Full master test-app link or on-target execution; local IDF 5.4.3 differs from the PR's master baseline in PSA ABI and Mbed TLS headers
In handle_session_command1(), if the second psa_cipher_update()
call (encrypting the device verify data to send back to the client)
fails, the error path only frees the outbuf ciphertext buffer. The
out (Sec1Payload) and out_resp (SessionResp1) structures allocated
just before it are never freed, and neither the cipher operation
(cur_session->ctx_aes) nor the imported key (key_id) are released.
The caller (sec1_req_handler(), via sec1_session_setup()) returns
immediately on a non-ESP_OK result without doing any cleanup of its
own here - sec1_session_setup_cleanup() only runs on the success
path, once resp->sec1 has actually been assigned - so nothing else
ever frees these on this path.
Add psa_cipher_abort()/psa_destroy_key() and free() for out/out_resp,
matching the cleanup already done for every other failure branch
earlier in this same function.
Fixes#18804
Signed-off-by: yi chen <94xhn1@gmail.com>
handle_session_command0() freed cur_session->srp_hd on every error path
without clearing the pointer. A subsequent sec2_close_session() call
(e.g. on BLE disconnect, or via sec2_new_session evicting the old
session) checks `if (cur_session->srp_hd)` and frees it again,
double-freeing a dangling pointer.
sec1_new_session()/sec2_new_session() were calling sec*_close_session()
with the *new* session_id parameter instead of the existing
cur_session->id. The close handler validates `cur_session->id ==
session_id` before performing teardown, so the call always failed with
ESP_ERR_INVALID_STATE.
Effect: when a peer started a new provisioning session while another was
already active, the previous session's PSA keys, AES context, SRP handle
and username buffer were leaked instead of being destroyed. The cleared
session struct was overwritten by the new session, leaking the previous
key handles inside PSA Crypto and (for security2) leaking heap memory
for the username and SRP context.
Fix: pass cur_session->id so the close path actually executes the
teardown (psa_destroy_key/psa_cipher_abort/esp_srp_free/free) before the
new session takes over.
Add checks to validate client_verify_data pointer and length before
processing in handle_session_command1. Prevents NULL pointer dereference
when client omits verifier data in Session_Command1, which could cause
device crash during provisioning (remote DoS attack).
Using same IV in AES-GCM across multiple invocation of
encryption/decryption operations can pose a security risk. It can help
to reveal co-relation between different plaintexts.
This commit introduces a change to use part of IV as a monotonic
counter, which must be incremented after every AES-GCM invocation
on both the client and the device side.
Concept of patch version for a security scheme has been introduced here
which can help to differentiate a protocol behavior for the provisioning
entity. The security patch version will be available in the JSON
response for `proto-ver` endpoint request with the field
`sec_patch_ver`.
Please refer to documentation for more details on the changes required
on the provisioning entity side (e.g., PhoneApps).
1) Rename srp component to esp_srp
2) Remove dependency on hkdf sha
3) Restructure protocomm component APIs to make them more flexible for allowing multiple security versions
4) esp_srp: convert API return type from int to esp_err_t
5) esp_srp: Formatting changes
6) Added mbedtls_gcm instead of aes_ctr
Co-authored-by: Laukik hase <laukik.hase@espressif.com>
*MBEDTLS_ECDH_LEGACY_CONTEXT is now disabled by default.
*Fixed MBEDTLS_ECDH_LEGACY_CONTEXT issue for protocomm component.
*Removed all code under MBEDTLS_DEPRECATED_REMOVED
With the introduction of cookies to track a session, it is possible that the
clients restart the provisioning on the same session, specifically when a user
cancels a current provisioning attempt. This can result in an error as the state
on the device side and client side will go out of sync.
This has now been changed such that if SESSION_STATE_CMD0 is received on
an existing session, the state is reset and flow allowed to continue.
List of changes:
* Security APIs accept handle to instance created during initialization
* Protocomm internally stores the security instance handle and calls security APIs with this handle as first parameter
List of changes:
* Corner case exceptions are properly handled to ensure release of memory occupied by security infrastructure
* fixed erroneous cleanup of security instance by protocomm_console
* This manages secure sessions and provides framework for multiple transports.
* The application can use protocomm layer directly to have application specific extensions for provisioning (or non-provisioning) use cases.
* Following features are available for provisioning :
* Security - Security0 (no security), Security1 (curve25519 key exchange + AES-CTR encryption)
* Proof-of-possession support for Security1
* Protocomm requires specific protocol buffer modules for compilation which can be generated from the `.proto` files in the `proto` directory using make.
Co-Authored-By: Amey Inamdar <amey@espressif.com>
Co-Authored-By: Anurag Kar <anurag.kar@espressif.com>
