Secret Memory Leakage Due to Extensive Cloning

[vnprc]

Problem Description

The CDK codebase has a fundamental security issue where cryptographic secrets are copied throughout memory without proper cleanup. The Secret type implements Clone, which creates copies that persist in memory even when the original secret goes out of scope.

Current State Analysis

Secret Cloning Hotspots

The Secret type is cloned extensively throughout the codebase:

// cashu/src/nuts/nut00/mod.rs:433
secret: self.secret.clone(),

// cashu/src/nuts/nut00/mod.rs:506  
secret: self.secret.clone(),

// cashu/src/nuts/nut00/mod.rs:894
self.iter().map(|pm| pm.secret.clone()).collect()

The Fundamental Issue

1. Secret generation: Secret::generate() creates a secret
2. Immediate cloning: Secret gets moved into Proof structs via .clone()
3. Multiple copies: Proof conversions (ProofV3 ↔ ProofV4 ↔ Proof) clone secrets again
4. Collection operations: Methods like secrets() clone all secrets in a collection
5. No cleanup: Cloned copies remain in memory until overwritten by other data

Result: Secret data scattered throughout memory in multiple uncleared copies.

---

Security Impact

• Secrets remain in memory longer than necessary
• Memory dumps could expose secret values
• Swap files may contain uncleared secrets
• Process memory inspection reveals sensitive data

---

Proposed Solution: Reference-Counted Secure Secrets

Implement an Arc<SecretBox> approach where all "clones" share the same underlying secret data.

Implementation

use std::sync::Arc;
use zeroize::{Zeroize, ZeroizeOnDrop};

#[derive(ZeroizeOnDrop)]
struct SecretBox {
    data: String,
}

#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
#[serde(transparent)]
pub struct Secret(Arc<SecretBox>);

impl Secret {
    pub fn generate() -> Self {
        let mut random_bytes = [0u8; 32];
        rand::thread_rng().fill_bytes(&mut random_bytes);
        let secret_data = hex::encode(random_bytes);
        random_bytes.zeroize();

        Secret(Arc::new(SecretBox { data: secret_data }))
    }

    pub fn new<S: Into<String>>(secret: S) -> Self {
        Secret(Arc::new(SecretBox { data: secret.into() }))
    }

    // Existing methods unchanged
    pub fn as_bytes(&self) -> &[u8] {
        self.0.data.as_bytes()
    }

    pub fn to_bytes(&self) -> Vec<u8> {
        self.as_bytes().to_vec()
    }
}

impl fmt::Display for Secret {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "{}", self.0.data)
    }
}

impl FromStr for Secret {
    type Err = Error;
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        Ok(Secret::new(s.to_string()))
    }
}

All existing trait implementations work the same way.

---

Benefits

• ✅ Solves memory leakage: Only one copy of secret data exists in memory
• ✅ Maintains API compatibility: All existing Clone calls continue to work
• ✅ Automatic cleanup: Secret data gets zeroized when last reference is dropped
• ✅ Thread-safe: Arc provides safe sharing across threads
• ✅ Minimal changes: Most code changes are internal implementation details

---

Trade-offs

• ⚠️ Longer lifetime: Secrets persist until last reference drops (vs immediate scope exit)
• ⚠️ Small overhead: Arc reference counting adds minimal performance cost
• ⚠️ Shared state: Multiple handles point to same data (usually not an issue)

---

Migration Strategy

1. Phase 1: Implement new SecureSecret type alongside existing Secret
2. Phase 2: Migrate internal APIs to use SecureSecret
3. Phase 3: Provide type Secret = SecureSecret; alias in major version
4. Phase 4: Remove old implementation after deprecation period

---

Alternative Approaches Considered

• Move-only secrets: Would require massive breaking API changes
• Copy-tracking: Complex and requires unsafe code
• Manual zeroization: Developers would need to remember to clear secrets manually

---

Request for Feedback

Would the maintainers be open to:

1. A PR implementing this approach?
2. The small performance overhead of Arc reference counting?
3. The change in secret lifetime semantics?

This represents a meaningful security improvement without breaking existing APIs.

[vnprc]

In the cdk call we determined that we can skip most of the example code and replace it with a manual drop impl that zeroizes the memory before releasing it. It was also suggested to start with the seed phrase, although I see no reason to skip this safety measure for other secrets.