mbedTLS 4 / PSA Crypto support on 2_x_dev#813
Conversation
Adds a parallel PSA Crypto code path for the mbedTLS backend, gated under `#if MBEDTLS_VERSION_MAJOR >= 4`. The legacy mbedTLS 3 path stays unchanged and remains the default. Covers crypto/cipher/aes_icm_mbedtls.c, aes_gcm_mbedtls.c, crypto/hash/hmac_mbedtls.c, the matching struct headers, plus cmake/FindMbedTLS.cmake (additionally finds libtfpsacrypto) and meson.build. CI matrix adds an mbedtls4 entry that builds mbedtls-4.0.0 from source on Ubuntu + macOS. Local: tested against system mbedTLS 3.6.4 (brew) and mbedTLS 4.0.0 (built from source), 10/10 ctest passing on both. For cisco#812.
…nter - GCM encrypt/decrypt now require exact dir match (catches crossed-ctx bugs PSA does not, since the key carries both ENCRYPT and DECRYPT usage flags) - GCM enc_fail returns cipher_fail (not bad_param) for genuine PSA crypto failures - AES-ICM: comment that PSA_ALG_CTR uses a full 128-bit counter, same as the legacy mbedtls_aes_crypt_ctr path; SRTP packet sizes (< 1 MiB) are unaffected
- mbedtls 2.x ships only mbedtls/version.h, not mbedtls/build_info.h; wrap the include with __has_include so the gate resolves on 2.x too - mbedtls4 install needs -DCMAKE_POSITION_INDEPENDENT_CODE=ON so static libtfpsacrypto can be linked into libsrtp2.so on Ubuntu - macOS mbedtls4 install path moved out of $GITHUB_WORKSPACE which the later actions/checkout@v2 step wipes; install to /tmp/ instead
acf4582 to
bd11b49
Compare
|
Dear @pabuhler can you please take a look? |
|
@vikramdattu , I started to look at this but I am no mbedTLS expert. So it is hard to say if the the usage is correct. I compare abit to main branch and use AI to review, it comes up with a few concerns. |
|
Dear @pabuhler I am confident enough with the change. Also, I have added the mbedtls test path for the change. One thing that makes me comfortable is, I have kept the default to existing (mbedtls3) with the mbedtls4 as an opt-in. So, the change should be safe to go if it passes the test suite for mbedtls4. I request you to do the top-level/API review. AI review will also do. I can then take this forward. PS: Adding @Harshal5 for the in-depth review who is one of the security engineers from Espressif Systems. |
|
@Ashish285, could you please help by taking a look at this as well? |
Harshal5
left a comment
There was a problem hiding this comment.
Unlike the old per-context mbedtls_* path, PSA stores keys in a shared global key store. If libsrtp processes streams from multiple threads, mbedTLS should be built with MBEDTLS_THREADING_C enabled.
This isn't a blocker, but it's probably worth documenting somewhere.
Other than that, LGTM!
| #if MBEDTLS_VERSION_MAJOR >= 4 | ||
| { | ||
| psa_hmac_ctx_t *state; | ||
| psa_status_t status = psa_crypto_init(); |
There was a problem hiding this comment.
Can we move the psa_crypto_init() call to some srtp's crypto initialization API (if it exists)? So, that we could avoid adding calls to psa_crypto_init() at multiple places.
Multiple calls to psa_crypto_init() is functionally fine as the API exits early in such cases:
psa_status_t psa_crypto_init(void)
{
psa_status_t status;
/* Double initialization is explicitly allowed. Early out if everything is
* done. */
if (psa_get_initialized()) {
return PSA_SUCCESS;
}
.
.
}
So this is more of a code maintenance improvement than a functional requirement.
There was a problem hiding this comment.
we are trying to move a way from having an explicit init function in libsrtp, not sure how that will end up. Maybe some thing that can be improved in the future.
There was a problem hiding this comment.
Agreed it's a maintenance improvement rather than a correctness issue — the repeated psa_crypto_init() calls are safe because of that early-out.
As @pabuhler notes, libsrtp is moving away from an explicit init entry point, so adding one now to centralize this would cut against that direction. I've kept the per-backend psa_crypto_init() calls for this reason. If/when libsrtp settles on a crypto-init hook, consolidating these is a straightforward follow-up.
| { | ||
| psa_hmac_ctx_t *state = (psa_hmac_ctx_t *)statev; | ||
| size_t out_len = 0; | ||
|
|
There was a problem hiding this comment.
Could we add a check like we have:
if (msg_octets < 0) {
return srtp_err_status_bad_param;
}
as done above in the srtp_hmac_mbedtls_update() API?
| python3 -m pip install --break-system-packages --upgrade jsonschema jinja2 | ||
| git clone https://github.com/Mbed-TLS/mbedtls.git /tmp/mbedtls4-src | ||
| cd /tmp/mbedtls4-src | ||
| git checkout mbedtls-4.0.0 |
There was a problem hiding this comment.
Is there any specific reason to use mbedtls-4.0.0? mbedtls-4.1.0 is available and is the LTS release
| } | ||
|
|
||
| status = | ||
| psa_import_key(&attr, key, key_size_in_bits / 8, &(c->ctx->key_id)); |
There was a problem hiding this comment.
After the key import, it is recommended to reset the key attributes with psa_reset_key_attributes(attr)
There was a problem hiding this comment.
Added psa_reset_key_attributes call
…4.1.0, PSA threading note - hmac compute: reject negative msg_octets (match update()) - aes_icm/aes_gcm: psa_reset_key_attributes after import - CI: build against mbedtls-4.1.0 (LTS) - README: MBEDTLS_THREADING_C note for multi-threaded PSA use
…TLS 4) TF-PSA-Crypto — the crypto backend in ESP-IDF v6 / mbedTLS 4 — rejects an in-place (overlapping src==dst) psa_cipher_update for AES-CTR on non-block-aligned lengths with PSA_ERROR_INVALID_ARGUMENT. SRTP always encrypts in place, so the AES-ICM cipher self-test failed at srtp_init() with srtp_err_status_cipher_fail on IDF v6. Mainline mbedTLS 4.1.0 permits overlapping buffers, so this only surfaces on TF-PSA-Crypto; the standalone ctest did not catch it. Stage the CTR update through a bounded stack buffer so input and output do not overlap. AES-CTR is a stream cipher here (out_len == in_len, no deferred bytes), so processing in fixed-size block-aligned chunks preserves the keystream position across calls. The GCM path (psa_aead_update) is unaffected — verified it handles overlapping buffers at non-aligned lengths.
|
Pushed one more fix ( While integrating this branch as an ESP-IDF v6 component — which ships TF-PSA-Crypto, not mainline mbedTLS — Root cause: the CTR path calls The fix stages the CTR update through a bounded stack buffer so input and output don't overlap. AES-CTR is a stream cipher here ( |
|
LGTM! |
Backport of mbedTLS 4 PSA Crypto. Gated under
#if MBEDTLS_VERSION_MAJOR >= 4; legacy mbedTLS 3 path unchanged.Local: both mbedTLS 3.6.4 and 4.0.0 → 10/10 ctest pass.
Closes #812. cc @pabuhler