cashubtc/cdk

Fork 0

Secret Memory Leakage Due to Extensive Cloning #979

New issue

Closed

opened 2025-08-20 03:03:13 +00:00 by vnprc · 1 comment

vnprc commented

2025-08-20 03:03:13 +00:00

(Migrated from github.com)

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

Secret generation: Secret::generate() creates a secret
Immediate cloning: Secret gets moved into Proof structs via .clone()
Multiple copies: Proof conversions (ProofV3 ↔ ProofV4 ↔ Proof) clone secrets again
Collection operations: Methods like secrets() clone all secrets in a collection
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

Phase 1: Implement new SecureSecret type alongside existing Secret
Phase 2: Migrate internal APIs to use SecureSecret
Phase 3: Provide type Secret = SecureSecret; alias in major version
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:

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

This represents a meaningful security improvement without breaking existing APIs.

# 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 commented

2025-08-20 15:57:35 +00:00

(Migrated from github.com)

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.

👍 1

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

cashubtc/cdk#979

No description provided.

Rows
Columns