mirror of
https://github.com/sudosylabs/vnidrop.git
synced 2026-08-05 02:29:55 +02:00
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.
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
|
||||
@@ -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)]
|
||||
|
||||
@@ -582,6 +582,24 @@ fn duplicate_file_descriptor(value: &str) -> Result<OwnedFd> {
|
||||
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::<libc::stat>::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) })
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in New Issue
Block a user