crypto/evp: fix double free of tmp_keymgmt in evp_keyexch_init() - #31394
sucloudflare wants to merge 1 commit into
Conversation
openssl-machine
added
the
approval: review pending
t8m
added
branch: master
openssl-machine
added
approval: done
github-actions
Bot
added
the
severity: fips change
|
There is no double-free, the description is bogus:
Yes, that's true.
Yes.
It does not need to,
No, because |
esyr
added
the
hold: discussion
|
Thank you for the detailed review, @esyr. You're correct that within the retry loop, evp_pkey_export_to_provider() sets *tmp_keymgmt to NULL via the pointer argument when it doesn't consume the keymgmt, making the subsequent EVP_KEYMGMT_free(tmp_keymgmt) at the top of the next iteration a safe no-op. The description overstated the issue as a confirmed double-free. |
1dd999a to
8ef8d44
Compare
openssl-machine
added
the
hold: cla required
|
Commit message has been updated to remove the double-free claim. The patch now accurately describes its intent as a consistency fix with the sibling files. Happy to address any remaining concerns. |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
|
CLA has been signed under [email protected]. The automated check does not seem to reflect this. Could @t8m or @nhorman verify manually? |
|
I can't check at the moment, but i can say that this usually happens after a cla is signed because the commit.in the pr contains an author email that doesn't match the one you signed your cla for. Please check that and, if need be, got commit --amend --reset-author your change specifying the proper email then force.push the changes. |
Commit a21f77d added explicit null assignments after EVP_KEYMGMT_free() calls at the err:, legacy:, and success exit paths in kem.c, asymcipher.c and signature.c. The identical exit paths in evp_keyexch_init() (crypto/evp/exchange.c) were not updated at the same time, leaving exchange.c as the only outlier in the family without these null assignments. This patch brings exchange.c into consistency with its sibling files by adding exchange = NULL and tmp_keymgmt = NULL after each free at the exit paths, matching the established pattern from a21f77d. No functional change intended.
8ef8d44 to
2c5c43b
Compare
openssl-machine
removed
the
hold: cla required
|
CLA is now resolved. The patch intent has been updated to reflect a consistency fix rather than a double-free. Happy to address any remaining concerns from @esyr before this moves forward. |
t8m
added
triaged: cleanup
|
IMO it can be merged with the updated commit message after the release. |
Commit a21f77d added explicit null assignments after EVP_KEYMGMT_free() calls at the err:, legacy:, and success exit paths in kem.c, asymcipher.c and signature.c. The identical exit paths in evp_keyexch_init() (crypto/evp/exchange.c) were not updated at the same time, leaving exchange.c as the only outlier in the family without these null assignments. This patch brings exchange.c into consistency with its sibling files by adding exchange = NULL and tmp_keymgmt = NULL after each free at the exit paths, matching the established pattern from a21f77d. No functional change intended. Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> MergeDate: Wed Jun 10 11:20:19 2026 (Merged from #31394)
|
Merged to the labeled branches. Thank you for your contribution! |
Commit a21f77d added explicit null assignments after EVP_KEYMGMT_free() calls at the err:, legacy:, and success exit paths in kem.c, asymcipher.c and signature.c. The identical exit paths in evp_keyexch_init() (crypto/evp/exchange.c) were not updated at the same time, leaving exchange.c as the only outlier in the family without these null assignments. This patch brings exchange.c into consistency with its sibling files by adding exchange = NULL and tmp_keymgmt = NULL after each free at the exit paths, matching the established pattern from a21f77d. No functional change intended. Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> MergeDate: Wed Jun 10 11:21:39 2026 (Merged from #31394)
Commit a21f77d added explicit null assignments after EVP_KEYMGMT_free() calls at the err:, legacy:, and success exit paths in kem.c, asymcipher.c and signature.c. The identical exit paths in evp_keyexch_init() (crypto/evp/exchange.c) were not updated at the same time, leaving exchange.c as the only outlier in the family without these null assignments. This patch brings exchange.c into consistency with its sibling files by adding exchange = NULL and tmp_keymgmt = NULL after each free at the exit paths, matching the established pattern from a21f77d. No functional change intended. Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> MergeDate: Wed Jun 10 11:21:26 2026 (Merged from #31394)
Commit a21f77d added explicit null assignments after EVP_KEYMGMT_free() calls at the err:, legacy:, and success exit paths in kem.c, asymcipher.c and signature.c. The identical exit paths in evp_keyexch_init() (crypto/evp/exchange.c) were not updated at the same time, leaving exchange.c as the only outlier in the family without these null assignments. This patch brings exchange.c into consistency with its sibling files by adding exchange = NULL and tmp_keymgmt = NULL after each free at the exit paths, matching the established pattern from a21f77d. No functional change intended. Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> MergeDate: Wed Jun 10 11:21:08 2026 (Merged from #31394)
Commit a21f77d added explicit null assignments after EVP_KEYMGMT_free() calls at the err:, legacy:, and success exit paths in kem.c, asymcipher.c and signature.c. The identical exit paths in evp_keyexch_init() (crypto/evp/exchange.c) were not updated at the same time, leaving exchange.c as the only outlier in the family without these null assignments. This patch brings exchange.c into consistency with its sibling files by adding exchange = NULL and tmp_keymgmt = NULL after each free at the exit paths, matching the established pattern from a21f77d. No functional change intended. Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> MergeDate: Wed Jun 10 11:21:50 2026 (Merged from #31394)
Commit a21f77d added explicit null assignments after EVP_KEYMGMT_free() calls at the err:, legacy:, and success exit paths in kem.c, asymcipher.c and signature.c. The identical exit paths in evp_keyexch_init() (crypto/evp/exchange.c) were not updated at the same time, leaving exchange.c as the only outlier in the family without these null assignments. This patch brings exchange.c into consistency with its sibling files by adding exchange = NULL and tmp_keymgmt = NULL after each free at the exit paths, matching the established pattern from a21f77d. No functional change intended. Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> MergeDate: Wed Jun 10 11:20:19 2026 (Merged from openssl#31394)
Commit a21f77d added explicit null assignments after EVP_KEYMGMT_free() calls at the err:, legacy:, and success exit paths in kem.c, asymcipher.c and signature.c. The identical exit paths in evp_keyexch_init() (crypto/evp/exchange.c) were not updated at the same time, leaving exchange.c as the only outlier in the family without these null assignments. This patch brings exchange.c into consistency with its sibling files by adding exchange = NULL and tmp_keymgmt = NULL after each free at the exit paths, matching the established pattern from a21f77d. No functional change intended. Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> MergeDate: Wed Jun 10 11:20:19 2026 (Merged from openssl#31394)
7 participants
Commit a21f77d fixed the double free of
tmp_keymgmt in the provider fallback loop for kem, signature and
asymcipher, but the identical pattern in evp_keyexch_init()
(crypto/evp/exchange.c) was missed.
At the top of the retry loop, EVP_KEYEXCH_free(exchange) and
EVP_KEYMGMT_free(tmp_keymgmt) are called before each attempt. When
evp_pkey_export_to_provider() sets tmp_keymgmt to NULL, the guard frees
tmp_keymgmt_tofree (PTR_B) but does not NULL out tmp_keymgmt. On the
next iteration, EVP_KEYMGMT_free(tmp_keymgmt) is called again on the
already-freed PTR_B — a double free.
Fix: add exchange = NULL and tmp_keymgmt = NULL after each free at the
top of the loop, consistent with the fix applied by a21f77d to kem.c,
asymcipher.c and signature.c.
Fixes: a21f77d ("crypto/evp: fix double free of tmp_keymgmt in sig/kem/asym init")
cc @nhorman @t8m