Secure-store overwrite keeps a stale keyring entry bound to the alias

[fnando]

002: Secure-store overwrite keeps a stale keyring entry bound to the alias

Date: 2026-04-17
Severity: Medium
Impact: signing with stale or attacker-chosen secure-store key
Subsystem: keys
Final review by: gpt-5.4, high

Summary

When a secure-store entry for an identity name already exists, stellar keys add --secure-store and stellar keys generate --secure-store do not refresh that entry. StellarEntry::write() only emits a warning, but the command still succeeds and writes or overwrites the identity file so that later address resolution and signing continue to use the old secure-store secret.

This is not just a cosmetic warning. The CLI reports success and persists the alias as though the requested seed phrase was stored, so a user rotating a key can remain bound to stale key material, and a same-user pre-seeded secure-store entry can steer future operations onto attacker-chosen keys.

Root Cause

signer::keyring::StellarEntry::write() treats an existing secure-store entry as a warning-only condition. If get_public_key(None) succeeds, it skips set_seed_phrase() entirely and still returns Ok(()).

signer::secure_store::save_secret() propagates that success-shaped result and returns the secure-store entry name unconditionally. Both commands::keys::add::Cmd::read_secret() and commands::keys::generate::Cmd::secret() then serialize that returned reference into the identity file, and later Secret::public_key() / SignerKind::SecureStore resolve back through the same stale entry for address lookup and signing.

Reproduction

On a system where secure-store support is enabled, create or retain a secure-store entry for an alias such as alice, then run either stellar keys generate alice --secure-store --overwrite or stellar keys add alice --secure-store --overwrite with different key material.

The command warns that a secure-store key already exists but still succeeds and overwrites the identity file. Subsequent stellar keys address alice and signing operations resolve through the pre-existing secure-store entry, not through the newly supplied or generated seed phrase.

Affected Code

• stellar-cli/cmd/soroban-cli/src/signer/keyring.rs:34-47 — write() warns on existing entries and skips the only state-changing call, set_seed_phrase().
• stellar-cli/cmd/soroban-cli/src/signer/secure_store.rs:44-55 — save_secret() always returns the secure-store entry name after write(), even if nothing was updated.
• stellar-cli/cmd/soroban-cli/src/commands/keys/add.rs:84-100 — secure-store imports accept the returned entry reference without verifying that the OS keyring changed.
• stellar-cli/cmd/soroban-cli/src/commands/keys/generate.rs:73-83,115-124 — --overwrite only governs the identity file; the secure-store entry is reused unchanged.
• stellar-cli/cmd/soroban-cli/src/config/secret.rs:129-158 — address resolution and signer construction for Secret::SecureStore read from the stored entry name, so the stale key is used for later operations.

PoC

• Target test file: cmd/soroban-cli/src/signer/keyring.rs
• Test name: test_write_silently_skips_update_on_existing_entry
• Test language: Rust
• How to run: Append the test body below to the target test file, then build and run.

Test Body

#[test]
fn test_write_silently_skips_update_on_existing_entry() {
    // PoC for H002: StellarEntry::write silently discards a new seed phrase
    // when the entry already exists, returning Ok(()) without updating the keyring.
    set_default_credential_builder(mock::default_credential_builder());

    // Generate two distinct seed phrases from different seeds
    let seed_phrase_1 =
        crate::config::secret::seed_phrase_from_seed(Some("0123456789abcdef")).unwrap();
    let seed_phrase_2 =
        crate::config::secret::seed_phrase_from_seed(Some("fedcba9876543210")).unwrap();

    // Record each phrase's expected public key at default hd_path
    let pubkey_1 = seed_phrase_1
        .from_path_index(0, None)
        .unwrap()
        .public()
        .0;
    let pubkey_2 = seed_phrase_2
        .from_path_index(0, None)
        .unwrap()
        .public()
        .0;

    // Sanity: the two seed phrases must produce different public keys
    assert_ne!(pubkey_1, pubkey_2, "test setup error: seeds must differ");

    let entry = StellarEntry::new("poc-stale-keyring").unwrap();
    let print_handle = print::Print::new(true);

    // First write succeeds and stores seed_phrase_1
    let result_1 = entry.write(seed_phrase_1, &print_handle);
    assert!(result_1.is_ok(), "first write must succeed");

    // Second write with a DIFFERENT seed phrase — this should update,
    // but the bug causes it to silently skip the update.
    let result_2 = entry.write(seed_phrase_2, &print_handle);
    assert!(
        result_2.is_ok(),
        "second write returns Ok even though it did NOT update the keyring"
    );

    // The keyring still holds the FIRST seed phrase's key material
    let stored_pubkey = entry.get_public_key(None).unwrap().0;

    // BUG DEMONSTRATION: stored key matches the OLD seed phrase, not the new one
    assert_eq!(
        stored_pubkey, pubkey_1,
        "keyring still holds the FIRST key — write was silently skipped"
    );
    assert_ne!(
        stored_pubkey, pubkey_2,
        "the second seed phrase was silently discarded"
    );
}

Expected vs Actual Behavior

• Expected: A secure-store overwrite should either replace the existing secure-store secret or fail without writing an identity alias that implies success.
• Actual: The secure-store write path returns success after a warning-only no-op, and the alias continues to resolve to the old secure-store secret.

Adversarial Review

1. Exercises claimed bug: YES — the canonical test proves that StellarEntry::write() returns success while leaving an existing entry unchanged, and code tracing from save_secret() through the keys commands shows the CLI persists that stale reference unchanged.
2. Realistic preconditions: YES — same-name secure-store entries naturally arise during key rotation/re-import, and --overwrite is the documented mechanism users reach for when replacing an identity.
3. Bug vs by-design: BUG — the command advertises overwrite semantics for the identity, reports success, and provides no documentation that secure-store writes intentionally become warning-only no-ops.
4. Final severity: Medium — the issue does not expose secrets directly, but it can leave future address resolution and signing bound to stale or same-user attacker-preseeded key material after a successful overwrite flow.
5. In scope: YES — the behavior is concrete, code-reachable through supported CLI options, and does not require privileged machine access.
6. Test correctness: CORRECT — the PoC targets the decisive helper branch. A reviewer-written higher-level mock test was discarded because the keyring mock backend is explicitly EntryOnly and cannot model persistence across fresh Entry::new(...) calls, so the helper test plus source trace is the correct harness here.
7. Alternative explanations: NONE — the warning itself does not explain away the finding because the code still returns Ok(()), writes the alias, and later resolves that alias back through the unchanged secure-store entry.
8. Novelty: NOVEL

Suggested Fix

Make secure-store collisions fail closed or support explicit replacement. Concretely, StellarEntry::write() should return an error when an entry already exists unless the caller explicitly requests replacement, and the keys --overwrite flow should either refresh the secure-store entry first or abort before writing the identity file if the secure-store update did not occur.