[crypto] Hold OpenSSL objects in an owning handle in the OpenSSL P256 PAL + double-free fix - #73455
Open
Alami-Amine wants to merge 1 commit into
Open
Conversation
… PAL _create_evp_key_from_binary_p256_key and P256Keypair::ECDH_derive_secret released their OpenSSL objects through trailing goto-exit blocks. Add ScopedOpenSSLObject to CHIPCryptoPALOpenSSL.h and use it in both, which removes the cleanup blocks and makes the helper assign its out-parameter once, on success. The helper's out-parameter becomes a reference to the handle. Assigning to a handle releases whatever it held, so the null-out-parameter precondition and its CHIP_ERROR_INVALID_ARGUMENT path are no longer needed. ECDH_derive_secret keeps its exit label because SSLErrorLog() runs on every path, but the block is now only that call and the return. Add TestECDH_RemotePublicKeyNotOnCurve, the suite's first negative ECDH case, covering the remote-key decode failure path.
Alami-Amine
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR strengthens OpenSSL-backed P-256 ECDH handling by adopting RAII wrappers for OpenSSL heap objects and adding a regression test that exercises the failure path for an invalid remote public key (not on the curve), ensuring no secret is produced and allocations are cleaned up.
Changes:
- Added a new unit test covering ECDH with a remote public key that decodes to a point not on the P-256 curve.
- Refactored OpenSSL object lifetime management in ECDH/key decoding to use scoped RAII wrappers.
- Introduced generic scoped OpenSSL handle types in the OpenSSL crypto PAL header.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/crypto/tests/TestChipCryptoPAL.cpp | Adds a negative ECDH test to validate cleanup/no-secret on invalid remote public key input. |
| src/crypto/P256KeyPairOpenSSL.cpp | Reworks ECDH and key decoding to use scoped OpenSSL handles and reduce manual free paths. |
| src/crypto/CHIPCryptoPALOpenSSL.h | Introduces reusable scoped RAII handle templates/aliases for OpenSSL types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+86
to
+88
| VerifyOrReturnError(EC_POINT_oct2point(group.get(), point.get(), Uint8::to_const_uchar(key), key.Length(), nullptr) == 1, | ||
| CHIP_ERROR_INTERNAL); | ||
| VerifyOrReturnError(EC_KEY_set_public_key(ec_key.get(), point.get()) == 1, CHIP_ERROR_INTERNAL); |
Comment on lines
+80
to
+84
| ScopedEcGroup group(EC_GROUP_new_by_curve_name(nid)); | ||
| VerifyOrReturnError(group, CHIP_ERROR_INTERNAL); | ||
|
|
||
| *out_evp_pkey = EVP_PKEY_new(); | ||
| VerifyOrExit(*out_evp_pkey != nullptr, error = CHIP_ERROR_INTERNAL); | ||
| ScopedEcPoint point(EC_POINT_new(group.get())); | ||
| VerifyOrReturnError(point, CHIP_ERROR_INTERNAL); |
|
PR #73455: Size comparison from cad0b20 to ee524fc Full report (23 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #73455 +/- ##
==========================================
+ Coverage 56.11% 56.16% +0.04%
==========================================
Files 1817 1818 +1
Lines 117720 117858 +138
Branches 13885 13898 +13
==========================================
+ Hits 66064 66194 +130
- Misses 51656 51664 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Summary
This change was motivated by a double-free in the helper's error path; the RAII conversion is how it is fixed
Replaces the
goto exitcleanup blocks in_create_evp_key_from_binary_p256_keyandP256Keypair::ECDH_derive_secretwith an owning handle.Problem
exit:block, putting correctness in code that ordinary tests never execute.out_evp_pkey = nullptr— the local parameter rather than*out_evp_pkey— so ifEVP_PKEY_set1_EC_KEY()failed afterEVP_PKEY_new()succeeded, the caller kept a pointer the helper had already released and released it again: a double free. It also used*out_evp_pkey != nullptras a proxy for "the helper allocated this", which is the condition its own precondition rejects, so a caller passing a non-empty out-parameter receivedCHIP_ERROR_INVALID_ARGUMENTand had its object released.EC_POINT_oct2point, before theEVP_PKEYexists.Solution
ScopedOpenSSLObject<T, Free>toCHIPCryptoPALOpenSSL.h:std::unique_ptrwith a stateless deleter and a private base, aliased per (type, free function) pair.Freeis a template parameter rather than derived fromTbecause types such asEC_POINThave both a plain and a zeroizing free, and the correct one depends on whether the object held secret material.ECDH_derive_secretholds all four of its OpenSSL objects in handles. Itsexit:label remains only becauseSSLErrorLog()runs on every path.Caveats
CHIP_ERROR_INVALID_ARGUMENT; its only caller did not distinguish it.ScopedOpenSSLObjectcan only name avoid (T *)cleanup, sosk_X509_freeandsk_<TYPE>_pop_freeelsewhere inCHIPCryptoPALOpenSSL.cppare unchanged.Testing
TestECDH_RemotePublicKeyNotOnCurveinsrc/crypto/tests/TestChipCryptoPAL.cpp: a correctly sized SEC1 uncompressed point that is not on P-256 must return an error without_secretlength still zero. This is the suite's first negative ECDH case.