From d0c8d8774a4d9a3cf7c94413d11258f924f72748 Mon Sep 17 00:00:00 2001 From: Hammed Abass Date: Mon, 13 Jul 2026 18:54:28 +0200 Subject: [PATCH] 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, )