SecKeychainItem::delete() discards the OSStatus, so a failed keychain deletion is indistinguishable from success

Open Beginner friendly
#256 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
78/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Quiet
Tech stack
macos, rust

Research direction

Start in src/os/macos/passwords.rs at SecKeychainItem::delete(), then compare it with set_password() and the other deletion paths in src/item.rs and src/passwords.rs. Use the existing cvt helper from src/lib.rs and verify that deletion failures are returned to callers, choosing either the breaking signature change or the additive try_delete approach described in the issue.

Written by the indexing model from the issue text.

Description

Summary

SecKeychainItem::delete() in src/os/macos/passwords.rs calls SecKeychainItemDelete and discards the returned OSStatus, so a caller cannot tell a successful deletion from a failed one.

/// Delete this item from its keychain
#[inline]
pub fn delete(self) {
    unsafe {
        SecKeychainItemDelete(self.as_concrete_TypeRef());
    }
}

(security-framework 3.7.0, src/os/macos/passwords.rs:79-85)

The underlying API does report failure — security-framework-sys declares it correctly:

#[cfg(target_os = "macos")]
pub fn SecKeychainItemDelete(itemRef: SecKeychainItemRef) -> OSStatus;

So the status is available and is being dropped on the floor.

Why this looks like an oversight rather than a decision

The method directly above it in the same impl SecKeychainItem block does exactly the right thing with the same pattern:

pub fn set_password(&mut self, password: &[u8]) -> Result<()> {
    unsafe {
        cvt(SecKeychainItemModifyAttributesAndData(
            self.as_concrete_TypeRef(),
            ptr::null(),
            password.len() as u32,
            password.as_ptr().cast(),
        ))?;
    }
    Ok(())
}

And the crate's other two deletion paths also check their status:

  • ItemSearchOptions::delete()cvt(unsafe { SecItemDelete(...) }) (src/item.rs:534)
  • delete_generic_password_options()cvt(unsafe { SecItemDelete(...) }) (src/passwords.rs:77)

cvt is already available at src/lib.rs:51. This one method is the outlier.

Impact

This is a silent data-integrity problem for anything doing credential revocation, because the failure is invisible rather than noisy.

I found it from the other end. Tracing why a "log out" path in our application reported success while the secret stayed in the login Keychain, the discarded status turned out to be the first of three layers that erase it:

  1. heredelete() returns (), OSStatus dropped
  2. apple-native-keyring-storeitem.delete(); Ok(()) (filed separately)
  3. @napi-rs/keyring.is_ok() on that Ok -> true

Net effect for a Node consumer on macOS: deleting a credential the OS refuses to delete resolves true. Not an error, not even false. Every downstream layer is doing something locally reasonable with information that was already destroyed at step 1, so no fix further up can recover it.

Suggested fix

/// Delete this item from its keychain
#[inline]
pub fn delete(self) -> Result<()> {
    cvt(unsafe { SecKeychainItemDelete(self.as_concrete_TypeRef()) })
}

This is a breaking change to a public signature, so it presumably wants the next major. If that is a problem, an additive try_delete(self) -> Result<()> with delete() deprecated in its favour would also unblock downstream crates without forcing a major.

I am happy to send either as a PR — just say which shape you'd prefer.

Notes

  • I have not attempted to judge whether the deprecated SecKeychain* family should be migrated to SecItemDelete here; that seems like a separate and larger question, and the one-line status check is useful regardless.
  • Verified against security-framework 3.7.0 / security-framework-sys 2.17.0 on aarch64-apple-darwin.
Dominant language
Rust
Stars
304
Forks
105
PR merge metrics
No merged PRs in 30d

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from kornelski/rust-security-framework

All issues in kornelski/rust-security-framework

Similar issues

More Rust issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.