Merge pull request #12 from BitcoinErrorLog/improvements/audit-fixes-and-tests

feat: comprehensive audit fixes, new tests, and production improvements

Author: BitcoinErrorLog

Cargo.lock

@@ -29,6 +29,7 @@ hkdf = "0.12"
 zeroize = "1"
 secrecy = "0.8"
 serde = { version = "1", features = ["derive"] }
+serde_bytes = "0.11"
 serde_json = "1"
 thiserror = "1"
 cfg-if = "1"

Cargo.toml

@@ -327,6 +327,82 @@ Signature = Ed25519.Sign(ed25519_secret, H)
 
 ---
 
+### 5. FFI Boundary (Mobile Applications)
+**Entry Points**:
+- UniFFI-generated bindings for iOS (Swift) and Android (Kotlin/Java)
+- Cross-language function calls
+- Memory ownership transfers
+
+**Threats**:
+- **Memory Safety**: Incorrect memory management across language boundaries
+- **Type Confusion**: Mismatched types between Rust and target language
+- **Resource Leaks**: Unclosed handles or sessions
+- **Panic Propagation**: Rust panics crossing FFI boundary
+
+**Mitigations**:
+- ✅ UniFFI handles memory management automatically (Arc refcounting)
+- ✅ Structured error codes (`NoiseErrorCode` as `i32`) prevent type confusion
+- ✅ No raw pointers exposed to generated bindings
+- ✅ Thread-safe wrappers (`ThreadSafeSessionManager`) for concurrent access
+- ✅ Error codes mapped to platform-specific error types
+
+**Risk**: LOW (UniFFI provides safe abstractions)
+
+**Mobile-Specific Considerations**:
+- **App Suspension**: Sessions must be persisted before app backgrounding
+- **Memory Dumps**: Keys in memory vulnerable if device is compromised
+- **Jailbreak/Root**: Elevated privileges can access process memory
+- **Debugging**: Debuggers can inspect memory (mitigated by release builds)
+
+**Recommendation**: Applications SHOULD:
+- Persist session state before app suspension (use `NoiseManager::save_state()`)
+- Use secure storage for master seeds (iOS Keychain, Android Keystore)
+- Enable `secure-mem` feature on servers (page locking)
+- Clear sensitive data on app termination
+
+---
+
+### 6. Mobile-Specific Threats
+
+#### 6.1 App Lifecycle Attacks
+**Threat**: App suspension/resume can cause session state loss or corruption
+
+**Mitigations**:
+- ✅ `NoiseManager` provides `save_state()` and `restore_state()` methods
+- ✅ Session state is serializable for persistence
+- ✅ Connection status tracking for reconnection logic
+
+**Risk**: LOW (if state is properly persisted)
+
+#### 6.2 Memory Dump Attacks
+**Threat**: Compromised device can dump process memory containing keys
+
+**Mitigations**:
+- ✅ `Zeroizing` reduces key lifetime in memory
+- ✅ Closure-based key access (keys don't escape function scope)
+- ✅ Optional `secure-mem` feature (page locking on supported OSes)
+- ❌ Cannot fully protect against root/admin access
+
+**Risk**: MEDIUM (requires device compromise)
+
+**Recommendation**: Use secure hardware storage (HSM, TEE) for master seeds in high-security deployments
+
+#### 6.3 Platform-Specific Threats
+
+**iOS**:
+- **Jailbreak Detection**: Jailbroken devices have elevated attack surface
+- **Keychain Access**: Secure storage via iOS Keychain (recommended)
+- **App Sandbox**: Provides isolation but keys still in process memory
+
+**Android**:
+- **Root Detection**: Rooted devices can access all app memory
+- **Keystore**: Hardware-backed key storage available on modern devices
+- **Debugging**: Release builds obfuscate but don't fully protect
+
+**Risk**: MEDIUM (platform-dependent)
+
+---
+
 ## Cryptographic Assumptions
 
 ### Standard Assumptions (Accepted)
@@ -347,6 +423,39 @@ Signature = Ed25519.Sign(ed25519_secret, H)
 
 ---
 
+## FFI Boundary Security
+
+### Trust Model
+The FFI layer (UniFFI) is considered **TRUSTED** for memory safety but **UNTRUSTED** for application logic:
+- ✅ UniFFI-generated code is safe (no manual memory management)
+- ⚠️ Application code calling FFI must handle errors correctly
+- ⚠️ Platform-specific code (Swift/Kotlin) must validate inputs
+
+### Error Handling
+FFI errors are mapped to structured error codes:
+- `NoiseErrorCode` enum provides platform-agnostic error types
+- Errors are serialized as `i32` for cross-language compatibility
+- Platform bindings should map these to native error types
+
+### Memory Safety
+- **Ownership**: UniFFI uses `Arc` for shared ownership (automatic refcounting)
+- **Lifetimes**: No manual lifetime management required
+- **Leaks**: Automatic cleanup when objects are dropped
+
+### Thread Safety
+- `ThreadSafeSessionManager` uses `Arc<Mutex<>>` for concurrent access
+- FFI layer is thread-safe if internal types are thread-safe
+- Mobile apps should use thread-safe wrappers for background workers
+
+### Security Best Practices for FFI
+1. **Input Validation**: Validate all inputs from platform code
+2. **Error Handling**: Never ignore FFI errors
+3. **Resource Management**: Ensure sessions are properly closed
+4. **State Persistence**: Save state before app suspension
+5. **Secure Storage**: Use platform secure storage for master seeds
+
+---
+
 ## Known Limitations
 
 ### 1. No Post-Quantum Cryptography

THREAT_MODEL.md

@@ -1,3 +1,4 @@
 fn main() {
     uniffi_build::generate_scaffolding("src/pubky_noise.udl").unwrap();
+    println!("cargo::rustc-check-cfg=cfg(loom)");
 }

build.rs

@@ -10,6 +10,8 @@ cargo-fuzz = true
 [dependencies]
 libfuzzer-sys = "0.4"
 arbitrary = { version = "1", features = ["derive"] }
+sha2 = "0.10"
+zeroize = "1"
 
 [dependencies.pubky-noise]
 path = ".."

fuzz/Cargo.toml

@@ -93,17 +93,24 @@ fuzz_target!(|input: HandshakeInput| {
     );
 
     // Should succeed with valid inputs
-    if let Ok((hs_state, first_msg)) = handshake_result {
-        // Test server processing valid message
-        let server_result = pubky_noise::datalink_adapter::server_accept_ik(&server, &first_msg);
+    if let Ok((_hs_state, first_msg)) = handshake_result {
+        // Test server processing valid message using 3-step handshake
+        let server_result = server.build_responder_read_ik(&first_msg);
         // Server should be able to process a properly formed message
         // (though verification may fail due to dummy signatures)
-        let _ = server_result;
+        if let Ok((mut hs, _identity)) = server_result {
+            // Generate response
+            let mut response = vec![0u8; 128];
+            if let Ok(n) = hs.write_message(&[], &mut response) {
+                response.truncate(n);
+                let _ = pubky_noise::datalink_adapter::server_complete_ik(hs);
+            }
+        }
     }
 
     // Test 2: Server handling malformed/arbitrary message
     // This should NOT panic, just return an error
-    let malformed_result = pubky_noise::datalink_adapter::server_accept_ik(&server, &input.malformed_message);
+    let malformed_result = server.build_responder_read_ik(&input.malformed_message);
 
     // We expect an error for malformed messages, but no panic
     match malformed_result {

fuzz/fuzz_targets/fuzz_handshake.rs

@@ -4,7 +4,7 @@ use arbitrary::Arbitrary;
 use libfuzzer_sys::fuzz_target;
 use pubky_noise::{NoiseClient, NoiseServer, RingKeyProvider, NoiseError};
 use pubky_noise::datalink_adapter::{
-    client_start_ik_direct, client_complete_ik, server_accept_ik, server_complete_ik,
+    client_start_ik_direct, client_complete_ik, server_complete_ik,
 };
 use std::sync::Arc;
 
@@ -74,10 +74,18 @@ fuzz_target!(|input: NoiseLinkInput| {
         Err(_) => return,
     };
 
-    let (s_hs, _identity, response) = match server_accept_ik(&server, &first_msg) {
+    let (mut s_hs, _identity) = match server.build_responder_read_ik(&first_msg) {
         Ok(result) => result,
         Err(_) => return,
     };
+    
+    // Generate response message
+    let mut response = vec![0u8; 128];
+    let n = match s_hs.write_message(&[], &mut response) {
+        Ok(n) => n,
+        Err(_) => return,
+    };
+    response.truncate(n);
 
     let mut client_link = match client_complete_ik(c_hs, &response) {
         Ok(link) => link,

fuzz/fuzz_targets/fuzz_noise_link.rs

@@ -33,7 +33,7 @@ impl From<FfiConnectionStatus> for ConnectionStatus {
 }
 
 /// FFI-safe mobile configuration wrapper
-#[derive(uniffi::Record)]
+#[derive(uniffi::Record, Clone)]
 pub struct FfiMobileConfig {
     pub auto_reconnect: bool,
     pub max_reconnect_attempts: u32,

src/ffi/types.rs

@@ -101,7 +101,7 @@ pub fn make_binding_message(params: &BindingMessageParams<'_>) -> [u8; 32] {
     if let Some(r) = params.remote_noise_pub {
         h.update(r);
     }
-    h.update(&INTERNAL_EPOCH.to_le_bytes());
+    h.update(INTERNAL_EPOCH.to_le_bytes());
     h.update(match params.role {
         Role::Client => b"client",
         Role::Server => b"server",

src/identity_payload.rs

@@ -19,6 +19,8 @@
 //! let server = NoiseServer::<_, ()>::new_direct("server_kid", b"server_device", server_ring);
 //! ```
 
+#![allow(unpredictable_function_pointer_comparisons)]
+
 pub mod client;
 pub mod datalink_adapter;
 pub mod errors;
@@ -60,5 +62,4 @@ pub use transport::NoiseTransport;
 
 // UniFFI setup - must be at crate root for proc macros to work
 #[cfg(feature = "uniffi_macros")]
-#[allow(clippy::fn_address_comparisons)]
 uniffi::setup_scaffolding!();