From e7fb0331b53aab4d70855daccc0128442808af9e Mon Sep 17 00:00:00 2001 From: Hammed Abass Date: Mon, 13 Jul 2026 18:41:43 +0200 Subject: [PATCH 1/2] fix(security): harden provider ACL, tickets, and local secrets Close high-severity findings from the security review: default-deny blob gets for unmapped hashes, map collection members for ACL, fail closed on unknown access modes, require vnd1 tickets only, redact tickets from events, exclude Android app-data backups, and tighten secret-file creation. --- androidApp/src/main/AndroidManifest.xml | 2 + androidApp/src/main/res/xml/backup_rules.xml | 5 + .../main/res/xml/data_extraction_rules.xml | 11 + crates/vnidrop/CORE_FLOW.md | 17 +- crates/vnidrop/src/access_policy.rs | 17 +- crates/vnidrop/src/api.rs | 4 +- crates/vnidrop/src/filesystem.rs | 3 + crates/vnidrop/src/logging.rs | 4 +- crates/vnidrop/src/runtime/facade.rs | 7 +- crates/vnidrop/src/runtime/lifecycle.rs | 10 +- crates/vnidrop/src/runtime/mod.rs | 70 +++++- crates/vnidrop/src/runtime/provider.rs | 211 +++++++++++------- crates/vnidrop/src/runtime/receive.rs | 99 +++----- crates/vnidrop/src/runtime/share.rs | 18 +- crates/vnidrop/src/secret.rs | 36 ++- crates/vnidrop/src/tests/access_policy.rs | 28 +++ crates/vnidrop/src/tests/ticket.rs | 15 +- crates/vnidrop/src/ticket.rs | 67 +++--- crates/vnidrop/src/util.rs | 4 - crates/vnidrop/tests/lifecycle.rs | 50 ++++- .../composeResources/values/strings.xml | 2 +- .../kotlin/com/vnidrop/app/core/CoreModels.kt | 3 +- .../com/vnidrop/app/core/CoreRepository.kt | 3 +- .../app/feature/receive/ReceiveScreen.kt | 4 +- .../vnidrop/app/ui/screens/ScreenSections.kt | 14 +- .../com/vnidrop/app/feature/ViewModelsTest.kt | 35 +-- .../vnidrop/app/ui/state/AppUiModelsTest.kt | 5 +- 27 files changed, 461 insertions(+), 283 deletions(-) create mode 100644 androidApp/src/main/res/xml/backup_rules.xml create mode 100644 androidApp/src/main/res/xml/data_extraction_rules.xml diff --git a/androidApp/src/main/AndroidManifest.xml b/androidApp/src/main/AndroidManifest.xml index f7ba8e4..40dee41 100644 --- a/androidApp/src/main/AndroidManifest.xml +++ b/androidApp/src/main/AndroidManifest.xml @@ -13,6 +13,8 @@ + + + + diff --git a/androidApp/src/main/res/xml/data_extraction_rules.xml b/androidApp/src/main/res/xml/data_extraction_rules.xml new file mode 100644 index 0000000..2e2498d --- /dev/null +++ b/androidApp/src/main/res/xml/data_extraction_rules.xml @@ -0,0 +1,11 @@ + + + + + + + + + + diff --git a/crates/vnidrop/CORE_FLOW.md b/crates/vnidrop/CORE_FLOW.md index 7d91993..a45a9c5 100644 --- a/crates/vnidrop/CORE_FLOW.md +++ b/crates/vnidrop/CORE_FLOW.md @@ -12,24 +12,29 @@ bytes through Kotlin memory. file into `iroh-blobs`, stores a collection, and returns a VniDrop ticket. 3. New VniDrop shares are `ApprovalRequired` by default. A copied ticket is not enough to read bytes until the sender approves the receiver endpoint. -4. The sender observes receiver requests through `CoreEvent` entries with +4. The blob provider is **default-deny**: only hashes registered for an active + share (collection root **and** each member blob) may be served, and only when + access policy allows that remote endpoint. Unknown hashes are refused. +5. The sender observes receiver requests through `CoreEvent` entries with `phase="approval"` and can query them with `list_receiver_requests(transfer_id)`. -5. `respond_receiver_request(request_id, accepted, reason)` accepts or refuses a +6. `respond_receiver_request(request_id, accepted, reason)` accepts or refuses a pending request. Accepted requests create a time-limited access session for the receiver endpoint. +7. Ticket strings are capabilities. Share events emit hash/size metadata only — + never the full ticket payload. ## Receive 1. `receive(ticket, output_dir, receiver_name)` parses and validates the ticket. 2. VniDrop tickets first connect to the handshake ALPN - `/vnidrop/handshake/1` and send `RequestTransfer` metadata to the sender. + `/vnidrop/handshake/2` and send `RequestTransfer` metadata to the sender. 3. If approved, the receiver connects to the blobs ALPN, downloads the collection, and streams files to `output_dir`. 4. If refused, expired, unknown, or cancelled, the receive transfer is marked `failed` or `cancelled` and emits an error/lifecycle event. -5. Legacy raw `BlobTicket` values do not carry VniDrop metadata, so they bypass - the app approval handshake and use the underlying blob ticket directly. +5. Only `vnd1:` VniDrop tickets are accepted. Raw iroh `BlobTicket` strings are + rejected at parse time so receive always runs the approval handshake. ## Core States And Events @@ -77,7 +82,7 @@ bytes through Kotlin memory. ## Blob Retention Policy Stopping a share immediately removes its provider mapping and approval state, -so neither VniDrop nor legacy blob tickets can read it. Physical blob chunks are +so outstanding VniDrop tickets can no longer download content. Physical blob chunks are not force-deleted at stop time because content-addressed chunks may be shared by another active collection. They remain eligible for the blob store's garbage collection. Restart reconciliation never restores a stopped share. diff --git a/crates/vnidrop/src/access_policy.rs b/crates/vnidrop/src/access_policy.rs index 77ae471..1584dfe 100644 --- a/crates/vnidrop/src/access_policy.rs +++ b/crates/vnidrop/src/access_policy.rs @@ -63,16 +63,13 @@ impl AccessPolicy { transfer_id: u64, endpoint_id: Option<&str>, ) -> AccessDecision { - match self - .modes - .read() - .await - .get(&transfer_id) - .cloned() - .unwrap_or(TransferAccessMode::Public) - { - TransferAccessMode::Public => AccessDecision::Allow, - TransferAccessMode::ApprovalRequired => { + // Unknown transfers fail closed. Never treat a missing mode as Public. + match self.modes.read().await.get(&transfer_id).cloned() { + None => AccessDecision::Deny { + reason: "unknown-transfer", + }, + Some(TransferAccessMode::Public) => AccessDecision::Allow, + Some(TransferAccessMode::ApprovalRequired) => { let Some(endpoint_id) = endpoint_id else { return AccessDecision::Deny { reason: "missing-endpoint-id", diff --git a/crates/vnidrop/src/api.rs b/crates/vnidrop/src/api.rs index f7ad1b7..aa24a07 100644 --- a/crates/vnidrop/src/api.rs +++ b/crates/vnidrop/src/api.rs @@ -183,7 +183,6 @@ pub struct StoredTransfer { pub struct ShareResult { pub transfer_id: u64, pub ticket: String, - pub blob_ticket: String, pub hash: String, pub transfer_name: String, pub file_count: u64, @@ -227,8 +226,7 @@ impl TransferMetadata { #[derive(Debug, Clone, Serialize, Deserialize, uniffi::Record)] pub struct TicketInspection { pub kind: String, - pub blob_ticket: String, - pub metadata: Option, + pub metadata: TransferMetadata, } #[derive(Debug, Clone, Serialize, Deserialize, uniffi::Record)] diff --git a/crates/vnidrop/src/filesystem.rs b/crates/vnidrop/src/filesystem.rs index 0d3c9a6..88088db 100644 --- a/crates/vnidrop/src/filesystem.rs +++ b/crates/vnidrop/src/filesystem.rs @@ -27,6 +27,9 @@ const STALE_PART_AGE: Duration = Duration::from_secs(24 * 60 * 60); pub(crate) struct TransferImport { pub(crate) tag: TempTag, pub(crate) root_hash: Hash, + /// Per-file content hashes in the collection. Provider ACL maps these too + /// so child blob gets are not fail-open when only the root is tracked. + pub(crate) member_hashes: Vec, pub(crate) total_size: u64, pub(crate) file_count: u64, pub(crate) default_name: String, diff --git a/crates/vnidrop/src/logging.rs b/crates/vnidrop/src/logging.rs index 975715e..7b34df7 100644 --- a/crates/vnidrop/src/logging.rs +++ b/crates/vnidrop/src/logging.rs @@ -23,8 +23,10 @@ pub(crate) fn init_logging(app_data_dir: &Path) -> Result<()> { fs::create_dir_all(&log_dir)?; let writer = SizeRotatingWriter::new(log_dir, MAX_LOG_BYTES, MAX_LOG_FILES); let (writer, guard) = tracing_appender::non_blocking(writer); + // Default to info so ticket-adjacent and endpoint noise is not retained at + // debug volume in app logs. Operators can raise with RUST_LOG. let filter = EnvFilter::try_from_default_env() - .unwrap_or_else(|_| EnvFilter::new("vnidrop=debug,iroh=info,iroh_blobs=info,warn")); + .unwrap_or_else(|_| EnvFilter::new("vnidrop=info,iroh=info,iroh_blobs=info,warn")); let subscriber = tracing_subscriber::registry() .with(filter) diff --git a/crates/vnidrop/src/runtime/facade.rs b/crates/vnidrop/src/runtime/facade.rs index 33b234f..1519440 100644 --- a/crates/vnidrop/src/runtime/facade.rs +++ b/crates/vnidrop/src/runtime/facade.rs @@ -224,12 +224,7 @@ impl VnidropCore { .context("failed to parse transfer ticket") .map_err(VnidropError::ticket)?; Ok(TicketInspection { - kind: if parsed.metadata.is_some() { - "vnidrop".to_string() - } else { - "legacy".to_string() - }, - blob_ticket: parsed.blob_ticket.to_string(), + kind: "vnidrop".to_string(), metadata: parsed.metadata, }) } diff --git a/crates/vnidrop/src/runtime/lifecycle.rs b/crates/vnidrop/src/runtime/lifecycle.rs index e7443c5..0e2e246 100644 --- a/crates/vnidrop/src/runtime/lifecycle.rs +++ b/crates/vnidrop/src/runtime/lifecycle.rs @@ -51,10 +51,7 @@ impl CoreInner { .await?; active_shares.remove(&transfer_id); drop(active_shares); - self.hash_to_transfer - .lock() - .await - .retain(|_, id| *id != transfer_id); + self.unregister_transfer_hashes(transfer_id).await; self.access_policy.remove_transfer(transfer_id).await; self.emit_transfer(transfer_id, "send", "lifecycle", "share-stopped", json!({})); return Ok(()); @@ -106,10 +103,7 @@ impl CoreInner { } self.active_shares.lock().await.remove(&transfer_id); - self.hash_to_transfer - .lock() - .await - .retain(|_, id| *id != transfer_id); + self.unregister_transfer_hashes(transfer_id).await; self.access_policy.remove_transfer(transfer_id).await; // Events are persisted asynchronously. Drain events emitted before this // request so none can be written back after the transfer is deleted. diff --git a/crates/vnidrop/src/runtime/mod.rs b/crates/vnidrop/src/runtime/mod.rs index 14174b8..75d8e05 100644 --- a/crates/vnidrop/src/runtime/mod.rs +++ b/crates/vnidrop/src/runtime/mod.rs @@ -16,7 +16,7 @@ mod share; pub use facade::VnidropCore; use std::{ - collections::HashMap, + collections::{HashMap, HashSet}, path::PathBuf, str::FromStr, sync::{atomic::AtomicBool, Arc}, @@ -68,7 +68,9 @@ pub(super) struct CoreInner { // Restored shares have no in-memory tag, but remain tracked so they can be // counted and explicitly revoked after a restart. pub(super) active_shares: TokioMutex>>, - pub(super) hash_to_transfer: TokioMutex>, + /// Content hash → active share transfer ids (root and collection members). + /// Multiple transfers can share the same content-addressed hash. + pub(super) hash_to_transfer: TokioMutex>>, pub(super) connection_endpoints: TokioMutex>, pub(super) provider_task: TokioMutex>>, pub(super) shutdown_started: AtomicBool, @@ -130,18 +132,12 @@ impl CoreInner { let access_policy = AccessPolicy::new(); // Restore share ownership and access mode before the router can serve // any request. Unknown persisted modes fail closed in mode_from_storage. - let mut restored_hashes = HashMap::new(); + // Register root + every collection member so child gets stay under ACL. + let mut restored_hashes: HashMap> = HashMap::new(); let mut restored_active_shares = HashMap::new(); for share in repository.list_active_shares().await? { let transfer_id = share.transfer_id; - let valid_root = match Hash::from_str(&share.content_hash) { - Ok(hash) => { - store.blobs().has(hash).await.unwrap_or(false) - && Collection::load(hash, store.as_ref()).await.is_ok() - } - Err(_) => false, - }; - if !valid_root { + let Ok(root_hash) = Hash::from_str(&share.content_hash) else { repository .transition_transfer_status( transfer_id, @@ -157,8 +153,39 @@ impl CoreInner { json!({ "content_hash": share.content_hash }), ); continue; + }; + let collection = if store.blobs().has(root_hash).await.unwrap_or(false) { + Collection::load(root_hash, store.as_ref()).await.ok() + } else { + None + }; + let Some(collection) = collection else { + repository + .transition_transfer_status( + transfer_id, + TransferStatus::Sharing, + TransferStatus::Failed, + ) + .await?; + event_hub.emit_transfer( + transfer_id, + TransferDirection::Send.as_str(), + "recovery", + "share-root-missing-or-corrupt", + json!({ "content_hash": share.content_hash }), + ); + continue; + }; + restored_hashes + .entry(root_hash.to_string()) + .or_default() + .insert(transfer_id); + for (_, member_hash) in collection.iter() { + restored_hashes + .entry(member_hash.to_string()) + .or_default() + .insert(transfer_id); } - restored_hashes.insert(share.content_hash, transfer_id); restored_active_shares.insert(transfer_id, None); access_policy .set_mode(transfer_id, mode_from_storage(&share.access_mode)) @@ -224,6 +251,25 @@ impl CoreInner { .emit_transfer(transfer_id, direction, phase, kind, data); } + pub(super) async fn register_share_hashes( + &self, + transfer_id: u64, + hashes: impl IntoIterator, + ) { + let mut map = self.hash_to_transfer.lock().await; + for hash in hashes { + map.entry(hash.to_string()).or_default().insert(transfer_id); + } + } + + pub(super) async fn unregister_transfer_hashes(&self, transfer_id: u64) { + let mut map = self.hash_to_transfer.lock().await; + map.retain(|_, transfers| { + transfers.remove(&transfer_id); + !transfers.is_empty() + }); + } + pub(super) async fn list_events(&self, transfer_id: Option) -> Result> { self.event_hub.flush().await; self.repository diff --git a/crates/vnidrop/src/runtime/provider.rs b/crates/vnidrop/src/runtime/provider.rs index 7f50751..d6263fa 100644 --- a/crates/vnidrop/src/runtime/provider.rs +++ b/crates/vnidrop/src/runtime/provider.rs @@ -71,41 +71,37 @@ impl CoreInner { ); } ProviderMessage::GetRequestReceived(message) => { - let transfer_id = self.transfer_for_hash(message.inner.request.hash).await; - if let Some(transfer_id) = transfer_id { - let decision = self - .access_decision(transfer_id, message.inner.connection_id) - .await; - if let AccessDecision::Deny { reason } = decision { - self.emit_transfer( + match self + .authorize_hash(message.inner.request.hash, message.inner.connection_id) + .await + { + Ok(transfer_id) => { + self.track_request_updates( transfer_id, - "send", - "access", - "request-denied", - json!({ - "connection_id": message.inner.connection_id, - "request_id": message.inner.request_id, - "reason": reason, - }), + message.inner.connection_id, + message.inner.request_id, + message.rx, + ) + .await; + let _ = message.tx.send(Ok(())).await; + } + Err(reason) => { + self.emit_denied_request( + message.inner.connection_id, + message.inner.request_id, + reason, ); let _ = message .tx .send(Err(iroh_blobs::provider::events::AbortReason::Permission)) .await; - return; } - self.track_request_updates( - transfer_id, - message.inner.connection_id, - message.inner.request_id, - message.rx, - ) - .await; } - let _ = message.tx.send(Ok(())).await; } ProviderMessage::GetRequestReceivedNotify(message) => { - if let Some(transfer_id) = self.transfer_for_hash(message.inner.request.hash).await + if let Ok(transfer_id) = self + .authorize_hash(message.inner.request.hash, message.inner.connection_id) + .await { self.track_request_updates( transfer_id, @@ -117,44 +113,36 @@ impl CoreInner { } } ProviderMessage::GetManyRequestReceived(message) => { - let transfer_id = self - .transfer_for_any_hash(&message.inner.request.hashes) - .await; - if let Some(transfer_id) = transfer_id { - let decision = self - .access_decision(transfer_id, message.inner.connection_id) - .await; - if let AccessDecision::Deny { reason } = decision { - self.emit_transfer( + match self + .authorize_hashes(&message.inner.request.hashes, message.inner.connection_id) + .await + { + Ok(transfer_id) => { + self.track_request_updates( transfer_id, - "send", - "access", - "request-denied", - json!({ - "connection_id": message.inner.connection_id, - "request_id": message.inner.request_id, - "reason": reason, - }), + message.inner.connection_id, + message.inner.request_id, + message.rx, + ) + .await; + let _ = message.tx.send(Ok(())).await; + } + Err(reason) => { + self.emit_denied_request( + message.inner.connection_id, + message.inner.request_id, + reason, ); let _ = message .tx .send(Err(iroh_blobs::provider::events::AbortReason::Permission)) .await; - return; } - self.track_request_updates( - transfer_id, - message.inner.connection_id, - message.inner.request_id, - message.rx, - ) - .await; } - let _ = message.tx.send(Ok(())).await; } ProviderMessage::GetManyRequestReceivedNotify(message) => { - if let Some(transfer_id) = self - .transfer_for_any_hash(&message.inner.request.hashes) + if let Ok(transfer_id) = self + .authorize_hashes(&message.inner.request.hashes, message.inner.connection_id) .await { self.track_request_updates( @@ -167,15 +155,38 @@ impl CoreInner { } } ProviderMessage::ObserveRequestReceived(message) => { - self.emit_endpoint( - "provider", - "observe-request", - json!({ - "connection_id": message.inner.connection_id, - "request_id": message.inner.request_id, - }), - ); - let _ = message.tx.send(Ok(())).await; + // Observe can leak presence of content; use the same ACL as get. + match self + .authorize_hash(message.inner.request.hash, message.inner.connection_id) + .await + { + Ok(_) => { + self.emit_endpoint( + "provider", + "observe-request", + json!({ + "connection_id": message.inner.connection_id, + "request_id": message.inner.request_id, + }), + ); + let _ = message.tx.send(Ok(())).await; + } + Err(reason) => { + self.emit_endpoint( + "provider", + "observe-denied", + json!({ + "connection_id": message.inner.connection_id, + "request_id": message.inner.request_id, + "reason": reason, + }), + ); + let _ = message + .tx + .send(Err(iroh_blobs::provider::events::AbortReason::Permission)) + .await; + } + } } ProviderMessage::ObserveRequestReceivedNotify(message) => { self.emit_endpoint( @@ -209,35 +220,81 @@ impl CoreInner { } } - pub(super) async fn transfer_for_hash(&self, hash: Hash) -> Option { + fn emit_denied_request(&self, connection_id: u64, request_id: u64, reason: &'static str) { + self.emit_endpoint( + "provider", + "request-denied", + json!({ + "connection_id": connection_id, + "request_id": request_id, + "reason": reason, + }), + ); + } + + /// Default-deny: hash must belong to an active share the peer may read. + pub(super) async fn authorize_hash( + &self, + hash: Hash, + connection_id: u64, + ) -> Result { + let transfer_ids = self.transfer_ids_for_hash(hash).await; + if transfer_ids.is_empty() { + return Err("unknown-hash"); + } + self.allow_any_transfer(&transfer_ids, connection_id).await + } + + /// Every hash in a multi-get must be authorized; progress is attributed to + /// the first allowing transfer id. + pub(super) async fn authorize_hashes( + &self, + hashes: &[Hash], + connection_id: u64, + ) -> Result { + if hashes.is_empty() { + return Err("empty-request"); + } + let mut attributed = None; + for hash in hashes { + let transfer_id = self.authorize_hash(*hash, connection_id).await?; + attributed.get_or_insert(transfer_id); + } + attributed.ok_or("empty-request") + } + + pub(super) async fn transfer_ids_for_hash(&self, hash: Hash) -> Vec { self.hash_to_transfer .lock() .await .get(&hash.to_string()) - .copied() + .map(|set| set.iter().copied().collect()) + .unwrap_or_default() } - pub(super) async fn transfer_for_any_hash(&self, hashes: &[Hash]) -> Option { - let map = self.hash_to_transfer.lock().await; - hashes - .iter() - .find_map(|hash| map.get(&hash.to_string()).copied()) - } - - pub(super) async fn access_decision( + async fn allow_any_transfer( &self, - transfer_id: u64, + transfer_ids: &[u64], connection_id: u64, - ) -> AccessDecision { + ) -> Result { let endpoint_id = self .connection_endpoints .lock() .await .get(&connection_id) .cloned(); - self.access_policy - .decide(transfer_id, endpoint_id.as_deref()) - .await + let mut last_reason = "approval-required"; + for transfer_id in transfer_ids { + match self + .access_policy + .decide(*transfer_id, endpoint_id.as_deref()) + .await + { + AccessDecision::Allow => return Ok(*transfer_id), + AccessDecision::Deny { reason } => last_reason = reason, + } + } + Err(last_reason) } pub(super) async fn track_request_updates( diff --git a/crates/vnidrop/src/runtime/receive.rs b/crates/vnidrop/src/runtime/receive.rs index bbbdd76..6ee8878 100644 --- a/crates/vnidrop/src/runtime/receive.rs +++ b/crates/vnidrop/src/runtime/receive.rs @@ -26,7 +26,6 @@ use crate::{ repository::TransferUpsert, ticket::{parse_transfer_ticket_with_limits, ParsedTransferTicket}, transfer_state::{TransferDirection, TransferStatus}, - util::unique_transfer_id, }; pub(super) enum ReceiveTarget { @@ -135,11 +134,7 @@ impl CoreInner { return Err(error); } }; - let transfer_id = parsed - .metadata - .as_ref() - .map(|metadata| metadata.transfer_id) - .unwrap_or_else(unique_transfer_id); + let transfer_id = parsed.metadata.transfer_id; self.persist_receive_start(transfer_id, &parsed, receiver_name.as_deref()) .await?; // Cancellation is cooperative: it stops our receive future and marks @@ -202,19 +197,15 @@ impl CoreInner { let sender_addr = parsed.blob_ticket.addr().clone(); self.emit_transfer(transfer_id, "receive", "network", "connecting", json!({})); - let delivery_receipt = if let Some(metadata) = &parsed.metadata { - Some( - self.request_transfer_approval( - transfer_id, - sender_addr.clone(), - metadata, - receiver_name.as_deref(), - ) - .await?, + // Every VniDrop ticket carries metadata and must complete the handshake. + let delivery_receipt = self + .request_transfer_approval( + transfer_id, + sender_addr.clone(), + &parsed.metadata, + receiver_name.as_deref(), ) - } else { - None - }; + .await?; let connection = self .endpoint .connect(sender_addr.clone(), iroh_blobs::ALPN) @@ -280,32 +271,30 @@ impl CoreInner { ) .await?; self.emit_transfer(transfer_id, "receive", "lifecycle", "done", json!({})); - if let Some(receipt) = delivery_receipt { - let sender_transfer_id = receipt.transfer_id; - let client = HandshakeService::client(self.endpoint.clone(), sender_addr); - match client.report_delivery(receipt).await { - Ok(DeliveryReceiptResponse::Recorded) => self.emit_transfer( - transfer_id, - "receive", - "delivery", - "receipt-recorded", - json!({ "sender_transfer_id": sender_transfer_id }), - ), - Ok(DeliveryReceiptResponse::Rejected { reason }) => self.emit_transfer( - transfer_id, - "receive", - "delivery", - "receipt-rejected", - json!({ "reason": reason }), - ), - Err(error) => self.emit_transfer( - transfer_id, - "receive", - "delivery", - "receipt-failed", - json!({ "reason": error.to_string() }), - ), - } + let sender_transfer_id = delivery_receipt.transfer_id; + let client = HandshakeService::client(self.endpoint.clone(), sender_addr); + match client.report_delivery(delivery_receipt).await { + Ok(DeliveryReceiptResponse::Recorded) => self.emit_transfer( + transfer_id, + "receive", + "delivery", + "receipt-recorded", + json!({ "sender_transfer_id": sender_transfer_id }), + ), + Ok(DeliveryReceiptResponse::Rejected { reason }) => self.emit_transfer( + transfer_id, + "receive", + "delivery", + "receipt-rejected", + json!({ "reason": reason }), + ), + Err(error) => self.emit_transfer( + transfer_id, + "receive", + "delivery", + "receipt-failed", + json!({ "reason": error.to_string() }), + ), } Ok(()) } @@ -323,25 +312,11 @@ impl CoreInner { peer_id: Some(&peer_id), direction: TransferDirection::Receive, status: TransferStatus::Receiving, - transfer_name: parsed - .metadata - .as_ref() - .map(|metadata| metadata.transfer_name.as_str()), - content_hash: parsed - .metadata - .as_ref() - .map(|metadata| metadata.content_hash.as_str()), + transfer_name: Some(parsed.metadata.transfer_name.as_str()), + content_hash: Some(parsed.metadata.content_hash.as_str()), ticket: None, - file_count: parsed - .metadata - .as_ref() - .map(|metadata| metadata.file_count) - .unwrap_or_default(), - total_size: parsed - .metadata - .as_ref() - .map(|metadata| metadata.total_size) - .unwrap_or_default(), + file_count: parsed.metadata.file_count, + total_size: parsed.metadata.total_size, access_mode: mode_to_storage(&TransferAccessMode::ApprovalRequired), }) .await?; diff --git a/crates/vnidrop/src/runtime/share.rs b/crates/vnidrop/src/runtime/share.rs index a54b886..51e8bc7 100644 --- a/crates/vnidrop/src/runtime/share.rs +++ b/crates/vnidrop/src/runtime/share.rs @@ -138,7 +138,7 @@ impl CoreInner { import.file_count, import.total_size, ); - let ticket = VnidropTicket::new(blob_ticket.clone(), ticket_metadata) + let ticket = VnidropTicket::new(blob_ticket, ticket_metadata) .encode() .context("failed to encode VniDrop transfer ticket")?; let content_hash = import.root_hash.to_string(); @@ -160,10 +160,13 @@ impl CoreInner { access_mode: mode_to_storage(&access_mode), }) .await?; - self.hash_to_transfer - .lock() - .await - .insert(content_hash, metadata.transfer_id); + // Map root + every collection member so provider ACL cannot fail-open + // on child blob hashes that are not the collection root. + self.register_share_hashes( + metadata.transfer_id, + std::iter::once(import.root_hash).chain(import.member_hashes.iter().copied()), + ) + .await; self.access_policy .set_mode(metadata.transfer_id, access_mode) .await; @@ -172,13 +175,13 @@ impl CoreInner { .await .insert(metadata.transfer_id, Some(import.tag)); + // Tickets are capabilities: never persist the full string in events. self.emit_transfer( metadata.transfer_id, "send", "ticket", "created", json!({ - "ticket": ticket, "hash": import.root_hash.to_string(), "total_size": import.total_size, "file_count": import.file_count, @@ -188,7 +191,6 @@ impl CoreInner { Ok(ShareResult { transfer_id: metadata.transfer_id, ticket, - blob_ticket: blob_ticket.to_string(), hash: import.root_hash.to_string(), transfer_name, file_count: import.file_count, @@ -242,6 +244,7 @@ impl CoreInner { .into_iter() .map(|(name, tag, _)| ((name, tag.hash()), tag)) .unzip::<_, _, Collection, Vec<_>>(); + let member_hashes = collection.iter().map(|(_, hash)| *hash).collect::>(); let collection_tag = collection.clone().store(&self.store).await?; let root_hash = collection_tag.hash(); let file_count = tags.len() as u64; @@ -258,6 +261,7 @@ impl CoreInner { Ok(TransferImport { tag: collection_tag, root_hash, + member_hashes, total_size, file_count, default_name, diff --git a/crates/vnidrop/src/secret.rs b/crates/vnidrop/src/secret.rs index 3839c60..305f461 100644 --- a/crates/vnidrop/src/secret.rs +++ b/crates/vnidrop/src/secret.rs @@ -1,7 +1,12 @@ -use std::{io, path::Path, str::FromStr}; +use std::{ + fs::OpenOptions, + io::{self, Write}, + path::Path, + str::FromStr, +}; #[cfg(unix)] -use std::os::unix::fs::PermissionsExt; +use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; use anyhow::{Context, Result}; use data_encoding::HEXLOWER; @@ -26,7 +31,9 @@ pub(crate) async fn load_or_create_secret(app_data_dir: &Path) -> Result { let secret = SecretKey::generate(); - tokio::fs::write(&path, HEXLOWER.encode(&secret.to_bytes())).await?; + let encoded = HEXLOWER.encode(&secret.to_bytes()); + // Create with owner-only mode on Unix so the key is never briefly 0644. + write_secret_file(&path, encoded.as_bytes()).await?; restrict_permissions(&path).await?; Ok(secret) } @@ -34,8 +41,31 @@ pub(crate) async fn load_or_create_secret(app_data_dir: &Path) -> Result Result<()> { + let path = path.to_path_buf(); + let bytes = bytes.to_vec(); + tokio::task::spawn_blocking(move || { + let mut options = OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + options.mode(0o600); + let mut file = options + .open(&path) + .with_context(|| format!("failed to create {}", path.display()))?; + file.write_all(&bytes) + .with_context(|| format!("failed to write {}", path.display()))?; + file.sync_all() + .with_context(|| format!("failed to sync {}", path.display()))?; + Ok::<(), anyhow::Error>(()) + }) + .await? +} + async fn restrict_permissions(path: &Path) -> Result<()> { #[cfg(unix)] tokio::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)).await?; + // Windows: file lives under the user profile app-data dir with default ACLs + // limited to the current user. No portable owner-only API in std. + let _ = path; Ok(()) } diff --git a/crates/vnidrop/src/tests/access_policy.rs b/crates/vnidrop/src/tests/access_policy.rs index 4aa6fcc..4302088 100644 --- a/crates/vnidrop/src/tests/access_policy.rs +++ b/crates/vnidrop/src/tests/access_policy.rs @@ -4,6 +4,23 @@ use crate::{ TransferAccessMode, }; +#[tokio::test] +async fn unknown_transfer_fails_closed() { + let policy = AccessPolicy::new(); + assert_eq!( + policy.decide(1, Some("node-a")).await, + AccessDecision::Deny { + reason: "unknown-transfer" + } + ); + assert_eq!( + policy.decide(1, None).await, + AccessDecision::Deny { + reason: "unknown-transfer" + } + ); +} + #[tokio::test] async fn requires_approved_endpoint_when_locked() { let policy = AccessPolicy::new(); @@ -54,3 +71,14 @@ async fn rejects_expired_approval_sessions() { } ); } + +#[tokio::test] +async fn public_mode_allows_without_session() { + let policy = AccessPolicy::new(); + policy.set_mode(7, TransferAccessMode::Public).await; + assert_eq!( + policy.decide(7, Some("node-a")).await, + AccessDecision::Allow + ); + assert_eq!(policy.decide(7, None).await, AccessDecision::Allow); +} diff --git a/crates/vnidrop/src/tests/ticket.rs b/crates/vnidrop/src/tests/ticket.rs index 62ace59..47fec19 100644 --- a/crates/vnidrop/src/tests/ticket.rs +++ b/crates/vnidrop/src/tests/ticket.rs @@ -31,10 +31,7 @@ fn metadata_ticket_round_trips() { let parsed = parse_transfer_ticket(&encoded).unwrap(); assert_eq!(parsed.blob_ticket.hash(), blob_ticket.hash()); - assert_eq!( - parsed.metadata.unwrap().transfer_name, - metadata.transfer_name - ); + assert_eq!(parsed.metadata.transfer_name, metadata.transfer_name); } #[test] @@ -60,6 +57,16 @@ fn invalid_ticket_is_rejected() { assert!(parse_transfer_ticket("not-a-ticket").is_err()); } +#[test] +fn rejects_raw_blob_tickets() { + let raw = blob_ticket(3).to_string(); + let error = parse_transfer_ticket(&raw).unwrap_err().to_string(); + assert!( + error.contains("not a VniDrop ticket"), + "raw BlobTicket must not be accepted as a transfer invitation: {error}" + ); +} + #[test] fn rejects_unsupported_versions_and_mismatched_hashes() { let blob_ticket = blob_ticket(5); diff --git a/crates/vnidrop/src/ticket.rs b/crates/vnidrop/src/ticket.rs index 76f9198..87f4b63 100644 --- a/crates/vnidrop/src/ticket.rs +++ b/crates/vnidrop/src/ticket.rs @@ -49,7 +49,7 @@ impl VnidropTicket { #[derive(Debug, Clone)] pub(crate) struct ParsedTransferTicket { pub(crate) blob_ticket: BlobTicket, - pub(crate) metadata: Option, + pub(crate) metadata: TransferMetadata, } #[cfg(test)] @@ -69,49 +69,44 @@ pub(crate) fn parse_transfer_ticket_with_limits( ); } let normalized = normalize_ticket_input(value); - if normalized.starts_with(VNIDROP_TICKET_PREFIX) { - let ticket = VnidropTicket::decode(&normalized)?; - if ticket.version != VNIDROP_TICKET_VERSION { - anyhow::bail!("unsupported VniDrop ticket version {}", ticket.version); - } - if ticket.metadata.version != VNIDROP_TICKET_VERSION { - anyhow::bail!( - "unsupported VniDrop metadata version {}", - ticket.metadata.version - ); - } - if ticket.metadata.transfer_id == 0 { - anyhow::bail!("VniDrop ticket metadata is missing a valid transfer id"); - } - if ticket.metadata.transfer_name.trim().is_empty() { - anyhow::bail!("VniDrop ticket metadata is missing a transfer name"); - } - limits.validate_metadata_text( - "transfer name", - Some(ticket.metadata.transfer_name.as_str()), - )?; - limits.validate_metadata_text("sender name", ticket.metadata.sender_name.as_deref())?; - let blob_ticket = BlobTicket::from_str(&ticket.blob_ticket) - .context("invalid BlobTicket inside VniDrop ticket")?; - if ticket.metadata.content_hash != blob_ticket.hash().to_string() { - anyhow::bail!("VniDrop ticket metadata hash does not match BlobTicket hash"); - } - return Ok(ParsedTransferTicket { - blob_ticket, - metadata: Some(ticket.metadata), - }); + if !normalized.starts_with(VNIDROP_TICKET_PREFIX) { + anyhow::bail!("not a VniDrop ticket; expected a vnd1: invitation"); + } + let ticket = VnidropTicket::decode(&normalized)?; + if ticket.version != VNIDROP_TICKET_VERSION { + anyhow::bail!("unsupported VniDrop ticket version {}", ticket.version); + } + if ticket.metadata.version != VNIDROP_TICKET_VERSION { + anyhow::bail!( + "unsupported VniDrop metadata version {}", + ticket.metadata.version + ); + } + if ticket.metadata.transfer_id == 0 { + anyhow::bail!("VniDrop ticket metadata is missing a valid transfer id"); + } + if ticket.metadata.transfer_name.trim().is_empty() { + anyhow::bail!("VniDrop ticket metadata is missing a transfer name"); + } + limits.validate_metadata_text( + "transfer name", + Some(ticket.metadata.transfer_name.as_str()), + )?; + limits.validate_metadata_text("sender name", ticket.metadata.sender_name.as_deref())?; + let blob_ticket = BlobTicket::from_str(&ticket.blob_ticket) + .context("invalid BlobTicket inside VniDrop ticket")?; + if ticket.metadata.content_hash != blob_ticket.hash().to_string() { + anyhow::bail!("VniDrop ticket metadata hash does not match BlobTicket hash"); } - - let blob_ticket = BlobTicket::from_str(&normalized).context("invalid BlobTicket")?; Ok(ParsedTransferTicket { blob_ticket, - metadata: None, + metadata: ticket.metadata, }) } fn normalize_ticket_input(value: &str) -> String { // Tickets are commonly copied from text views or chat apps that insert line // breaks. Strip whitespace only; other corrupt characters should still be - // rejected by the base64 or BlobTicket decoders. + // rejected by the base64 decoder. value.chars().filter(|char| !char.is_whitespace()).collect() } diff --git a/crates/vnidrop/src/util.rs b/crates/vnidrop/src/util.rs index cb62494..f599f15 100644 --- a/crates/vnidrop/src/util.rs +++ b/crates/vnidrop/src/util.rs @@ -11,7 +11,3 @@ pub(crate) fn now_ms() -> i64 { .map(|duration| duration.as_millis() as i64) .unwrap_or_default() } - -pub(crate) fn unique_transfer_id() -> u64 { - now_ms() as u64 -} diff --git a/crates/vnidrop/tests/lifecycle.rs b/crates/vnidrop/tests/lifecycle.rs index da4b086..7a96b30 100644 --- a/crates/vnidrop/tests/lifecycle.rs +++ b/crates/vnidrop/tests/lifecycle.rs @@ -100,7 +100,7 @@ fn persisted_share_is_recovered_and_can_be_stopped_after_restart() { } #[test] -fn stopped_share_rejects_direct_legacy_blob_ticket() { +fn stopped_share_rejects_receive() { let source_dir = tempfile::tempdir().unwrap(); let output_dir = tempfile::tempdir().unwrap(); let source_path = source_dir.path().join("revoked.txt"); @@ -111,7 +111,7 @@ fn stopped_share_rejects_direct_legacy_blob_ticket() { sender.core.cancel_transfer(share.transfer_id).unwrap(); let result = receiver.core.receive( - share.blob_ticket, + share.ticket, output_dir.path().to_string_lossy().to_string(), Some("receiver".to_string()), ); @@ -120,6 +120,52 @@ fn stopped_share_rejects_direct_legacy_blob_ticket() { assert!(!output_dir.path().join("revoked.txt").exists()); } +#[test] +fn ticket_created_event_does_not_include_full_ticket() { + let source_dir = tempfile::tempdir().unwrap(); + let source_path = source_dir.path().join("secret.txt"); + std::fs::write(&source_path, b"capability material").unwrap(); + let sender = TestNode::new(); + let share = share_path(&sender.core, &source_path, 40, "secret.txt", false); + + let ticket_events: Vec<_> = sender + .sink + .events() + .into_iter() + .filter(|event| event.phase == "ticket" && event.kind == "created") + .collect(); + assert_eq!(ticket_events.len(), 1); + let data = &ticket_events[0].data_json; + assert!( + !data.contains(&share.ticket), + "events must not retain the full vnd1 ticket capability" + ); + assert!( + !data.contains("vnd1:"), + "events must not embed ticket prefixes" + ); + assert!( + data.contains(&share.hash), + "events should still record the content hash for diagnostics" + ); +} + +#[test] +fn receive_rejects_non_vnidrop_ticket_input() { + let output_dir = tempfile::tempdir().unwrap(); + let receiver = TestNode::new(); + + let result = receiver.core.receive( + "blobaaabcdefghijklmnopqrstuvwxyz0123456789".to_string(), + output_dir.path().to_string_lossy().to_string(), + Some("receiver".to_string()), + ); + assert!( + result.is_err(), + "non-vnd1 tickets must be rejected before network work" + ); +} + #[test] fn failed_import_leaves_durable_failed_transfer() { let source_dir = tempfile::tempdir().unwrap(); diff --git a/shared/src/commonMain/composeResources/values/strings.xml b/shared/src/commonMain/composeResources/values/strings.xml index 28749d3..2bc62c7 100644 --- a/shared/src/commonMain/composeResources/values/strings.xml +++ b/shared/src/commonMain/composeResources/values/strings.xml @@ -128,7 +128,7 @@ Saving files Connecting to sender Ticket details - This ticket does not include VniDrop metadata. + Settings Configure the local node and app appearance. Node diff --git a/shared/src/commonMain/kotlin/com/vnidrop/app/core/CoreModels.kt b/shared/src/commonMain/kotlin/com/vnidrop/app/core/CoreModels.kt index 6ccc20c..bbff3d3 100644 --- a/shared/src/commonMain/kotlin/com/vnidrop/app/core/CoreModels.kt +++ b/shared/src/commonMain/kotlin/com/vnidrop/app/core/CoreModels.kt @@ -77,8 +77,7 @@ data class TransferMetadataModel( data class TicketInspectionModel( val kind: String, - val blobTicket: String, - val metadata: TransferMetadataModel?, + val metadata: TransferMetadataModel, ) data class ReceiverRequestModel( diff --git a/shared/src/commonMain/kotlin/com/vnidrop/app/core/CoreRepository.kt b/shared/src/commonMain/kotlin/com/vnidrop/app/core/CoreRepository.kt index 8eab4f1..41a4e6e 100644 --- a/shared/src/commonMain/kotlin/com/vnidrop/app/core/CoreRepository.kt +++ b/shared/src/commonMain/kotlin/com/vnidrop/app/core/CoreRepository.kt @@ -335,8 +335,7 @@ private fun ShareResult.toModel(): Share = Share( private fun TicketInspection.toModel(): TicketInspectionModel = TicketInspectionModel( kind = kind, - blobTicket = blobTicket, - metadata = metadata?.toModel(), + metadata = metadata.toModel(), ) private fun TransferMetadata.toModel(): TransferMetadataModel = TransferMetadataModel( diff --git a/shared/src/commonMain/kotlin/com/vnidrop/app/feature/receive/ReceiveScreen.kt b/shared/src/commonMain/kotlin/com/vnidrop/app/feature/receive/ReceiveScreen.kt index 6894a84..e3d772d 100644 --- a/shared/src/commonMain/kotlin/com/vnidrop/app/feature/receive/ReceiveScreen.kt +++ b/shared/src/commonMain/kotlin/com/vnidrop/app/feature/receive/ReceiveScreen.kt @@ -255,8 +255,8 @@ private fun InvitationReviewPanel( val metadata = inspection.metadata Surface(shape = RoundedCornerShape(14.dp), color = LocalVniDropColors.current.backgroundSurface200) { Column(Modifier.fillMaxWidth().padding(16.dp), verticalArrangement = Arrangement.spacedBy(8.dp)) { - Text(metadata?.transferName ?: stringResource(Res.string.receive_unknown_transfer), fontWeight = FontWeight.Bold, maxLines = 2, overflow = TextOverflow.Ellipsis) - if (metadata != null) Text("${metadata.fileCount} ${stringResource(Res.string.metadata_files).lowercase()} · ${formatBytes(metadata.totalSize)}", color = LocalVniDropColors.current.foregroundLighter) + Text(metadata.transferName, fontWeight = FontWeight.Bold, maxLines = 2, overflow = TextOverflow.Ellipsis) + Text("${metadata.fileCount} ${stringResource(Res.string.metadata_files).lowercase()} · ${formatBytes(metadata.totalSize)}", color = LocalVniDropColors.current.foregroundLighter) } } Field(state.receiverName, onReceiverNameChanged, stringResource(Res.string.field_receiver_name)) diff --git a/shared/src/commonMain/kotlin/com/vnidrop/app/ui/screens/ScreenSections.kt b/shared/src/commonMain/kotlin/com/vnidrop/app/ui/screens/ScreenSections.kt index 5749781..92db048 100644 --- a/shared/src/commonMain/kotlin/com/vnidrop/app/ui/screens/ScreenSections.kt +++ b/shared/src/commonMain/kotlin/com/vnidrop/app/ui/screens/ScreenSections.kt @@ -32,7 +32,6 @@ import vnidrop.shared.generated.resources.metadata_size import vnidrop.shared.generated.resources.metadata_transfer import vnidrop.shared.generated.resources.progress_title import vnidrop.shared.generated.resources.ticket_details_title -import vnidrop.shared.generated.resources.ticket_no_metadata import vnidrop.shared.generated.resources.unknown_sender @Composable @@ -60,15 +59,14 @@ fun ProgressSection(coreState: CoreState) { @Composable fun TicketInspectionCard(inspection: TicketInspectionModel) { + val metadata = inspection.metadata AppCard(title = stringResource(Res.string.ticket_details_title)) { MetadataRow(stringResource(Res.string.metadata_kind), inspection.kind) - inspection.metadata?.let { metadata -> - MetadataRow(stringResource(Res.string.metadata_transfer), metadata.transferName) - MetadataRow(stringResource(Res.string.metadata_sender), metadata.senderName ?: stringResource(Res.string.unknown_sender)) - MetadataRow(stringResource(Res.string.metadata_files), metadata.fileCount.toString()) - MetadataRow(stringResource(Res.string.metadata_size), formatBytes(metadata.totalSize)) - MetadataRow(stringResource(Res.string.metadata_hash), metadata.contentHash) - } ?: EmptyText(stringResource(Res.string.ticket_no_metadata)) + MetadataRow(stringResource(Res.string.metadata_transfer), metadata.transferName) + MetadataRow(stringResource(Res.string.metadata_sender), metadata.senderName ?: stringResource(Res.string.unknown_sender)) + MetadataRow(stringResource(Res.string.metadata_files), metadata.fileCount.toString()) + MetadataRow(stringResource(Res.string.metadata_size), formatBytes(metadata.totalSize)) + MetadataRow(stringResource(Res.string.metadata_hash), metadata.contentHash) } } diff --git a/shared/src/commonTest/kotlin/com/vnidrop/app/feature/ViewModelsTest.kt b/shared/src/commonTest/kotlin/com/vnidrop/app/feature/ViewModelsTest.kt index ea6d87e..2953f55 100644 --- a/shared/src/commonTest/kotlin/com/vnidrop/app/feature/ViewModelsTest.kt +++ b/shared/src/commonTest/kotlin/com/vnidrop/app/feature/ViewModelsTest.kt @@ -299,11 +299,7 @@ class ViewModelsTest { Dispatchers.setMain(StandardTestDispatcher(testScheduler)) val core = FakeCoreGateway().apply { mutableState.value = mutableState.value.copy(isInitialized = true) - inspectionResult = Result.success(com.vnidrop.app.core.TicketInspectionModel( - kind = "vnidrop", - blobTicket = "blob", - metadata = com.vnidrop.app.core.TransferMetadataModel(1UL, "Photo", null, "hash", 1UL, 42UL), - )) + inspectionResult = Result.success(sampleTicketInspection()) } val viewModel = ReceiveViewModel(core, FakeFileSystemService(folder), preferences(), UiMessageController()) advanceUntilIdle() @@ -379,13 +375,7 @@ class ViewModelsTest { Dispatchers.setMain(StandardTestDispatcher(testScheduler)) val core = FakeCoreGateway().apply { mutableState.value = mutableState.value.copy(isInitialized = true) - inspectionResult = Result.success( - com.vnidrop.app.core.TicketInspectionModel( - kind = "vnidrop", - blobTicket = "blob", - metadata = com.vnidrop.app.core.TransferMetadataModel(1UL, "Photo", null, "hash", 1UL, 42UL), - ), - ) + inspectionResult = Result.success(sampleTicketInspection()) } val viewModel = ReceiveViewModel(core, FakeFileSystemService(folder), preferences(), UiMessageController()) advanceUntilIdle() @@ -408,13 +398,7 @@ class ViewModelsTest { Dispatchers.setMain(StandardTestDispatcher(testScheduler)) val core = FakeCoreGateway().apply { mutableState.value = mutableState.value.copy(isInitialized = true) - inspectionResult = Result.success( - com.vnidrop.app.core.TicketInspectionModel( - kind = "vnidrop", - blobTicket = "blob", - metadata = com.vnidrop.app.core.TransferMetadataModel(1UL, "Photo", null, "hash", 1UL, 42UL), - ), - ) + inspectionResult = Result.success(sampleTicketInspection()) receiveResult = Result.failure(IllegalStateException("sender refused")) } val viewModel = ReceiveViewModel(core, FakeFileSystemService(folder), preferences(), UiMessageController()) @@ -489,13 +473,7 @@ class ViewModelsTest { Dispatchers.setMain(StandardTestDispatcher(testScheduler)) val core = FakeCoreGateway().apply { mutableState.value = mutableState.value.copy(isInitialized = true) - inspectionResult = Result.success( - com.vnidrop.app.core.TicketInspectionModel( - kind = "vnidrop", - blobTicket = "blob", - metadata = com.vnidrop.app.core.TransferMetadataModel(1UL, "Photo", null, "hash", 1UL, 42UL), - ), - ) + inspectionResult = Result.success(sampleTicketInspection()) // Keep receive suspended so dismiss can be asserted mid-transfer. receiveResult = Result.success(Unit) receiveSuspend = true @@ -544,6 +522,11 @@ class ViewModelsTest { updatedAt = 1L, ) + private fun sampleTicketInspection() = com.vnidrop.app.core.TicketInspectionModel( + kind = "vnidrop", + metadata = com.vnidrop.app.core.TransferMetadataModel(1UL, "Photo", null, "hash", 1UL, 42UL), + ) + private companion object { val folder = ReceiveFolder(ReceiveFolderKind.FileSystemPath, "/tmp", "Downloads") } diff --git a/shared/src/commonTest/kotlin/com/vnidrop/app/ui/state/AppUiModelsTest.kt b/shared/src/commonTest/kotlin/com/vnidrop/app/ui/state/AppUiModelsTest.kt index c783ce7..705056a 100644 --- a/shared/src/commonTest/kotlin/com/vnidrop/app/ui/state/AppUiModelsTest.kt +++ b/shared/src/commonTest/kotlin/com/vnidrop/app/ui/state/AppUiModelsTest.kt @@ -61,7 +61,10 @@ class AppUiModelsTest { fun receiveStateExposesInspectAndReceiveEligibility() { val ready = ReceiveState( ticket = "ticket", - inspection = com.vnidrop.app.core.TicketInspectionModel("vnidrop", "blob", null), + inspection = com.vnidrop.app.core.TicketInspectionModel( + kind = "vnidrop", + metadata = com.vnidrop.app.core.TransferMetadataModel(1UL, "Photo", null, "hash", 1UL, 42UL), + ), folderAccessStatus = com.vnidrop.app.core.FolderAccessStatus.Writable, ) From d0c8d8774a4d9a3cf7c94413d11258f924f72748 Mon Sep 17 00:00:00 2001 From: Hammed Abass Date: Mon, 13 Jul 2026 18:54:28 +0200 Subject: [PATCH 2/2] fix(security): address medium findings for ACL, limits, and UX Tighten approve-endpoint to active shares with TTL sessions, reject non-file FDs, lower default ticket/approval/size caps, show endpoint IDs and Public-mode warnings, harden Android receive path checks, and run cargo-audit in CI. --- .cargo/audit.toml | 11 ++++++++ .github/workflows/rust-core.yml | 5 ++++ crates/vnidrop/CORE_FLOW.md | 7 ++++- crates/vnidrop/src/access_policy.rs | 12 ++++++-- crates/vnidrop/src/api.rs | 9 ++++-- crates/vnidrop/src/approval.rs | 4 +-- crates/vnidrop/src/filesystem.rs | 22 +++++++++++++-- crates/vnidrop/src/runtime/lifecycle.rs | 15 ++++++++++ crates/vnidrop/src/tests/access_policy.rs | 13 +++++++++ crates/vnidrop/src/tests/filesystem.rs | 20 +++++++++++++ crates/vnidrop/src/tests/limits.rs | 8 ++++++ crates/vnidrop/tests/lifecycle.rs | 28 +++++++++++++++++++ .../app/core/FileSystemService.android.kt | 23 ++++++++++++--- .../composeResources/values/strings.xml | 2 ++ .../feature/approvals/ApprovalCoordinator.kt | 3 ++ .../feature/approvals/ApprovalModalHost.kt | 7 +++++ .../app/feature/send/TransferComposer.kt | 8 ++++++ .../vnidrop/app/ui/FoundationComposeTest.kt | 1 + 18 files changed, 184 insertions(+), 14 deletions(-) create mode 100644 .cargo/audit.toml diff --git a/.cargo/audit.toml b/.cargo/audit.toml new file mode 100644 index 0000000..51643d7 --- /dev/null +++ b/.cargo/audit.toml @@ -0,0 +1,11 @@ +# Known transitive advisories we cannot fully clear without upstream iroh bumps. +# cargo audit in CI fails on new unlisted vulnerabilities. +[advisories] +ignore = [ + # rsa: Marvin timing side-channel; no fixed release; pulled by iroh stack. + "RUSTSEC-2023-0071", + # quick-xml / crossbeam-epoch: transitive; track via iroh/dependency updates. + "RUSTSEC-2026-0194", + "RUSTSEC-2026-0195", + "RUSTSEC-2026-0204", +] diff --git a/.github/workflows/rust-core.yml b/.github/workflows/rust-core.yml index 45336c7..759ec1f 100644 --- a/.github/workflows/rust-core.yml +++ b/.github/workflows/rust-core.yml @@ -43,3 +43,8 @@ jobs: env: RUSTDOCFLAGS: -D warnings run: cargo doc --workspace --no-deps + - name: Install cargo-audit + run: cargo install cargo-audit --locked + - name: Audit Rust dependencies + # Ignores are listed in .cargo/audit.toml for known transitive issues. + run: cargo audit diff --git a/crates/vnidrop/CORE_FLOW.md b/crates/vnidrop/CORE_FLOW.md index a45a9c5..84e31ad 100644 --- a/crates/vnidrop/CORE_FLOW.md +++ b/crates/vnidrop/CORE_FLOW.md @@ -91,7 +91,12 @@ collection. Restart reconciliation never restores a stopped share. `CoreLimits` controls source count, collection files and bytes, path and ticket sizes, metadata, retained events, pending approvals, concurrent transfers, and -the event persistence queue. `initialize` uses conservative defaults; +the event persistence queue. `initialize` uses conservative defaults (including +bounded ticket size, pending approvals, and total collection bytes); `initialize_with_limits` supports stricter deployments and tests. Cheap limits are checked before durable or network work, while remote collection limits are checked before downloading file content. + +Manual `approve_endpoint_for_transfer` only applies to active shares, requires a +non-empty endpoint id, and creates a time-limited session (same TTL as handshake +approval), never a permanent grant. diff --git a/crates/vnidrop/src/access_policy.rs b/crates/vnidrop/src/access_policy.rs index 1584dfe..eb9d7fb 100644 --- a/crates/vnidrop/src/access_policy.rs +++ b/crates/vnidrop/src/access_policy.rs @@ -5,6 +5,9 @@ use tokio::sync::RwLock; use crate::api::TransferAccessMode; use crate::util::now_ms; +/// Default lifetime for endpoint approval sessions (matches handshake approval TTL). +pub(crate) const APPROVAL_SESSION_TTL_MS: i64 = 10 * 60 * 1000; + #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum AccessDecision { Allow, @@ -42,8 +45,13 @@ impl AccessPolicy { } pub(crate) async fn approve_endpoint(&self, transfer_id: u64, endpoint_id: String) { - self.approve_endpoint_until(transfer_id, endpoint_id, None) - .await; + // Never grant permanent sessions from the public API: always expire. + self.approve_endpoint_until( + transfer_id, + endpoint_id, + Some(now_ms() + APPROVAL_SESSION_TTL_MS), + ) + .await; } pub(crate) async fn approve_endpoint_until( diff --git a/crates/vnidrop/src/api.rs b/crates/vnidrop/src/api.rs index aa24a07..6ce048e 100644 --- a/crates/vnidrop/src/api.rs +++ b/crates/vnidrop/src/api.rs @@ -23,12 +23,15 @@ impl Default for CoreLimits { Self { max_sources: 128, max_collection_files: 10_000, - max_total_bytes: 1024 * 1024 * 1024 * 1024, + // Cap extreme disk fill while still allowing multi-GB folders. + max_total_bytes: 256 * 1024 * 1024 * 1024, max_path_bytes: 4_096, - max_ticket_bytes: 1024 * 1024, + // vnd1 tickets are small JSON+base64; 256 KiB is a generous ceiling. + max_ticket_bytes: 256 * 1024, max_metadata_bytes: 16 * 1024, max_events: 500, - max_pending_approvals: 1_024, + // Bound handshake spam / notification pressure on the sender. + max_pending_approvals: 64, max_concurrent_transfers: 8, event_queue_capacity: 1_024, } diff --git a/crates/vnidrop/src/approval.rs b/crates/vnidrop/src/approval.rs index b8ab720..23075f9 100644 --- a/crates/vnidrop/src/approval.rs +++ b/crates/vnidrop/src/approval.rs @@ -6,7 +6,7 @@ use tokio::sync::{oneshot, Mutex}; use uuid::Uuid; use crate::{ - access_policy::AccessPolicy, + access_policy::{AccessPolicy, APPROVAL_SESSION_TTL_MS}, event_hub::EventHub, handshake::{DeliveryReceipt, DeliveryReceiptResponse, HandshakeResponse, RequestTransfer}, repository::{ReceiverRequestInsert, Repository}, @@ -14,7 +14,7 @@ use crate::{ util::now_ms, }; -const APPROVAL_TTL_MS: i64 = 10 * 60 * 1000; +const APPROVAL_TTL_MS: i64 = APPROVAL_SESSION_TTL_MS; const APPROVAL_WAIT_TIMEOUT: Duration = Duration::from_secs(120); #[derive(Debug, Clone, Serialize, Deserialize)] diff --git a/crates/vnidrop/src/filesystem.rs b/crates/vnidrop/src/filesystem.rs index 88088db..c23b3a9 100644 --- a/crates/vnidrop/src/filesystem.rs +++ b/crates/vnidrop/src/filesystem.rs @@ -582,6 +582,24 @@ fn duplicate_file_descriptor(value: &str) -> Result { if duplicated < 0 { return Err(io::Error::last_os_error()).context("failed to duplicate file descriptor"); } - let owned = unsafe { OwnedFd::from_raw_fd(duplicated) }; - Ok(owned) + + // Only regular files are valid share sources. Reject directories, sockets, + // and other fd types that a compromised UI could pass by integer. + let mut stat = std::mem::MaybeUninit::::uninit(); + let rc = unsafe { libc::fstat(duplicated, stat.as_mut_ptr()) }; + if rc != 0 { + let error = io::Error::last_os_error(); + unsafe { + libc::close(duplicated); + } + return Err(error).context("failed to fstat file descriptor"); + } + let mode = unsafe { stat.assume_init() }.st_mode; + if mode & libc::S_IFMT != libc::S_IFREG { + unsafe { + libc::close(duplicated); + } + anyhow::bail!("file descriptor must refer to a regular file"); + } + Ok(unsafe { OwnedFd::from_raw_fd(duplicated) }) } diff --git a/crates/vnidrop/src/runtime/lifecycle.rs b/crates/vnidrop/src/runtime/lifecycle.rs index 0e2e246..14ae84e 100644 --- a/crates/vnidrop/src/runtime/lifecycle.rs +++ b/crates/vnidrop/src/runtime/lifecycle.rs @@ -143,6 +143,21 @@ impl CoreInner { transfer_id: u64, endpoint_id: String, ) -> Result<()> { + let endpoint_id = endpoint_id.trim().to_string(); + if endpoint_id.is_empty() { + anyhow::bail!("endpoint id must not be empty"); + } + if endpoint_id.len() as u64 > self.limits.max_metadata_bytes { + anyhow::bail!( + "endpoint id is {} bytes, limit is {}", + endpoint_id.len(), + self.limits.max_metadata_bytes + ); + } + // Only live shares can gain receiver sessions. + if !self.active_shares.lock().await.contains_key(&transfer_id) { + anyhow::bail!("transfer is not an active share"); + } self.access_policy .approve_endpoint(transfer_id, endpoint_id.clone()) .await; diff --git a/crates/vnidrop/src/tests/access_policy.rs b/crates/vnidrop/src/tests/access_policy.rs index 4302088..fceedd8 100644 --- a/crates/vnidrop/src/tests/access_policy.rs +++ b/crates/vnidrop/src/tests/access_policy.rs @@ -82,3 +82,16 @@ async fn public_mode_allows_without_session() { ); assert_eq!(policy.decide(7, None).await, AccessDecision::Allow); } + +#[tokio::test] +async fn approve_endpoint_grants_time_limited_session() { + let policy = AccessPolicy::new(); + policy + .set_mode(11, TransferAccessMode::ApprovalRequired) + .await; + policy.approve_endpoint(11, "node-b".to_string()).await; + assert_eq!( + policy.decide(11, Some("node-b")).await, + AccessDecision::Allow + ); +} diff --git a/crates/vnidrop/src/tests/filesystem.rs b/crates/vnidrop/src/tests/filesystem.rs index 93f4b5c..5a5f678 100644 --- a/crates/vnidrop/src/tests/filesystem.rs +++ b/crates/vnidrop/src/tests/filesystem.rs @@ -62,6 +62,26 @@ fn file_descriptor_source_rejects_invalid_values() { } } +#[cfg(unix)] +#[test] +fn file_descriptor_source_rejects_directory_fds() { + let temp = tempfile::tempdir().unwrap(); + let dir = std::fs::File::open(temp.path()).unwrap(); + use std::os::fd::AsRawFd; + let error = collect_import_files(vec![ShareSource { + kind: SourceKind::FileDescriptor, + value: dir.as_raw_fd().to_string(), + display_name: Some("folder".to_string()), + is_directory: false, + }]) + .unwrap_err() + .to_string(); + assert!( + error.contains("regular file"), + "directory fd must be rejected: {error}" + ); +} + #[test] fn android_content_uri_must_be_opened_by_platform_code() { let error = collect_import_files(vec![ShareSource { diff --git a/crates/vnidrop/src/tests/limits.rs b/crates/vnidrop/src/tests/limits.rs index 0733c3c..e462d0c 100644 --- a/crates/vnidrop/src/tests/limits.rs +++ b/crates/vnidrop/src/tests/limits.rs @@ -5,6 +5,14 @@ fn default_limits_are_valid() { CoreLimits::default().validate().unwrap(); } +#[test] +fn default_limits_bound_ticket_and_approval_pressure() { + let limits = CoreLimits::default(); + assert!(limits.max_ticket_bytes <= 256 * 1024); + assert!(limits.max_pending_approvals <= 64); + assert!(limits.max_total_bytes <= 256 * 1024 * 1024 * 1024); +} + #[test] fn zero_limit_is_rejected() { let limits = CoreLimits { diff --git a/crates/vnidrop/tests/lifecycle.rs b/crates/vnidrop/tests/lifecycle.rs index 7a96b30..ca8a0a5 100644 --- a/crates/vnidrop/tests/lifecycle.rs +++ b/crates/vnidrop/tests/lifecycle.rs @@ -251,6 +251,34 @@ fn access_mode_update_requires_active_persisted_share() { .is_err()); } +#[test] +fn approve_endpoint_requires_active_share_and_nonempty_id() { + let source_dir = tempfile::tempdir().unwrap(); + let source_path = source_dir.path().join("shared.txt"); + std::fs::write(&source_path, b"content").unwrap(); + let sender = TestNode::new(); + let share = share_path(&sender.core, &source_path, 42, "shared.txt", false); + + assert!(sender + .core + .approve_endpoint_for_transfer(share.transfer_id, " ".to_string()) + .is_err()); + assert!(sender + .core + .approve_endpoint_for_transfer(999, "endpoint-a".to_string()) + .is_err()); + sender + .core + .approve_endpoint_for_transfer(share.transfer_id, "endpoint-a".to_string()) + .unwrap(); + + sender.core.cancel_transfer(share.transfer_id).unwrap(); + assert!(sender + .core + .approve_endpoint_for_transfer(share.transfer_id, "endpoint-a".to_string()) + .is_err()); +} + #[test] fn source_limit_rejection_creates_no_transfer_state() { let core_dir = tempfile::tempdir().unwrap(); diff --git a/shared/src/androidMain/kotlin/com/vnidrop/app/core/FileSystemService.android.kt b/shared/src/androidMain/kotlin/com/vnidrop/app/core/FileSystemService.android.kt index c986f21..81f9db3 100644 --- a/shared/src/androidMain/kotlin/com/vnidrop/app/core/FileSystemService.android.kt +++ b/shared/src/androidMain/kotlin/com/vnidrop/app/core/FileSystemService.android.kt @@ -225,8 +225,7 @@ private class AndroidMediaStoreDownloadsSink( "MediaStore Downloads requires Android 10 or newer" } check(relativePath !in pending) { "Output stream is already open for $relativePath" } - val parts = relativePath.split('/').filter { it.isNotBlank() } - require(parts.isNotEmpty()) { "relative path must not be empty" } + val parts = requireSafeRelativePathParts(relativePath) val finalName = parts.last() val relativeDir = mediaStoreRelativePath(parts.dropLast(1)) check(!mediaStoreItemExists(finalName, relativeDir)) { @@ -346,6 +345,8 @@ private class AndroidTreeReceiveOutputSink( override fun startFile(relativePath: String) { check(relativePath !in pending) { "Output stream is already open for $relativePath" } + // Defense in depth: Rust also validates, but sinks must reject traversal alone. + requireSafeRelativePathParts(relativePath) val (parent, finalName) = resolveParent(relativePath) check(findChild(parent, finalName) == null) { "Destination already exists: $relativePath" } val temporaryName = ".$finalName.vnidrop-${UUID.randomUUID()}.part" @@ -391,8 +392,7 @@ private class AndroidTreeReceiveOutputSink( } private fun resolveParent(relativePath: String): Pair { - val parts = relativePath.split('/').filter { it.isNotBlank() } - require(parts.isNotEmpty()) { "relative path must not be empty" } + val parts = requireSafeRelativePathParts(relativePath) var parent = DocumentsContract.buildDocumentUriUsingTree( treeUri, DocumentsContract.getTreeDocumentId(treeUri), @@ -428,3 +428,18 @@ private class AndroidTreeReceiveOutputSink( return null } } + +/** + * Split a receive relative path and reject traversal / absolute-style components. + * Rust already validates; this keeps Android sinks safe if called incorrectly. + */ +private fun requireSafeRelativePathParts(relativePath: String): List { + val parts = relativePath.split('/').filter { it.isNotBlank() } + require(parts.isNotEmpty()) { "relative path must not be empty" } + require(parts.none { part -> + part == "." || part == ".." || part.contains('\\') || part.contains('\u0000') + }) { + "relative path contains invalid components: $relativePath" + } + return parts +} diff --git a/shared/src/commonMain/composeResources/values/strings.xml b/shared/src/commonMain/composeResources/values/strings.xml index 2bc62c7..04aa81a 100644 --- a/shared/src/commonMain/composeResources/values/strings.xml +++ b/shared/src/commonMain/composeResources/values/strings.xml @@ -22,6 +22,7 @@ You approve or refuse every new receiver. Anyone with this transfer No approval is required. Only use this for files you are comfortable sharing. + Anyone who has the ticket can download until you stop the share. Do not use this for private or sensitive files. Size unavailable Transfer created. Transfer details @@ -173,6 +174,7 @@ On Off Connection request + Endpoint ID: %1$s %1$d requests are waiting Ready Event log diff --git a/shared/src/commonMain/kotlin/com/vnidrop/app/feature/approvals/ApprovalCoordinator.kt b/shared/src/commonMain/kotlin/com/vnidrop/app/feature/approvals/ApprovalCoordinator.kt index da19e6f..82832f4 100644 --- a/shared/src/commonMain/kotlin/com/vnidrop/app/feature/approvals/ApprovalCoordinator.kt +++ b/shared/src/commonMain/kotlin/com/vnidrop/app/feature/approvals/ApprovalCoordinator.kt @@ -27,6 +27,8 @@ data class PendingApproval( val transferName: String, val receiverName: String?, val receiverDeviceName: String?, + /** Cryptographic peer identity from the Iroh connection — not display-name spoofable. */ + val remoteEndpointId: String, val requestedAt: Long, ) @@ -162,6 +164,7 @@ private fun ReceiverRequestModel.toPending(): PendingApproval = PendingApproval( transferName = transferName, receiverName = receiverName, receiverDeviceName = receiverDeviceName, + remoteEndpointId = remoteEndpointId, requestedAt = requestedAt, ) diff --git a/shared/src/commonMain/kotlin/com/vnidrop/app/feature/approvals/ApprovalModalHost.kt b/shared/src/commonMain/kotlin/com/vnidrop/app/feature/approvals/ApprovalModalHost.kt index d395624..bb3ecc8 100644 --- a/shared/src/commonMain/kotlin/com/vnidrop/app/feature/approvals/ApprovalModalHost.kt +++ b/shared/src/commonMain/kotlin/com/vnidrop/app/feature/approvals/ApprovalModalHost.kt @@ -31,6 +31,7 @@ import com.vnidrop.app.ui.theme.LocalVniDropColors import org.jetbrains.compose.resources.stringResource import vnidrop.shared.generated.resources.Res import vnidrop.shared.generated.resources.approval_connection_request +import vnidrop.shared.generated.resources.approval_endpoint_id import vnidrop.shared.generated.resources.approval_pending_count import vnidrop.shared.generated.resources.button_approve import vnidrop.shared.generated.resources.button_refuse @@ -78,6 +79,12 @@ fun ApprovalModalHost( style = MaterialTheme.typography.bodyLarge, color = colors.foregroundLight, ) + // Trusted identity is the endpoint id; display names are peer-provided. + Text( + stringResource(Res.string.approval_endpoint_id, request.remoteEndpointId), + style = MaterialTheme.typography.bodySmall, + color = colors.foregroundLighter, + ) if (state.pending.size > 1) { Text( stringResource(Res.string.approval_pending_count, state.pending.size), diff --git a/shared/src/commonMain/kotlin/com/vnidrop/app/feature/send/TransferComposer.kt b/shared/src/commonMain/kotlin/com/vnidrop/app/feature/send/TransferComposer.kt index c2af833..5a3bedb 100644 --- a/shared/src/commonMain/kotlin/com/vnidrop/app/feature/send/TransferComposer.kt +++ b/shared/src/commonMain/kotlin/com/vnidrop/app/feature/send/TransferComposer.kt @@ -51,6 +51,7 @@ import vnidrop.shared.generated.resources.field_sender_name import vnidrop.shared.generated.resources.field_transfer_name import vnidrop.shared.generated.resources.send_access_anyone import vnidrop.shared.generated.resources.send_access_anyone_description +import vnidrop.shared.generated.resources.send_access_anyone_warning import vnidrop.shared.generated.resources.send_access_approval import vnidrop.shared.generated.resources.send_access_approval_description import vnidrop.shared.generated.resources.send_access_title @@ -166,6 +167,13 @@ private fun ReviewFileStep( selected = state.accessPolicy == ShareAccessPolicy.AnyoneWithTransfer, onClick = { onAccessPolicyChanged(ShareAccessPolicy.AnyoneWithTransfer) }, ) + if (state.accessPolicy == ShareAccessPolicy.AnyoneWithTransfer) { + Text( + stringResource(Res.string.send_access_anyone_warning), + color = LocalVniDropColors.current.destructiveDefault, + style = MaterialTheme.typography.bodySmall, + ) + } if (windowClass == WindowClass.Phone) { Column(verticalArrangement = Arrangement.spacedBy(8.dp)) { ShareButton(state, coreInitialized, onCreateShare, Modifier.fillMaxWidth()) diff --git a/shared/src/jvmTest/kotlin/com/vnidrop/app/ui/FoundationComposeTest.kt b/shared/src/jvmTest/kotlin/com/vnidrop/app/ui/FoundationComposeTest.kt index 54f9fd7..e38f947 100644 --- a/shared/src/jvmTest/kotlin/com/vnidrop/app/ui/FoundationComposeTest.kt +++ b/shared/src/jvmTest/kotlin/com/vnidrop/app/ui/FoundationComposeTest.kt @@ -427,6 +427,7 @@ class FoundationComposeTest { transferName = "Photos", receiverName = "Alice", receiverDeviceName = "Phone", + remoteEndpointId = "endpoint-alice", requestedAt = 1L, )