Issue 79: Detect use-after-free in sa_invoke#80
Issue 79: Detect use-after-free in sa_invoke#80riwoh wants to merge 3 commits intordkcentral:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses Issue 79 by preventing undefined behavior in sa_invoke when it is called with a dangling Sec_ProcessorHandle*, by validating that the handle is still registered before accessing/locking it.
Changes:
- Register newly created
Sec_ProcessorHandlein the globalprocessorHandleListearlier during initialization. - Add a
processorHandleListmembership check insa_invokebefore locking/accessing handle fields. - Adjust locking order in
sa_invoketo hold the global list mutex while acquiring the per-handle mutex.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Validate that processorHandle is still in the global processorHandleList before accessing its fields. This prevents undefined behavior when sa_invoke is called with a dangling pointer to a freed handle. Signed-off-by: jhewit200 <joseph_hewitt@cable.comcast.com>
riwoh
force-pushed
the
issue-79-use-after-free-sa-invoke
branch
from
e971871 to
d789e69
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mhabrat
left a comment
mhabrat
left a comment
There was a problem hiding this comment.
With the potential race condition addressed in a future PR.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Attempt to use the freed handle - sa_invoke should detect this and return failure | ||
| Sec_Result result = SecKey_Generate(dangling, SEC_OBJECTID_USER_BASE, SEC_KEYTYPE_AES_128, | ||
| SEC_STORAGELOC_RAM); | ||
| if (result == SEC_RESULT_SUCCESS) { | ||
| SEC_LOG_ERROR("SecKey_Generate should have failed on a freed handle"); | ||
| return SEC_RESULT_FAILURE; |
There was a problem hiding this comment.
This unit test will pass even if SecKey_Generate fails for unrelated reasons (e.g., unsupported SA backend), because it only asserts that generation fails after release. To make the test actually validate the regression, consider first calling SecKey_Generate(proc, ...) (or another sa_invoke-based operation) and asserting it succeeds before releasing, then repeat the call with the dangling pointer and assert it fails.
Validate that processorHandle is still in the global processorHandleList before accessing its fields. This prevents undefined behavior when sa_invoke is called with a dangling pointer to a freed handle.